[ovs-dev,v11,03/14] dp-packet: Fix allocated size on DPDK init.

Message ID 1539188552-129083-4-git-send-email-tiago.lam@intel.com
State Changes Requested
Delegated to: Ian Stokes
Headers show
Series
  • Support multi-segment mbufs
Related show

Commit Message

Lam, Tiago Oct. 10, 2018, 4:22 p.m.
When enabled with DPDK OvS deals with two types of packets, the ones
coming from the mempool and the ones locally created by OvS - which are
copied to mempool mbufs before output. In the latter, the space is
allocated from the system, while in the former the mbufs are allocated
from a mempool, which takes care of initialising them appropriately.

In the current implementation, during mempool's initialisation of mbufs,
dp_packet_set_allocated() is called from dp_packet_init_dpdk() without
considering that the allocated space, in the case of multi-segment
mbufs, might be greater than a single mbuf.  Furthermore, given that
dp_packet_init_dpdk() is on the code path that's called upon mempool's
initialisation, a call to dp_packet_set_allocated() is redundant, since
mempool takes care of initialising it.

To fix this, dp_packet_set_allocated() is no longer called after
initialisation of a mempool, only in dp_packet_init__(), which is still
called by OvS when initialising locally created packets.

Signed-off-by: Tiago Lam <tiago.lam@intel.com>
Acked-by: Eelco Chaudron <echaudro@redhat.com>
Acked-by: Flavio Leitner <fbl@sysclose.org>
---
 lib/dp-packet.c   | 3 +--
 lib/dp-packet.h   | 2 +-
 lib/netdev-dpdk.c | 2 +-
 3 files changed, 3 insertions(+), 4 deletions(-)

Comments

Ian Stokes Oct. 17, 2018, 11:11 a.m. | #1
> When enabled with DPDK OvS deals with two types of packets, the ones
> coming from the mempool and the ones locally created by OvS - which are
> copied to mempool mbufs before output. In the latter, the space is
> allocated from the system, while in the former the mbufs are allocated
> from a mempool, which takes care of initialising them appropriately.
> 
> In the current implementation, during mempool's initialisation of mbufs,
> dp_packet_set_allocated() is called from dp_packet_init_dpdk() without
> considering that the allocated space, in the case of multi-segment mbufs,
> might be greater than a single mbuf.  Furthermore, given that
> dp_packet_init_dpdk() is on the code path that's called upon mempool's
> initialisation, a call to dp_packet_set_allocated() is redundant, since
> mempool takes care of initialising it.
> 
> To fix this, dp_packet_set_allocated() is no longer called after
> initialisation of a mempool, only in dp_packet_init__(), which is still
> called by OvS when initialising locally created packets.

Thanks for this Tiago, 1 minor comment below.

> 
> Signed-off-by: Tiago Lam <tiago.lam@intel.com>
> Acked-by: Eelco Chaudron <echaudro@redhat.com>
> Acked-by: Flavio Leitner <fbl@sysclose.org>
> ---
>  lib/dp-packet.c   | 3 +--
>  lib/dp-packet.h   | 2 +-
>  lib/netdev-dpdk.c | 2 +-
>  3 files changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/lib/dp-packet.c b/lib/dp-packet.c index 443c225..782e7c2
> 100644
> --- a/lib/dp-packet.c
> +++ b/lib/dp-packet.c
> @@ -99,9 +99,8 @@ dp_packet_use_const(struct dp_packet *b, const void
> *data, size_t size)
>   * buffer.  Here, non-transient ovs dp-packet fields are initialized for
>   * packets that are part of a DPDK memory pool. */  void -

Although not visible in the patch here, the comment for dp_packet_init_dpdk() still makes reference to ''allocated'.
As 'allocated' is removed the comment should be reworked also to reflect it also.

Ian

> dp_packet_init_dpdk(struct dp_packet *b, size_t allocated)
> +dp_packet_init_dpdk(struct dp_packet *b)
>  {
> -    dp_packet_set_allocated(b, allocated);
>      b->source = DPBUF_DPDK;
>  }
> 
> diff --git a/lib/dp-packet.h b/lib/dp-packet.h index b948fe1..6376039
> 100644
> --- a/lib/dp-packet.h
> +++ b/lib/dp-packet.h
> @@ -114,7 +114,7 @@ void dp_packet_use(struct dp_packet *, void *,
> size_t);  void dp_packet_use_stub(struct dp_packet *, void *, size_t);
> void dp_packet_use_const(struct dp_packet *, const void *, size_t);
> 
> -void dp_packet_init_dpdk(struct dp_packet *, size_t allocated);
> +void dp_packet_init_dpdk(struct dp_packet *);
> 
>  void dp_packet_init(struct dp_packet *, size_t);  void
> dp_packet_uninit(struct dp_packet *); diff --git a/lib/netdev-dpdk.c
> b/lib/netdev-dpdk.c index 11659eb..b6a22bd 100644
> --- a/lib/netdev-dpdk.c
> +++ b/lib/netdev-dpdk.c
> @@ -550,7 +550,7 @@ ovs_rte_pktmbuf_init(struct rte_mempool *mp
> OVS_UNUSED,  {
>      struct rte_mbuf *pkt = _p;
> 
> -    dp_packet_init_dpdk((struct dp_packet *) pkt, pkt->buf_len);
> +    dp_packet_init_dpdk((struct dp_packet *) pkt);
>  }
> 
>  static int
> --
> 2.7.4

Patch

diff --git a/lib/dp-packet.c b/lib/dp-packet.c
index 443c225..782e7c2 100644
--- a/lib/dp-packet.c
+++ b/lib/dp-packet.c
@@ -99,9 +99,8 @@  dp_packet_use_const(struct dp_packet *b, const void *data, size_t size)
  * buffer.  Here, non-transient ovs dp-packet fields are initialized for
  * packets that are part of a DPDK memory pool. */
 void
-dp_packet_init_dpdk(struct dp_packet *b, size_t allocated)
+dp_packet_init_dpdk(struct dp_packet *b)
 {
-    dp_packet_set_allocated(b, allocated);
     b->source = DPBUF_DPDK;
 }
 
diff --git a/lib/dp-packet.h b/lib/dp-packet.h
index b948fe1..6376039 100644
--- a/lib/dp-packet.h
+++ b/lib/dp-packet.h
@@ -114,7 +114,7 @@  void dp_packet_use(struct dp_packet *, void *, size_t);
 void dp_packet_use_stub(struct dp_packet *, void *, size_t);
 void dp_packet_use_const(struct dp_packet *, const void *, size_t);
 
-void dp_packet_init_dpdk(struct dp_packet *, size_t allocated);
+void dp_packet_init_dpdk(struct dp_packet *);
 
 void dp_packet_init(struct dp_packet *, size_t);
 void dp_packet_uninit(struct dp_packet *);
diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
index 11659eb..b6a22bd 100644
--- a/lib/netdev-dpdk.c
+++ b/lib/netdev-dpdk.c
@@ -550,7 +550,7 @@  ovs_rte_pktmbuf_init(struct rte_mempool *mp OVS_UNUSED,
 {
     struct rte_mbuf *pkt = _p;
 
-    dp_packet_init_dpdk((struct dp_packet *) pkt, pkt->buf_len);
+    dp_packet_init_dpdk((struct dp_packet *) pkt);
 }
 
 static int