On 08.08.2017 21:36, Andy Zhou wrote:
> On Mon, Aug 7, 2017 at 11:01 PM, Ilya Maximets <i.maxim...@samsung.com> wrote:
>> On 07.08.2017 23:24, Andy Zhou wrote:
>>> On Mon, Aug 7, 2017 at 8:50 AM, Ilya Maximets <i.maxim...@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.maxim...@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.
Maybe I don't fully understand what you're trying to say, but I want to use
unsafe function in dpif-netdev for per-flow packet batching (see the patch)
and it should not be internal for that case.
(It's safe to use unsafe function there because per-flow batches are
guaranteed to be less than original rx batch.)
>>
>> 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?
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
