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

Reply via email to