Attention is currently required from: laforge, pespin.
6 comments:
File include/osmo-bts/pcuif_proto.h:
Patch Set #2, 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:
Patch Set #2, Line 1092: return -1;
osmo_wqueue_bfd_cb: […]
Done
Patch Set #2, 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.
Patch Set #2, 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:
Patch Set #3, 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').
Patch Set #3, Line 993: LOGP(DLGLOBAL, LOGL_NOTICE, "PCU not reacting (more thatn %u messages waiting). Closing connection\n",
typo: than
Done
To view, visit change 31533. To unsubscribe, or for help writing mail filters, visit settings.