On 3/20/25 12:49, Ilya Maximets wrote:
> 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.
Sounds good to me, I'll change it for v2 of the patch set.
> 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?
Sure, I will add some tests in v2.
Thanks a lot,
Martin
>
> 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
