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 d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev