| Message ID | 1502121043-16895-1-git-send-email-i.maximets@samsung.com |
|---|---|
| State | Changes Requested |
| Headers | show |
On Mon, Aug 7, 2017 at 8:50 AM, Ilya Maximets <i.maximets@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@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. Thanks.
On 07.08.2017 23:24, Andy Zhou wrote: > On Mon, Aug 7, 2017 at 8:50 AM, Ilya Maximets <i.maximets@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@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. 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. What do you think? > > Thanks.
On Mon, Aug 7, 2017 at 11:01 PM, Ilya Maximets <i.maximets@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@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@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?
On 08.08.2017 21:36, Andy Zhou wrote: > On Mon, Aug 7, 2017 at 11:01 PM, Ilya Maximets <i.maximets@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@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@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?
> 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.) Let me try again. As you ave pointed out, it is not really unsafe to use the function at those calling sites, I think dp_packet_batch_add__() is a suitable name. In case you prefer a different function name, that's fine too, I just don't think unsafe is accurate.
On 09.08.2017 17:19, Andy Zhou wrote: >> 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.) > > Let me try again. As you ave pointed out, it is not really unsafe to > use the function > at those calling sites, I think dp_packet_batch_add__() is a suitable > name. In case > you prefer a different function name, that's fine too, I just don't > think unsafe is accurate. Oh. What I'm trying to achieve: 1. Eliminate direct usages of batch internal structure. 2. Don't check the boundaries in lib/dpif-netdev.c:packet_batch_per_flow_update(). The second goal requires existence of the exposed to the end users function, which will not check the boundaries while adding packet to batch. dp_packet_batch_add__() is not suitable in this case, because it has "__" as a suffix and should not be exposed to the end users because of this. Could you suggest another dp_packet_batch_XXX() name (which can be exposed to the end user) for the function that doesn't check the boundaries instead of 'add_unsafe', if you think it is not accurate? Best regards, Ilya Maximets.
On Fri, Aug 11, 2017 at 7:41 AM, Ilya Maximets <i.maximets@samsung.com> wrote: > > Could you suggest another dp_packet_batch_XXX() name (which can be exposed > to the end user) for the function that doesn't check the boundaries instead > of 'add_unsafe', if you think it is not accurate? How about using an existing API, dp_packet_batch_refill()
diff --git a/lib/dp-packet.h b/lib/dp-packet.h index 9dbb611..8ab14ce 100644 --- a/lib/dp-packet.h +++ b/lib/dp-packet.h @@ -685,11 +685,18 @@ dp_packet_batch_init(struct dp_packet_batch *batch) } static inline void +dp_packet_batch_add_unsafe(struct dp_packet_batch *batch, + struct dp_packet *packet) +{ + batch->packets[batch->count++] = packet; +} + +static inline void dp_packet_batch_add__(struct dp_packet_batch *batch, struct dp_packet *packet, size_t limit) { if (batch->count < limit) { - batch->packets[batch->count++] = packet; + dp_packet_batch_add_unsafe(batch, packet); } else { dp_packet_delete(packet); } @@ -764,7 +771,7 @@ dp_packet_batch_clone(struct dp_packet_batch *dst, dp_packet_batch_init(dst); DP_PACKET_BATCH_FOR_EACH (packet, src) { - dp_packet_batch_add(dst, dp_packet_clone(packet)); + dp_packet_batch_add_unsafe(dst, dp_packet_clone(packet)); } dst->trunc = src->trunc; } diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c index 8ecc9d1..d60f071 100644 --- a/lib/dpif-netdev.c +++ b/lib/dpif-netdev.c @@ -4604,7 +4604,7 @@ packet_batch_per_flow_update(struct packet_batch_per_flow *batch, { batch->byte_count += dp_packet_size(packet); batch->tcp_flags |= miniflow_get_tcp_flags(mf); - batch->array.packets[batch->array.count++] = packet; + dp_packet_batch_add_unsafe(&batch->array, packet); } static inline void diff --git a/lib/netdev-bsd.c b/lib/netdev-bsd.c index 8a4cdb3..92f1253 100644 --- a/lib/netdev-bsd.c +++ b/lib/netdev-bsd.c @@ -640,8 +640,7 @@ netdev_bsd_rxq_recv(struct netdev_rxq *rxq_, struct dp_packet_batch *batch) if (retval) { dp_packet_delete(packet); } else { - batch->packets[0] = packet; - batch->count = 1; + dp_packet_batch_init_packet(batch, packet); } return retval; } diff --git a/lib/netdev-dummy.c b/lib/netdev-dummy.c index 752f157..b5a35b7 100644 --- a/lib/netdev-dummy.c +++ b/lib/netdev-dummy.c @@ -1017,8 +1017,7 @@ netdev_dummy_rxq_recv(struct netdev_rxq *rxq_, struct dp_packet_batch *batch) netdev->stats.rx_bytes += dp_packet_size(packet); ovs_mutex_unlock(&netdev->mutex); - batch->packets[0] = packet; - batch->count = 1; + dp_packet_batch_init_packet(batch, packet); return 0; } diff --git a/tests/test-conntrack.c b/tests/test-conntrack.c index 6a77db8..28a7dc0 100644 --- a/tests/test-conntrack.c +++ b/tests/test-conntrack.c @@ -175,7 +175,7 @@ pcap_batch_execute_conntrack(struct conntrack *ct, NULL, NULL, NULL, NULL); dp_packet_batch_init(&new_batch); } - new_batch.packets[new_batch.count++] = packet;; + dp_packet_batch_add(&new_batch, packet); } if (!dp_packet_batch_is_empty(&new_batch)) {
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@samsung.com> --- lib/dp-packet.h | 11 +++++++++-- lib/dpif-netdev.c | 2 +- lib/netdev-bsd.c | 3 +-- lib/netdev-dummy.c | 3 +-- tests/test-conntrack.c | 2 +- 5 files changed, 13 insertions(+), 8 deletions(-)