[ovs-dev,v10,08/14] dp-packet: copy data from multi-seg. DPDK mbuf

Message ID 1538151315-117132-9-git-send-email-tiago.lam@intel.com
State Superseded
Headers show
Series
  • Support multi-segment mbufs
Related show

Commit Message

Lam, Tiago Sept. 28, 2018, 4:15 p.m.
From: Michael Qiu <qiudayu@chinac.com>

When doing packet clone, if packet source is from DPDK driver,
multi-segment must be considered, and copy the segment's data one by
one.

Also, lots of DPDK mbuf's info is missed during a copy, like packet
type, ol_flags, etc.  That information is very important for DPDK to do
packets processing.

Co-authored-by: Mark Kavanagh <mark.b.kavanagh@intel.com>
Co-authored-by: Tiago Lam <tiago.lam@intel.com>

Signed-off-by: Michael Qiu <qiudayu@chinac.com>
Signed-off-by: Mark Kavanagh <mark.b.kavanagh@intel.com>
Signed-off-by: Tiago Lam <tiago.lam@intel.com>
Acked-by: Eelco Chaudron <echaudro@redhat.com>
---
 lib/dp-packet.c   | 69 ++++++++++++++++++++++++++++++++++++++++++++++---------
 lib/dp-packet.h   |  3 +++
 lib/netdev-dpdk.c |  1 +
 3 files changed, 62 insertions(+), 11 deletions(-)

Comments

Flavio Leitner Oct. 3, 2018, 6:26 p.m. | #1
On Fri, Sep 28, 2018 at 05:15:09PM +0100, Tiago Lam wrote:
> From: Michael Qiu <qiudayu@chinac.com>
> 
> When doing packet clone, if packet source is from DPDK driver,
> multi-segment must be considered, and copy the segment's data one by
> one.
> 
> Also, lots of DPDK mbuf's info is missed during a copy, like packet
> type, ol_flags, etc.  That information is very important for DPDK to do
> packets processing.
> 
> Co-authored-by: Mark Kavanagh <mark.b.kavanagh@intel.com>
> Co-authored-by: Tiago Lam <tiago.lam@intel.com>
> 
> Signed-off-by: Michael Qiu <qiudayu@chinac.com>
> Signed-off-by: Mark Kavanagh <mark.b.kavanagh@intel.com>
> Signed-off-by: Tiago Lam <tiago.lam@intel.com>
> Acked-by: Eelco Chaudron <echaudro@redhat.com>
> ---
>  lib/dp-packet.c   | 69 ++++++++++++++++++++++++++++++++++++++++++++++---------
>  lib/dp-packet.h   |  3 +++
>  lib/netdev-dpdk.c |  1 +
>  3 files changed, 62 insertions(+), 11 deletions(-)
> 
> diff --git a/lib/dp-packet.c b/lib/dp-packet.c
> index 167bf43..806640b 100644
> --- a/lib/dp-packet.c
> +++ b/lib/dp-packet.c
> @@ -48,6 +48,22 @@ dp_packet_use__(struct dp_packet *b, void *base, size_t allocated,
>      dp_packet_set_size(b, 0);
>  }
>  
> +#ifdef DPDK_NETDEV
> +void
> +dp_packet_copy_mbuf_flags(struct dp_packet *dst, const struct dp_packet *src)
> +{
> +    ovs_assert(dst != NULL && src != NULL);
> +    struct rte_mbuf *buf_dst = &(dst->mbuf);
> +    struct rte_mbuf buf_src = src->mbuf;
> +
> +    buf_dst->ol_flags = buf_src.ol_flags;
> +    buf_dst->packet_type = buf_src.packet_type;
> +    buf_dst->tx_offload = buf_src.tx_offload;

Nit: use dereferencing on both sides.

Also, since this is a simple function that only copies few fields, maybe it could
be in the dp-packet.h, so that netdev-dpdk.c can inline it.

> +}
> +#else
> +#define dp_packet_copy_mbuf_flags(arg1, arg2)

Nit2: since this is very specific to mbuf, we don't need to expose it
outside of DPDK.

> +#endif
> +
>  /* Initializes 'b' as an empty dp_packet that contains the 'allocated' bytes of
>   * memory starting at 'base'.  'base' should be the first byte of a region
>   * obtained from malloc().  It will be freed (with free()) if 'b' is resized or
> @@ -158,6 +174,44 @@ dp_packet_clone(const struct dp_packet *buffer)
>      return dp_packet_clone_with_headroom(buffer, 0);
>  }
>  
> +#ifdef DPDK_NETDEV
> +struct dp_packet *
> +dp_packet_clone_with_headroom(const struct dp_packet *b, size_t headroom) {
> +    struct dp_packet *new_buffer;
> +    uint32_t pkt_len = dp_packet_size(b);
> +
> +    /* copy multi-seg data */
> +    if (b->source == DPBUF_DPDK && !rte_pktmbuf_is_contiguous(&b->mbuf)) {
> +        void *dst = NULL;
> +        struct rte_mbuf *mbuf = CONST_CAST(struct rte_mbuf *, &b->mbuf);
> +
> +        new_buffer = dp_packet_new_with_headroom(pkt_len, headroom);
> +        dst = dp_packet_data(new_buffer);
> +        dp_packet_set_size(new_buffer, pkt_len);
> +
> +        if (!rte_pktmbuf_read(mbuf, 0, pkt_len, dst)) {

Here we leak new_buffer which is xmalloc'ed.

> +            return NULL;
> +        }
> +    } else {
> +        new_buffer = dp_packet_clone_data_with_headroom(dp_packet_data(b),
> +                                                        dp_packet_size(b),
> +                                                        headroom);
> +    }
> +
> +    /* Copy the following fields into the returned buffer: l2_pad_size,
> +     * l2_5_ofs, l3_ofs, l4_ofs, cutlen, packet_type and md. */
> +    memcpy(&new_buffer->l2_pad_size, &b->l2_pad_size,
> +           sizeof(struct dp_packet) -
> +           offsetof(struct dp_packet, l2_pad_size));
> +
> +    dp_packet_copy_mbuf_flags(new_buffer, b);
> +    if (dp_packet_rss_valid(new_buffer)) {
> +        new_buffer->mbuf.hash.rss = b->mbuf.hash.rss;
> +    }
> +
> +    return new_buffer;
> +}
> +#else
>  /* Creates and returns a new dp_packet whose data are copied from 'buffer'.
>   * The returned dp_packet will additionally have 'headroom' bytes of
>   * headroom. */
> @@ -165,32 +219,25 @@ struct dp_packet *
>  dp_packet_clone_with_headroom(const struct dp_packet *buffer, size_t headroom)
>  {
>      struct dp_packet *new_buffer;
> +    uint32_t pkt_len = dp_packet_size(buffer);
>  
>      new_buffer = dp_packet_clone_data_with_headroom(dp_packet_data(buffer),
> -                                                 dp_packet_size(buffer),
> -                                                 headroom);
> +                                                 pkt_len, headroom);
> +
>      /* Copy the following fields into the returned buffer: l2_pad_size,
>       * l2_5_ofs, l3_ofs, l4_ofs, cutlen, packet_type and md. */
>      memcpy(&new_buffer->l2_pad_size, &buffer->l2_pad_size,
>              sizeof(struct dp_packet) -
>              offsetof(struct dp_packet, l2_pad_size));
>  
> -#ifdef DPDK_NETDEV
> -    new_buffer->mbuf.ol_flags = buffer->mbuf.ol_flags;
> -#else
>      new_buffer->rss_hash_valid = buffer->rss_hash_valid;
> -#endif
> -
>      if (dp_packet_rss_valid(new_buffer)) {
> -#ifdef DPDK_NETDEV
> -        new_buffer->mbuf.hash.rss = buffer->mbuf.hash.rss;
> -#else
>          new_buffer->rss_hash = buffer->rss_hash;
> -#endif
>      }
>  
>      return new_buffer;
>  }
> +#endif

Nit3: Looks like the above functions share some code which could be on
another function or maybe move the copy specifics to a separate
functions.


>  /* Creates and returns a new dp_packet that initially contains a copy of the
>   * 'size' bytes of data starting at 'data' with no headroom or tailroom. */
> diff --git a/lib/dp-packet.h b/lib/dp-packet.h
> index aab8f62..cbf002c 100644
> --- a/lib/dp-packet.h
> +++ b/lib/dp-packet.h
> @@ -124,6 +124,9 @@ void dp_packet_init_dpdk(struct dp_packet *);
>  void dp_packet_init(struct dp_packet *, size_t);
>  void dp_packet_uninit(struct dp_packet *);
>  
> +void dp_packet_copy_mbuf_flags(struct dp_packet *dst,
> +                               const struct dp_packet *src);
> +
>  struct dp_packet *dp_packet_new(size_t);
>  struct dp_packet *dp_packet_new_with_headroom(size_t, size_t headroom);
>  struct dp_packet *dp_packet_clone(const struct dp_packet *);
> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
> index 3b11546..8484239 100644
> --- a/lib/netdev-dpdk.c
> +++ b/lib/netdev-dpdk.c
> @@ -2364,6 +2364,7 @@ dpdk_do_tx_copy(struct netdev *netdev, int qid, struct dp_packet_batch *batch)
>          memcpy(rte_pktmbuf_mtod(pkts[txcnt], void *),
>                 dp_packet_data(packet), size);
>          dp_packet_set_size((struct dp_packet *)pkts[txcnt], size);
> +        dp_packet_copy_mbuf_flags((struct dp_packet *)pkts[txcnt], packet);
>  
>          txcnt++;
>      }
> -- 
> 2.7.4
> 
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Lam, Tiago Oct. 5, 2018, 2:50 p.m. | #2
On 03/10/2018 19:26, Flavio Leitner wrote:
> On Fri, Sep 28, 2018 at 05:15:09PM +0100, Tiago Lam wrote:
>> From: Michael Qiu <qiudayu@chinac.com>
>>
>> When doing packet clone, if packet source is from DPDK driver,
>> multi-segment must be considered, and copy the segment's data one by
>> one.
>>
>> Also, lots of DPDK mbuf's info is missed during a copy, like packet
>> type, ol_flags, etc.  That information is very important for DPDK to do
>> packets processing.
>>
>> Co-authored-by: Mark Kavanagh <mark.b.kavanagh@intel.com>
>> Co-authored-by: Tiago Lam <tiago.lam@intel.com>
>>
>> Signed-off-by: Michael Qiu <qiudayu@chinac.com>
>> Signed-off-by: Mark Kavanagh <mark.b.kavanagh@intel.com>
>> Signed-off-by: Tiago Lam <tiago.lam@intel.com>
>> Acked-by: Eelco Chaudron <echaudro@redhat.com>
>> ---
>>  lib/dp-packet.c   | 69 ++++++++++++++++++++++++++++++++++++++++++++++---------
>>  lib/dp-packet.h   |  3 +++
>>  lib/netdev-dpdk.c |  1 +
>>  3 files changed, 62 insertions(+), 11 deletions(-)
>>
>> diff --git a/lib/dp-packet.c b/lib/dp-packet.c
>> index 167bf43..806640b 100644
>> --- a/lib/dp-packet.c
>> +++ b/lib/dp-packet.c
>> @@ -48,6 +48,22 @@ dp_packet_use__(struct dp_packet *b, void *base, size_t allocated,
>>      dp_packet_set_size(b, 0);
>>  }
>>  
>> +#ifdef DPDK_NETDEV
>> +void
>> +dp_packet_copy_mbuf_flags(struct dp_packet *dst, const struct dp_packet *src)
>> +{
>> +    ovs_assert(dst != NULL && src != NULL);
>> +    struct rte_mbuf *buf_dst = &(dst->mbuf);
>> +    struct rte_mbuf buf_src = src->mbuf;
>> +
>> +    buf_dst->ol_flags = buf_src.ol_flags;
>> +    buf_dst->packet_type = buf_src.packet_type;
>> +    buf_dst->tx_offload = buf_src.tx_offload;
> 
> Nit: use dereferencing on both sides.
> 
> Also, since this is a simple function that only copies few fields, maybe it could
> be in the dp-packet.h, so that netdev-dpdk.c can inline it.
> 
>> +}
>> +#else
>> +#define dp_packet_copy_mbuf_flags(arg1, arg2)
> 
> Nit2: since this is very specific to mbuf, we don't need to expose it
> outside of DPDK.
> 
>> +#endif
>> +
>>  /* Initializes 'b' as an empty dp_packet that contains the 'allocated' bytes of
>>   * memory starting at 'base'.  'base' should be the first byte of a region
>>   * obtained from malloc().  It will be freed (with free()) if 'b' is resized or
>> @@ -158,6 +174,44 @@ dp_packet_clone(const struct dp_packet *buffer)
>>      return dp_packet_clone_with_headroom(buffer, 0);
>>  }
>>  
>> +#ifdef DPDK_NETDEV
>> +struct dp_packet *
>> +dp_packet_clone_with_headroom(const struct dp_packet *b, size_t headroom) {
>> +    struct dp_packet *new_buffer;
>> +    uint32_t pkt_len = dp_packet_size(b);
>> +
>> +    /* copy multi-seg data */
>> +    if (b->source == DPBUF_DPDK && !rte_pktmbuf_is_contiguous(&b->mbuf)) {
>> +        void *dst = NULL;
>> +        struct rte_mbuf *mbuf = CONST_CAST(struct rte_mbuf *, &b->mbuf);
>> +
>> +        new_buffer = dp_packet_new_with_headroom(pkt_len, headroom);
>> +        dst = dp_packet_data(new_buffer);
>> +        dp_packet_set_size(new_buffer, pkt_len);
>> +
>> +        if (!rte_pktmbuf_read(mbuf, 0, pkt_len, dst)) {
> 
> Here we leak new_buffer which is xmalloc'ed.

Fixed. Thanks!

> 
>> +            return NULL;
>> +        }
>> +    } else {
>> +        new_buffer = dp_packet_clone_data_with_headroom(dp_packet_data(b),
>> +                                                        dp_packet_size(b),
>> +                                                        headroom);
>> +    }
>> +
>> +    /* Copy the following fields into the returned buffer: l2_pad_size,
>> +     * l2_5_ofs, l3_ofs, l4_ofs, cutlen, packet_type and md. */
>> +    memcpy(&new_buffer->l2_pad_size, &b->l2_pad_size,
>> +           sizeof(struct dp_packet) -
>> +           offsetof(struct dp_packet, l2_pad_size));
>> +
>> +    dp_packet_copy_mbuf_flags(new_buffer, b);
>> +    if (dp_packet_rss_valid(new_buffer)) {
>> +        new_buffer->mbuf.hash.rss = b->mbuf.hash.rss;
>> +    }
>> +
>> +    return new_buffer;
>> +}
>> +#else
>>  /* Creates and returns a new dp_packet whose data are copied from 'buffer'.
>>   * The returned dp_packet will additionally have 'headroom' bytes of
>>   * headroom. */
>> @@ -165,32 +219,25 @@ struct dp_packet *
>>  dp_packet_clone_with_headroom(const struct dp_packet *buffer, size_t headroom)
>>  {
>>      struct dp_packet *new_buffer;
>> +    uint32_t pkt_len = dp_packet_size(buffer);
>>  
>>      new_buffer = dp_packet_clone_data_with_headroom(dp_packet_data(buffer),
>> -                                                 dp_packet_size(buffer),
>> -                                                 headroom);
>> +                                                 pkt_len, headroom);
>> +
>>      /* Copy the following fields into the returned buffer: l2_pad_size,
>>       * l2_5_ofs, l3_ofs, l4_ofs, cutlen, packet_type and md. */
>>      memcpy(&new_buffer->l2_pad_size, &buffer->l2_pad_size,
>>              sizeof(struct dp_packet) -
>>              offsetof(struct dp_packet, l2_pad_size));
>>  
>> -#ifdef DPDK_NETDEV
>> -    new_buffer->mbuf.ol_flags = buffer->mbuf.ol_flags;
>> -#else
>>      new_buffer->rss_hash_valid = buffer->rss_hash_valid;
>> -#endif
>> -
>>      if (dp_packet_rss_valid(new_buffer)) {
>> -#ifdef DPDK_NETDEV
>> -        new_buffer->mbuf.hash.rss = buffer->mbuf.hash.rss;
>> -#else
>>          new_buffer->rss_hash = buffer->rss_hash;
>> -#endif
>>      }
>>  
>>      return new_buffer;
>>  }
>> +#endif
> 
> Nit3: Looks like the above functions share some code which could be on
> another function or maybe move the copy specifics to a separate
> functions.

The "nits" sound good to me. I'll include them in the next iteration.

Tiago.

> 
> 
>>  /* Creates and returns a new dp_packet that initially contains a copy of the
>>   * 'size' bytes of data starting at 'data' with no headroom or tailroom. */
>> diff --git a/lib/dp-packet.h b/lib/dp-packet.h
>> index aab8f62..cbf002c 100644
>> --- a/lib/dp-packet.h
>> +++ b/lib/dp-packet.h
>> @@ -124,6 +124,9 @@ void dp_packet_init_dpdk(struct dp_packet *);
>>  void dp_packet_init(struct dp_packet *, size_t);
>>  void dp_packet_uninit(struct dp_packet *);
>>  
>> +void dp_packet_copy_mbuf_flags(struct dp_packet *dst,
>> +                               const struct dp_packet *src);
>> +
>>  struct dp_packet *dp_packet_new(size_t);
>>  struct dp_packet *dp_packet_new_with_headroom(size_t, size_t headroom);
>>  struct dp_packet *dp_packet_clone(const struct dp_packet *);
>> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
>> index 3b11546..8484239 100644
>> --- a/lib/netdev-dpdk.c
>> +++ b/lib/netdev-dpdk.c
>> @@ -2364,6 +2364,7 @@ dpdk_do_tx_copy(struct netdev *netdev, int qid, struct dp_packet_batch *batch)
>>          memcpy(rte_pktmbuf_mtod(pkts[txcnt], void *),
>>                 dp_packet_data(packet), size);
>>          dp_packet_set_size((struct dp_packet *)pkts[txcnt], size);
>> +        dp_packet_copy_mbuf_flags((struct dp_packet *)pkts[txcnt], packet);
>>  
>>          txcnt++;
>>      }
>> -- 
>> 2.7.4
>>
>> _______________________________________________
>> dev mailing list
>> dev@openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>

Patch

diff --git a/lib/dp-packet.c b/lib/dp-packet.c
index 167bf43..806640b 100644
--- a/lib/dp-packet.c
+++ b/lib/dp-packet.c
@@ -48,6 +48,22 @@  dp_packet_use__(struct dp_packet *b, void *base, size_t allocated,
     dp_packet_set_size(b, 0);
 }
 
+#ifdef DPDK_NETDEV
+void
+dp_packet_copy_mbuf_flags(struct dp_packet *dst, const struct dp_packet *src)
+{
+    ovs_assert(dst != NULL && src != NULL);
+    struct rte_mbuf *buf_dst = &(dst->mbuf);
+    struct rte_mbuf buf_src = src->mbuf;
+
+    buf_dst->ol_flags = buf_src.ol_flags;
+    buf_dst->packet_type = buf_src.packet_type;
+    buf_dst->tx_offload = buf_src.tx_offload;
+}
+#else
+#define dp_packet_copy_mbuf_flags(arg1, arg2)
+#endif
+
 /* Initializes 'b' as an empty dp_packet that contains the 'allocated' bytes of
  * memory starting at 'base'.  'base' should be the first byte of a region
  * obtained from malloc().  It will be freed (with free()) if 'b' is resized or
@@ -158,6 +174,44 @@  dp_packet_clone(const struct dp_packet *buffer)
     return dp_packet_clone_with_headroom(buffer, 0);
 }
 
+#ifdef DPDK_NETDEV
+struct dp_packet *
+dp_packet_clone_with_headroom(const struct dp_packet *b, size_t headroom) {
+    struct dp_packet *new_buffer;
+    uint32_t pkt_len = dp_packet_size(b);
+
+    /* copy multi-seg data */
+    if (b->source == DPBUF_DPDK && !rte_pktmbuf_is_contiguous(&b->mbuf)) {
+        void *dst = NULL;
+        struct rte_mbuf *mbuf = CONST_CAST(struct rte_mbuf *, &b->mbuf);
+
+        new_buffer = dp_packet_new_with_headroom(pkt_len, headroom);
+        dst = dp_packet_data(new_buffer);
+        dp_packet_set_size(new_buffer, pkt_len);
+
+        if (!rte_pktmbuf_read(mbuf, 0, pkt_len, dst)) {
+            return NULL;
+        }
+    } else {
+        new_buffer = dp_packet_clone_data_with_headroom(dp_packet_data(b),
+                                                        dp_packet_size(b),
+                                                        headroom);
+    }
+
+    /* Copy the following fields into the returned buffer: l2_pad_size,
+     * l2_5_ofs, l3_ofs, l4_ofs, cutlen, packet_type and md. */
+    memcpy(&new_buffer->l2_pad_size, &b->l2_pad_size,
+           sizeof(struct dp_packet) -
+           offsetof(struct dp_packet, l2_pad_size));
+
+    dp_packet_copy_mbuf_flags(new_buffer, b);
+    if (dp_packet_rss_valid(new_buffer)) {
+        new_buffer->mbuf.hash.rss = b->mbuf.hash.rss;
+    }
+
+    return new_buffer;
+}
+#else
 /* Creates and returns a new dp_packet whose data are copied from 'buffer'.
  * The returned dp_packet will additionally have 'headroom' bytes of
  * headroom. */
@@ -165,32 +219,25 @@  struct dp_packet *
 dp_packet_clone_with_headroom(const struct dp_packet *buffer, size_t headroom)
 {
     struct dp_packet *new_buffer;
+    uint32_t pkt_len = dp_packet_size(buffer);
 
     new_buffer = dp_packet_clone_data_with_headroom(dp_packet_data(buffer),
-                                                 dp_packet_size(buffer),
-                                                 headroom);
+                                                 pkt_len, headroom);
+
     /* Copy the following fields into the returned buffer: l2_pad_size,
      * l2_5_ofs, l3_ofs, l4_ofs, cutlen, packet_type and md. */
     memcpy(&new_buffer->l2_pad_size, &buffer->l2_pad_size,
             sizeof(struct dp_packet) -
             offsetof(struct dp_packet, l2_pad_size));
 
-#ifdef DPDK_NETDEV
-    new_buffer->mbuf.ol_flags = buffer->mbuf.ol_flags;
-#else
     new_buffer->rss_hash_valid = buffer->rss_hash_valid;
-#endif
-
     if (dp_packet_rss_valid(new_buffer)) {
-#ifdef DPDK_NETDEV
-        new_buffer->mbuf.hash.rss = buffer->mbuf.hash.rss;
-#else
         new_buffer->rss_hash = buffer->rss_hash;
-#endif
     }
 
     return new_buffer;
 }
+#endif
 
 /* Creates and returns a new dp_packet that initially contains a copy of the
  * 'size' bytes of data starting at 'data' with no headroom or tailroom. */
diff --git a/lib/dp-packet.h b/lib/dp-packet.h
index aab8f62..cbf002c 100644
--- a/lib/dp-packet.h
+++ b/lib/dp-packet.h
@@ -124,6 +124,9 @@  void dp_packet_init_dpdk(struct dp_packet *);
 void dp_packet_init(struct dp_packet *, size_t);
 void dp_packet_uninit(struct dp_packet *);
 
+void dp_packet_copy_mbuf_flags(struct dp_packet *dst,
+                               const struct dp_packet *src);
+
 struct dp_packet *dp_packet_new(size_t);
 struct dp_packet *dp_packet_new_with_headroom(size_t, size_t headroom);
 struct dp_packet *dp_packet_clone(const struct dp_packet *);
diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
index 3b11546..8484239 100644
--- a/lib/netdev-dpdk.c
+++ b/lib/netdev-dpdk.c
@@ -2364,6 +2364,7 @@  dpdk_do_tx_copy(struct netdev *netdev, int qid, struct dp_packet_batch *batch)
         memcpy(rte_pktmbuf_mtod(pkts[txcnt], void *),
                dp_packet_data(packet), size);
         dp_packet_set_size((struct dp_packet *)pkts[txcnt], size);
+        dp_packet_copy_mbuf_flags((struct dp_packet *)pkts[txcnt], packet);
 
         txcnt++;
     }