Attention is currently required from: laforge, pespin. arehbein has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-bts/+/31533 ) Change subject: common: Have PCU socket connection use osmo_wqueue ...................................................................... Patch Set 4: (6 comments) File include/osmo-bts/pcuif_proto.h: https://gerrit.osmocom.org/c/osmo-bts/+/31533/comment/f10f9ec7_96d976af PS2, Line 8: #define PCU_SOCK_QLENGTH_MAX_DEFAULT 10 Ah or course I see what you mean, thanks for the explanation. I have put it in `bts.h`. It might also make sense to move it to libosmocore, because 10 seems to be the implicit default wqueue length (I grepped for calls to `osmo_wqueue_setup()`) for Osmocom's pcu, bsc and bts. File src/common/pcu_sock.c: https://gerrit.osmocom.org/c/osmo-bts/+/31533/comment/3cda8dd9_45754a6b PS2, Line 1092: return -1; Done https://gerrit.osmocom.org/c/osmo-bts/+/31533/comment/3173124d_1004b04e PS2, Line 1102: osmo_fd_write_disable(&state->upqueue.bfd); Ah alright, good to know (just read a bit into the code/`fd_set`-related matters). In the core of the function body before my change, `bfd` was already disabled similarly - only if the queue wasn't empty - and before writing. That's what I meant by saying I didn't change the logic (as the loop becoming part of `osmo_wqueue_bfd_cb` was the main change). `osmo-bts at 495cf39cb9e6bb54a7a5fd1195988f4a03f3171a` on branch master: ``` while (!llist_empty(&state->upqueue)) { struct msgb *msg, *msg2; struct gsm_pcu_if *pcu_prim; /* peek at the beginning of the queue */ msg = llist_entry(state->upqueue.next, struct msgb, list); pcu_prim = (struct gsm_pcu_if *)msg->data; osmo_fd_write_disable(bfd); /* bug hunter 8-): maybe someone forgot msgb_put(...) ? */ if (!msgb_length(msg)) { LOGP(DPCU, LOGL_ERROR, "message type (%d) with ZERO " "bytes!\n", pcu_prim->msg_type); goto dontsend; } /* try to send it over the socket */ rc = write(bfd->fd, msgb_data(msg), msgb_length(msg)); if (rc == 0) goto close; if (rc < 0) { if (errno == EAGAIN) { osmo_fd_write_enable(bfd); break; } goto close; } dontsend: /* _after_ we send it, we can deueue */ msg2 = msgb_dequeue(&state->upqueue); assert(msg == msg2); msgb_free(msg); } return 0; ``` I think I've seen the same snippet in other socket write callbacks of ours, too. The enable happens in `pcu_sock_send()` or if `-EAGAIN` is received. https://gerrit.osmocom.org/c/osmo-bts/+/31533/comment/96912ed1_9864d2c1 PS2, Line 1181: osmo_wqueue_init(&state->upqueue, PCU_SOCK_QLENGTH_MAX_DEFAULT); Done File src/common/pcu_sock.c: https://gerrit.osmocom.org/c/osmo-bts/+/31533/comment/16971e3b_72bf6e64 PS3, Line 993: LOGP(DLGLOBAL, LOGL_NOTICE, "PCU not reacting (more thatn %u messages waiting). Closing connection\n", Ah I see. I just went to the `LOGP` define and the comment above there doesn't mention that there are project-specific logging related headers like `include/osmocom/bsc/debug.h`. If `include/osmocom/<component>/debug.h` exists for different projects, it might be worth moving that code to libosmocore (seems like it's rather 'cross-component'). https://gerrit.osmocom.org/c/osmo-bts/+/31533/comment/3cdf00b0_e31e3f15 PS3, Line 993: LOGP(DLGLOBAL, LOGL_NOTICE, "PCU not reacting (more thatn %u messages waiting). Closing connection\n", Done -- To view, visit https://gerrit.osmocom.org/c/osmo-bts/+/31533 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-bts Gerrit-Branch: master Gerrit-Change-Id: Ia6e61dda4b3cd4bba76e6acb7771d70335062fe1 Gerrit-Change-Number: 31533 Gerrit-PatchSet: 4 Gerrit-Owner: arehbein &lt;arehbein(a)sysmocom.de&gt; Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: pespin &lt;pespin(a)sysmocom.de&gt; Gerrit-CC: laforge &lt;laforge(a)osmocom.org&gt; Gerrit-Attention: laforge &lt;laforge(a)osmocom.org&gt; Gerrit-Attention: pespin &lt;pespin(a)sysmocom.de&gt; Gerrit-Comment-Date: Thu, 02 Mar 2023 22:13:03 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: arehbein &lt;arehbein(a)sysmocom.de&gt; Comment-In-Reply-To: laforge &lt;laforge(a)osmocom.org&gt; Comment-In-Reply-To: pespin &lt;pespin(a)sysmocom.de&gt; Gerrit-MessageType: comment