Attention is currently required from: osmith, arehbein.
Hello osmith, Jenkins Builder, arehbein, fixeria,
I'd like you to reexamine a change. Please visit
https://gerrit.osmocom.org/c/osmo-bts/+/31924
to look at the new patch set (#2).
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(-)
git pull ssh://gerrit.osmocom.org:29418/osmo-bts refs/changes/24/31924/2
--
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(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: arehbein <arehbein(a)sysmocom.de>
Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Reviewer: osmith <osmith(a)sysmocom.de>
Gerrit-Attention: osmith <osmith(a)sysmocom.de>
Gerrit-Attention: arehbein <arehbein(a)sysmocom.de>
Gerrit-MessageType: newpatchset
Attention is currently required from: arehbein, laforge, fixeria.
pespin 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 5:
(1 comment)
File src/common/pcu_sock.c:
https://gerrit.osmocom.org/c/osmo-bts/+/31533/comment/87a0f064_fe79df2e
PS2, Line 1102: osmo_fd_write_disable(&state->upqueue.bfd);
> I have looked at some of our projects and seen similar code (code that disables the fd before calling write or transmitting data (probably also ultimately calling write) in osmo-trx, osmo-bts and libosmo-abis code.
That should happen only when the wqueue becomes empty, please have a look at the code and if it's not the case share a detailed example.
I see that this can be done in the previous because it's inside a while(!llist_empty()) loop, but then it makes no sense to call it on every loop iteration. In this new code here, you cannot do that.
> Probably isn't noticeable because sooner or later, the enable happens, anyways.
Yes, next time a packet is enqueued. That doesn't mean it's correct though.
I submitted https://gerrit.osmocom.org/c/osmo-bts/+/31924 to clean up this function, this may amke it easier for you to understand, feel free to rebase your work on top of it.
--
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: 5
Gerrit-Owner: arehbein <arehbein(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-CC: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-CC: laforge <laforge(a)osmocom.org>
Gerrit-Attention: arehbein <arehbein(a)sysmocom.de>
Gerrit-Attention: laforge <laforge(a)osmocom.org>
Gerrit-Attention: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Comment-Date: Wed, 15 Mar 2023 16:31:34 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: arehbein <arehbein(a)sysmocom.de>
Comment-In-Reply-To: pespin <pespin(a)sysmocom.de>
Gerrit-MessageType: comment
pespin has uploaded this change for review. ( 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, 31 insertions(+), 25 deletions(-)
git pull ssh://gerrit.osmocom.org:29418/osmo-bts refs/changes/24/31924/1
diff --git a/src/common/pcu_sock.c b/src/common/pcu_sock.c
index 145942b..b639026 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,32 @@
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;
+ msgb_enqueue(&state->upqueue, msg);
+ 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: 1
Gerrit-Owner: pespin <pespin(a)sysmocom.de>
Gerrit-MessageType: newchange
Attention is currently required from: laforge, fixeria, 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 5:
(2 comments)
File include/osmo-bts/pcuif_proto.h:
https://gerrit.osmocom.org/c/osmo-bts/+/31533/comment/d0edd8d4_78d2fe9f
PS4, Line 7:
> ws change, please revert this chunk
Done
File src/common/pcu_sock.c:
https://gerrit.osmocom.org/c/osmo-bts/+/31533/comment/a25ceeba_585fac4f
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). […]
@pespin I wrote this because I wanted to make sure the previous code was indeed wrong (as I said, I didn't change the logic with respect to that).
I have looked at some of our projects and seen similar code (code that disables the fd before calling `write` or transmitting data (probably also ultimately calling write) in osmo-trx, osmo-bts and libosmo-abis code.
Probably isn't noticeable because sooner or later, the enable happens, anyways.
Should I still change the code so that the fd is disabled only when there is no data to write anymore (and enabled after data is added to the msg queue)?
--
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: 5
Gerrit-Owner: arehbein <arehbein(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-CC: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-CC: laforge <laforge(a)osmocom.org>
Gerrit-Attention: laforge <laforge(a)osmocom.org>
Gerrit-Attention: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Attention: pespin <pespin(a)sysmocom.de>
Gerrit-Comment-Date: Wed, 15 Mar 2023 15:48:15 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: arehbein <arehbein(a)sysmocom.de>
Comment-In-Reply-To: fixeria <vyanitskiy(a)sysmocom.de>
Comment-In-Reply-To: pespin <pespin(a)sysmocom.de>
Gerrit-MessageType: comment