pespin has submitted this change. ( https://gerrit.osmocom.org/c/osmo-bts/+/31924 ) Change subject: Rewrite pcu_sock_write() ...................................................................... Rewrite pcu_sock_write() The code in that function is pretty rotten: * osmo_fd_write_disable() is called for each message in the queue, there's no need for that. Let's simply call it at the end if the queue is empty. * Asserting for obvious stuff like dequeue returning the first entry in the list. * Having error code path for empty message: That shouldn't happen, abort immediately. With all thse changes, the function is way simpler, easy to read and more efficient. Change-Id: I7ffff98cd8cdf2041bff486c3fde6f16365028d5 --- M src/common/pcu_sock.c 1 file changed, 32 insertions(+), 25 deletions(-) Approvals: Jenkins Builder: Verified fixeria: Looks good to me, but someone else must approve osmith: Looks good to me, but someone else must approve pespin: Looks good to me, approved diff --git a/src/common/pcu_sock.c b/src/common/pcu_sock.c index 145942b..5e4b06b 100644 --- a/src/common/pcu_sock.c +++ b/src/common/pcu_sock.c @@ -28,6 +28,7 @@ #include <inttypes.h> #include <osmocom/core/talloc.h> +#include <osmocom/core/utils.h> #include <osmocom/core/select.h> #include <osmocom/core/socket.h> #include <osmocom/gsm/gsm23003.h> @@ -1087,48 +1088,33 @@ static int pcu_sock_write(struct osmo_fd *bfd) { struct pcu_sock_state *state = bfd->data; + struct msgb *msg; int rc; - 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); - + while ((msg = msgb_dequeue(&state->upqueue))) { /* 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; - } + OSMO_ASSERT(msgb_length(msg) > 0); /* try to send it over the socket */ rc = write(bfd->fd, msgb_data(msg), msgb_length(msg)); - if (rc == 0) + if (OSMO_UNLIKELY(rc == 0)) goto close; - if (rc < 0) { + if (OSMO_UNLIKELY(rc < 0)) { if (errno == EAGAIN) { - osmo_fd_write_enable(bfd); - break; + /* Re-insert at the start of the queue, skip disabling fd WRITE */ + llist_add(&msg->list, &state->upqueue); + return 0; } goto close; } - -dontsend: - /* _after_ we send it, we can deueue */ - msg2 = msgb_dequeue(&state->upqueue); - assert(msg == msg2); msgb_free(msg); } + osmo_fd_write_disable(bfd); return 0; close: + msgb_free(msg); pcu_sock_close(state); - return -1; } -- To view, visit https://gerrit.osmocom.org/c/osmo-bts/+/31924 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-bts Gerrit-Branch: master Gerrit-Change-Id: I7ffff98cd8cdf2041bff486c3fde6f16365028d5 Gerrit-Change-Number: 31924 Gerrit-PatchSet: 2 Gerrit-Owner: pespin <pespin@sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: arehbein <arehbein@sysmocom.de> Gerrit-Reviewer: fixeria <vyanitskiy@sysmocom.de> Gerrit-Reviewer: osmith <osmith@sysmocom.de> Gerrit-Reviewer: pespin <pespin@sysmocom.de> Gerrit-MessageType: merged