[ovs-dev] [PATCH] dp-packet: Introduce dp_packet_batch_add_unsafe().

Andy Zhou azhou at ovn.org
Tue Aug 8 18:36:10 UTC 2017


On Mon, Aug 7, 2017 at 11:01 PM, Ilya Maximets <i.maximets at samsung.com> wrote:
> On 07.08.2017 23:24, Andy Zhou wrote:
>> On Mon, Aug 7, 2017 at 8:50 AM, Ilya Maximets <i.maximets at samsung.com> wrote:
>>> Almost all batch usecases covered by the new API introduced
>>> in commit 72c84bc2db23 ("dp-packet: Enhance packet batch APIs.")
>>> except unsafe batch addition. It used in dpif-netdev for fast
>>> per-flow batches filling. Introduction of this new function will
>>> allow to avoid most direct accesses to the batch structure.
>>>
>>> Function defined as 'inline' in .h file. Should not impact on
>>> performance.
>>>
>>> Additionally unsafe version now used in dp_packet_batch_clone()
>>> to speed up it a little, and also fixed few cases missed while
>>> API enchancing.
>>>
>>> Signed-off-by: Ilya Maximets <i.maximets at samsung.com>
>>
>> Those are worthwhile optimizations.  I have a minor suggestion for you
>> to consider:
>>
>> Instead of adding a new API dp_packet_batch_add_unsafe(), how about
>> just redefine
>> current APIs.
>>
>> dp_packet_batch_add() currently does not do much, it simply call
>> dp_packet_batch_add__(),
>> We can just turn this API into the safe version, same as
>> dp_packet_batch_add__().
>>
>> dp_packet_batch_add__() can be changed into the unsafe version. Use it
>> within the reset
>> of the patch where boundary checks can be avoided.
>
> dp_packet_batch_add__() should be renamed in this case to *add_unsafe()
> or something else anyway because "__" suffix means "internal use only".
> We should not expose function with such name for global using.

Because the add__() version omits boundary checks, it should not be used
by the end user. Rather it should only be used for implementing
dp_packet_batch_xxx APIs.
So, in this case, it is internal use only.  I don't think add_unsafe()
adds any additional value.
>
> Also, dp_packet_batch_add__() used inside dp_packet_batch_refill(). We
> will need to modify refill logic somehow in this case. --> more API
> changes.

We can make add__() API takes one additional argument, "idx",  Would this help?


More information about the dev mailing list