laforge has submitted this change. ( https://gerrit.osmocom.org/c/libosmocore/+/40584?usp=email )

Change subject: Avoid reusing pending buffer; append incoming data instead ......................................................................

Avoid reusing pending buffer; append incoming data instead

When reading from a stream, a single read may return only part of a message segment. In such cases, the partial data was stored in 'iofd->pending' and reused for subsequent reads to complete the message.

With upcoming changes that submit multiple read SQEs to io_uring, each read uses its own pre-submitted buffer. Reusing 'iofd->pending' for submitting next read is not possible, as the next read buffer is already submitted.

Instead, create a new msgb which is used for the read operation and, once completed, memcopy to the existing pending buffer, allowing message segments to accumulate until complete.

Change-Id: I08df9736ccc5e9a7df61ca6dcf94629ee010752f --- M src/core/osmo_io.c M src/core/osmo_io_internal.h M src/core/osmo_io_poll.c M src/core/osmo_io_uring.c 4 files changed, 29 insertions(+), 28 deletions(-)

Approvals: pespin: Looks good to me, approved Jenkins Builder: Verified

diff --git a/src/core/osmo_io.c b/src/core/osmo_io.c index aaf7bcc..6b4b919 100644 --- a/src/core/osmo_io.c +++ b/src/core/osmo_io.c @@ -174,36 +174,19 @@ talloc_free(msghdr); }

-/*! convenience wrapper to call msgb_alloc with parameters from osmo_io_fd */ -struct msgb *iofd_msgb_alloc(struct osmo_io_fd *iofd) +/*! convenience wrapper to call msgb_alloc with parameters from osmo_io_fd (of given size) */ +struct msgb *iofd_msgb_alloc2(struct osmo_io_fd *iofd, size_t size) { uint16_t headroom = iofd->msgb_alloc.headroom;

- OSMO_ASSERT(iofd->msgb_alloc.size <= 0xffff - headroom); - return msgb_alloc_headroom_c(iofd, iofd->msgb_alloc.size + headroom, headroom, "osmo_io_msgb"); + OSMO_ASSERT(size <= 0xffff - headroom); + return msgb_alloc_headroom_c(iofd, size + headroom, headroom, "osmo_io_msgb"); }

-/*! return the pending msgb in iofd or NULL if there is none*/ -struct msgb *iofd_msgb_pending(struct osmo_io_fd *iofd) +/*! convenience wrapper to call msgb_alloc with parameters from osmo_io_fd */ +struct msgb *iofd_msgb_alloc(struct osmo_io_fd *iofd) { - struct msgb *msg = NULL; - - msg = iofd->pending; - iofd->pending = NULL; - - return msg; -} - -/*! Return the pending msgb or allocate and return a new one */ -struct msgb *iofd_msgb_pending_or_alloc(struct osmo_io_fd *iofd) -{ - struct msgb *msg = NULL; - - msg = iofd_msgb_pending(iofd); - if (!msg) - msg = iofd_msgb_alloc(iofd); - - return msg; + return iofd_msgb_alloc2(iofd, iofd->msgb_alloc.size); }

/*! Enqueue a message to be sent. @@ -348,6 +331,25 @@ return; }

+ /* If we have a pending message, append the received message. + * If the pending message is not large enough, create a larger message. */ + if (OSMO_UNLIKELY(iofd->pending)) { + if (OSMO_UNLIKELY(msgb_tailroom(iofd->pending) < msgb_length(msg))) { + /* Data of msg does not fit into pending message. Allocate a new message that is larger. + * This implies that msgb_length(iofd->pending) + msgb_length(msg) > iofd.msgb_alloc.size. */ + pending = iofd_msgb_alloc2(iofd, msgb_length(iofd->pending) + msgb_length(msg)); + OSMO_ASSERT(pending); + memcpy(msgb_put(pending, msgb_length(iofd->pending)), msgb_data(iofd->pending), + msgb_length(iofd->pending)); + msgb_free(iofd->pending); + iofd->pending = pending; + } + memcpy(msgb_put(iofd->pending, msgb_length(msg)), msgb_data(msg), msgb_length(msg)); + msgb_free(msg); + msg = iofd->pending; + iofd->pending = NULL; + } + do { pending = NULL; res = iofd_handle_segmentation(iofd, msg, &pending); diff --git a/src/core/osmo_io_internal.h b/src/core/osmo_io_internal.h index a4b0749..79b3fa9 100644 --- a/src/core/osmo_io_internal.h +++ b/src/core/osmo_io_internal.h @@ -153,9 +153,8 @@ struct iofd_msghdr *iofd_msghdr_alloc(struct osmo_io_fd *iofd, enum iofd_msg_action action, struct msgb *msg, size_t cmsg_size); void iofd_msghdr_free(struct iofd_msghdr *msghdr);

+struct msgb *iofd_msgb_alloc2(struct osmo_io_fd *iofd, size_t size); struct msgb *iofd_msgb_alloc(struct osmo_io_fd *iofd); -struct msgb *iofd_msgb_pending(struct osmo_io_fd *iofd); -struct msgb *iofd_msgb_pending_or_alloc(struct osmo_io_fd *iofd);

void iofd_handle_recv(struct osmo_io_fd *iofd, struct msgb *msg, int rc, struct iofd_msghdr *msghdr); void iofd_handle_send_completion(struct osmo_io_fd *iofd, int rc, struct iofd_msghdr *msghdr); diff --git a/src/core/osmo_io_poll.c b/src/core/osmo_io_poll.c index 8782adf..466973d 100644 --- a/src/core/osmo_io_poll.c +++ b/src/core/osmo_io_poll.c @@ -50,7 +50,7 @@ if (what & OSMO_FD_READ) { struct iofd_msghdr hdr;

- msg = iofd_msgb_pending_or_alloc(iofd); + msg = iofd_msgb_alloc(iofd); if (!msg) { LOGPIO(iofd, LOGL_ERROR, "Could not allocate msgb for reading\n"); OSMO_ASSERT(0); diff --git a/src/core/osmo_io_uring.c b/src/core/osmo_io_uring.c index 347461e..17853b6 100644 --- a/src/core/osmo_io_uring.c +++ b/src/core/osmo_io_uring.c @@ -140,7 +140,7 @@ struct iofd_msghdr *msghdr; struct io_uring_sqe *sqe;

- msg = iofd_msgb_pending_or_alloc(iofd); + msg = iofd_msgb_alloc(iofd); if (!msg) { LOGPIO(iofd, LOGL_ERROR, "Could not allocate msgb for reading\n"); OSMO_ASSERT(0);