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.
> 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