[ovs-dev] dp-packet: Introduce dp_packet_batch_add_unsafe().

Message ID 1502121043-16895-1-git-send-email-i.maximets@samsung.com
State New
Headers show

Commit Message

Ilya Maximets Aug. 7, 2017, 3:50 p.m.
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(-)

Comments

Andy Zhou Aug. 7, 2017, 8:24 p.m. | #1
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.
Ilya Maximets Aug. 8, 2017, 6:01 a.m. | #2
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.
Andy Zhou Aug. 8, 2017, 6:36 p.m. | #3
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?
Ilya Maximets Aug. 9, 2017, 1:16 p.m. | #4
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?
Andy Zhou Aug. 9, 2017, 2:19 p.m. | #5
> 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.
Ilya Maximets Aug. 11, 2017, 2:41 p.m. | #6
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.
Andy Zhou Aug. 11, 2017, 6:39 p.m. | #7
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()

Patch

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)) {