On 3/3/25 10:19, Martin Morgenstern via dev wrote:
> From: Felix Huettner <[email protected]>
>
> Previously, we reused the jsonrpc byteq with the state from a previous
> iteration with potentially less headroom. However, in case we have an
> empty byteq, we can reset the pointers to maximize the available headroom
> and thus, the number of bytes that we can process in a single iteration.
>
> Co-authored-by: Martin Morgenstern <[email protected]>
> Signed-off-by: Martin Morgenstern <[email protected]>
> Signed-off-by: Felix Huettner <[email protected]>
> ---
> lib/byteq.c | 17 +++++++++++++++++
> lib/byteq.h | 1 +
> lib/jsonrpc.c | 1 +
> 3 files changed, 19 insertions(+)
Thanks for the patch! The change seems reasonable to me in general, see
some comments below.
>
> diff --git a/lib/byteq.c b/lib/byteq.c
> index 3f865cf9e..0134a8f3b 100644
> --- a/lib/byteq.c
> +++ b/lib/byteq.c
> @@ -198,3 +198,20 @@ byteq_advance_head(struct byteq *q, unsigned int n)
> ovs_assert(byteq_headroom(q) >= n);
> q->head += n;
> }
> +
> +/* Optimize the byteq to have the most headroom available.
> + * Previous pointers returned by byteq_tail() or byteq_head() are potentially
> + * invalid afterwards. */
> +void
> +byteq_optimize(struct byteq *q)
The word 'optimize' is a little vague and it's not clear what this function
would actually do without reading the comment or the code.
I'd suggest to re-name it to 'byteq_fast_forward' or something similar.
This name suggests that we're moving the head/tail pointers forward to some
predefined position. This also removes the need to explain why we're not
setting them to zero.
The comment for the function should say that it is equivalent to advancing
both head and tail in an empty byteq by the current headroom size.
Also, should we add some basic unit test for this into tests/test-byteq.c?
Best regards, Ilya Maximets.
> +{
> + ovs_assert(byteq_is_empty(q));
> + /* Do not simply reset to zero, but to the next multiple of size because
> + * we currently have callers that depend on increasing values. */
> + unsigned int pos = q->head & (q->size - 1);
> + if (pos) {
> + /* Only advance head if we are not already at a multiple of size. */
> + q->head += q->size - pos;
> + }
> + q->tail = q->head;
> +}
> diff --git a/lib/byteq.h b/lib/byteq.h
> index d73e3684e..4b35c7987 100644
> --- a/lib/byteq.h
> +++ b/lib/byteq.h
> @@ -42,6 +42,7 @@ int byteq_read(struct byteq *, int fd);
>
> uint8_t *byteq_head(struct byteq *);
> int byteq_headroom(const struct byteq *);
> +void byteq_optimize(struct byteq *);
> void byteq_advance_head(struct byteq *, unsigned int n);
> int byteq_tailroom(const struct byteq *);
> const uint8_t *byteq_tail(const struct byteq *);
> diff --git a/lib/jsonrpc.c b/lib/jsonrpc.c
> index 0a2b8dd7d..eacfd7e26 100644
> --- a/lib/jsonrpc.c
> +++ b/lib/jsonrpc.c
> @@ -344,6 +344,7 @@ jsonrpc_recv(struct jsonrpc *rpc, struct jsonrpc_msg
> **msgp)
> size_t chunk;
> int retval;
>
> + byteq_optimize(&rpc->input);
> chunk = byteq_headroom(&rpc->input);
> retval = stream_recv(rpc->stream, byteq_head(&rpc->input),
> chunk);
> if (retval < 0) {
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
