Attention is currently required from: jolly.
pespin has posted comments on this change by jolly. ( https://gerrit.osmocom.org/c/libosmocore/+/40493?usp=email )
Change subject: Add multiple messages buffers to io_uring write operations
......................................................................
Patch Set 7:
(2 comments)
File src/core/osmo_io.c:
https://gerrit.osmocom.org/c/libosmocore/+/40493/comment/e78c3773_ac38c49c?… :
PS4, Line 444: /* Re-enqueue remaining buffers. */
> Then we must change the way the tx_queue stores. […]
No, what I mean is that in here a function should be called to re-arrange msgs inside msghdrs in the list to make sure we fill the arrays of msgs of the first msghdrs enqueued. Otherwise we may end up submitting half-filled msghdr->{msg,iov}[] as a consequence of this code path.
Feel free to submit a proper function to squeeze those in a follow-up commit, but at least add a TODO here in a comment explaining that. IMHO this should be implemented before doing any benchmarking.
File src/core/osmo_io.c:
https://gerrit.osmocom.org/c/libosmocore/+/40493/comment/426576b2_c1ebc583?… :
PS7, Line 442: /* Re-enqueue the complete msgb. */
This can be moved to the top of the function before the for loop, to simplify function now that you added the loop.
--
To view, visit https://gerrit.osmocom.org/c/libosmocore/+/40493?usp=email
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-Change-Id: I8c4e0a785cf66becd7fb5b2caf718c9724b56686
Gerrit-Change-Number: 40493
Gerrit-PatchSet: 7
Gerrit-Owner: jolly <andreas(a)eversberg.eu>
Gerrit-Reviewer: Jenkins Builder
Gerrit-CC: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: jolly <andreas(a)eversberg.eu>
Gerrit-Comment-Date: Wed, 23 Jul 2025 14:18:19 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: jolly <andreas(a)eversberg.eu>
Comment-In-Reply-To: pespin <pespin(a)sysmocom.de>
Attention is currently required from: jolly, laforge.
pespin has posted comments on this change by jolly. ( https://gerrit.osmocom.org/c/libosmocore/+/40491?usp=email )
Change subject: Add multiple messages buffers to struct iofd_msghdr
......................................................................
Patch Set 6:
(4 comments)
File src/core/osmo_io.c:
https://gerrit.osmocom.org/c/libosmocore/+/40491/comment/8fdce964_6ad3d2c2?… :
PS6, Line 747: * If your osmo_io_fd is in OSMO_IO_FD_MODE_READ_WRITE mode, this API function can be used to tell the
Please add proper doxygen documentation of params here.
https://gerrit.osmocom.org/c/libosmocore/+/40491/comment/155cebec_e6facfe4?… :
PS6, Line 748: * osmo_io code how many buffers should be read or written with a single read or write operation.
"read or write". Extra space.
https://gerrit.osmocom.org/c/libosmocore/+/40491/comment/97467d14_19cb241b?… :
PS6, Line 751: int osmo_iofd_set_io_buffers(struct osmo_io_fd *iofd, int buffers)
unsigned int buffers?
Only reason to use an "int" here would be to allow some sort of -1=default, which I don't see documented.
File src/core/osmo_io_internal.h:
https://gerrit.osmocom.org/c/libosmocore/+/40491/comment/03e7a402_06382545?… :
PS6, Line 143: int io_len;
this can also be a unsigned int. And even actually a uint8_t.
--
To view, visit https://gerrit.osmocom.org/c/libosmocore/+/40491?usp=email
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-Change-Id: I4fb1067de4615cc22cc6caf99b481491e7f2ef92
Gerrit-Change-Number: 40491
Gerrit-PatchSet: 6
Gerrit-Owner: jolly <andreas(a)eversberg.eu>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-CC: laforge <laforge(a)osmocom.org>
Gerrit-Attention: jolly <andreas(a)eversberg.eu>
Gerrit-Attention: laforge <laforge(a)osmocom.org>
Gerrit-Comment-Date: Wed, 23 Jul 2025 14:04:47 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Attention is currently required from: jolly, laforge.
pespin has posted comments on this change by jolly. ( https://gerrit.osmocom.org/c/libosmocore/+/40491?usp=email )
Change subject: Add multiple messages buffers to struct iofd_msghdr
......................................................................
Patch Set 6:
(1 comment)
File src/core/osmo_io_internal.h:
https://gerrit.osmocom.org/c/libosmocore/+/40491/comment/7f1cd00c_291ae212?… :
PS5, Line 143: int io_len;
> Without it, we would have extra loops. I want to avoid that. […]
Fine then, just wanted to make sure whether we can simply change the existing loops to something like "while(msgb[idx]) { ....; idx++ }".
--
To view, visit https://gerrit.osmocom.org/c/libosmocore/+/40491?usp=email
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-Change-Id: I4fb1067de4615cc22cc6caf99b481491e7f2ef92
Gerrit-Change-Number: 40491
Gerrit-PatchSet: 6
Gerrit-Owner: jolly <andreas(a)eversberg.eu>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-CC: laforge <laforge(a)osmocom.org>
Gerrit-Attention: jolly <andreas(a)eversberg.eu>
Gerrit-Attention: laforge <laforge(a)osmocom.org>
Gerrit-Comment-Date: Wed, 23 Jul 2025 14:01:26 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: jolly <andreas(a)eversberg.eu>
Comment-In-Reply-To: pespin <pespin(a)sysmocom.de>
Attention is currently required from: jolly, laforge.
pespin has posted comments on this change by jolly. ( https://gerrit.osmocom.org/c/libosmocore/+/40584?usp=email )
Change subject: Avoid reusing pending buffer; append incoming data instead
......................................................................
Patch Set 4:
(3 comments)
Commit Message:
https://gerrit.osmocom.org/c/libosmocore/+/40584/comment/7d712385_94a8c125?… :
PS3, Line 17: This change ensures that newly received data is appended to the existing
> Maybe I should state that I don't want to reuse the pending buffer for the read operation. […]
This is still confusing to me, because you are still using the term "pending buffer" twice.
IIUC on the second one you mean "to the buffer where next read will be done by the kernel"?
File src/core/osmo_io.c:
https://gerrit.osmocom.org/c/libosmocore/+/40584/comment/cf285de9_509310cd?… :
PS3, Line 178: struct msgb *iofd_msgb_alloc2(struct osmo_io_fd *iofd, size_t size)
> The application should only care about the length of data in the buffer not the size.
I disagree. Apps may want to make sure a msgb allocated is of at least a certain size, so that they can reuse the msgb with zero-copy by appending/prepending to it, etc.
If you say you garantee the msgb recevied by the user is of at least osmo_iofd msgb_size configured by the user, then I'm fine, otherwise we need to revisit this.
https://gerrit.osmocom.org/c/libosmocore/+/40584/comment/1afc42ee_da26b476?… :
PS3, Line 335: * If the pending message is not large enough, create a larger message. */
> The application should only care about the length of data in the buffer not the size.
My understanding is that since user expects no segment bigger than the configured msgb_size, then we should read() only up to the maximum msgb_size configured. Hence it can never happen that we read more than the size of the msgb.
--
To view, visit https://gerrit.osmocom.org/c/libosmocore/+/40584?usp=email
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-Change-Id: I08df9736ccc5e9a7df61ca6dcf94629ee010752f
Gerrit-Change-Number: 40584
Gerrit-PatchSet: 4
Gerrit-Owner: jolly <andreas(a)eversberg.eu>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: jolly <andreas(a)eversberg.eu>
Gerrit-Attention: laforge <laforge(a)osmocom.org>
Gerrit-Comment-Date: Wed, 23 Jul 2025 13:59:59 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: jolly <andreas(a)eversberg.eu>
Comment-In-Reply-To: pespin <pespin(a)sysmocom.de>
Attention is currently required from: jolly, laforge.
pespin has posted comments on this change by jolly. ( https://gerrit.osmocom.org/c/libosmocore/+/40489?usp=email )
Change subject: Allow io_uring_submit batching just ahead of poll/select
......................................................................
Patch Set 4: Code-Review-1
(6 comments)
Patchset:
PS4:
Fine with having the extra envvar for now (I guess we'll want to remove it later on once we figure out this new approach is good).
Marking -1 because the optimization to skip calling io_uring if not needed was not yet added. See comments attached.
File src/core/osmo_io_uring.c:
https://gerrit.osmocom.org/c/libosmocore/+/40489/comment/2c959aae_fcaa711d?… :
PS4, Line 187: io_uring_submit(&g_ring.ring);
else
g_io_uring_submit_needed = true;
https://gerrit.osmocom.org/c/libosmocore/+/40489/comment/5232545f_41388f48?… :
PS4, Line 330: io_uring_submit(&g_ring.ring);
else
g_io_uring_submit_needed = true;
https://gerrit.osmocom.org/c/libosmocore/+/40489/comment/cf237e81_db669c01?… :
PS4, Line 431: io_uring_submit(&g_ring.ring);
else
g_io_uring_submit_needed = true;
https://gerrit.osmocom.org/c/libosmocore/+/40489/comment/a42061d3_c42803d3?… :
PS4, Line 480: if (OSMO_LIKELY(!g_io_uring_batch))
else
g_io_uring_submit_needed = true;
(and yes, you probably want to have this as a static inline function to avoid logic duplication: io_uring_submit_needed()).
https://gerrit.osmocom.org/c/libosmocore/+/40489/comment/1d96d8f0_ed7dcc1a?… :
PS4, Line 554: io_uring_submit(&g_ring.ring);
if (OSMO_LIKELY(g_io_uring_submit_needed))
io_uring_submit(&g_ring.ring);
--
To view, visit https://gerrit.osmocom.org/c/libosmocore/+/40489?usp=email
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-Change-Id: Id34fe2ced32c63d15b14810e145744f7509064cc
Gerrit-Change-Number: 40489
Gerrit-PatchSet: 4
Gerrit-Owner: jolly <andreas(a)eversberg.eu>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-CC: laforge <laforge(a)osmocom.org>
Gerrit-Attention: jolly <andreas(a)eversberg.eu>
Gerrit-Attention: laforge <laforge(a)osmocom.org>
Gerrit-Comment-Date: Wed, 23 Jul 2025 13:54:52 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes