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
Ack
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;
osmo_wqueue_bfd_cb: […]
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).
This is wrong. You should disable the bfd after successful write() only if the queue is empty.
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);
I'm fine with whatever but use WQUEUE instead of the QLENGTH stuff :)
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",
the code above is using DPCU everywhere, so I don't see a need to use a non-PCU-specific log subsyst […]
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",
typo: than
Done