[ovs-dev,5/5] lib/netdev-dpdk: copy large packet to multi-segment mbufs

From: Michael Qiu <qiudayu@chinac.com>

Currently, one packet is only copied to one segment
in function dpdk_do_tx_copy(),  this could be an issue
when a jumbo frame comes, especially for multiple segments.

This patch calculate the segment number needed by the packet and
copy the data to different segments.

Signed-off-by: Michael Qiu <qiudayu@chinac.com>
---
 lib/netdev-dpdk.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 48 insertions(+), 5 deletions(-)

>From: Michael Qiu [mailto:qdy220091330@gmail.com]
>Sent: Monday, June 19, 2017 6:29 AM
>To: dev@openvswitch.org
>Cc: Kavanagh, Mark B <mark.b.kavanagh@intel.com>; blp@ovn.org;
>dball@vmware.com; Michael Qiu <qiudayu@chinac.com>
>Subject: [PATCH 5/5] lib/netdev-dpdk: copy large packet to multi-segment mbufs
>
>From: Michael Qiu <qiudayu@chinac.com>
>
>Currently, one packet is only copied to one segment
>in function dpdk_do_tx_copy(),  this could be an issue
>when a jumbo frame comes, especially for multiple segments.
>
>This patch calculate the segment number needed by the packet and
>copy the data to different segments.
>
>Signed-off-by: Michael Qiu <qiudayu@chinac.com>

Hi Michael,

One comment inline.

Thanks,
Mark


>---
> lib/netdev-dpdk.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++++-----
> 1 file changed, 48 insertions(+), 5 deletions(-)
>
>diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
>index 0485872..38ec2ed 100644
>--- a/lib/netdev-dpdk.c
>+++ b/lib/netdev-dpdk.c
>@@ -1776,14 +1776,16 @@ dpdk_do_tx_copy(struct netdev *netdev, int qid, struct
>dp_packet_batch *batch)
> #endif
>     struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);
>     struct rte_mbuf *pkts[PKT_ARRAY_SIZE];
>+    struct rte_mbuf *temp, *head = NULL;
>     int dropped = 0;
>     int newcnt = 0;
>-    int i;
>+    int i, j, nb_segs;
>
>     dp_packet_batch_apply_cutlen(batch);
>
>     for (i = 0; i < batch->count; i++) {
>         int size = dp_packet_size(batch->packets[i]);
>+        int max_data_len, tmp_len;
>
>         if (OVS_UNLIKELY(size > dev->max_packet_len)) {
>             VLOG_WARN_RL(&rl, "Too big size %d max_packet_len %d",
>@@ -1793,7 +1795,24 @@ dpdk_do_tx_copy(struct netdev *netdev, int qid, struct
>dp_packet_batch *batch)
>             continue;
>         }
>
>-        pkts[newcnt] = rte_pktmbuf_alloc(dev->dpdk_mp->mp);
>+        temp = pkts[newcnt] = rte_pktmbuf_alloc(dev->dpdk_mp->mp);
>

Need to check if rte_pktmbuf_alloc() succeeded here.


+
>+        /* all new allocated mbuf's max data len is the same */
>+        max_data_len = temp->buf_len - temp->data_off;
>+
>+        nb_segs = size/max_data_len;
>+        if (size % max_data_len)
>+            nb_segs = nb_segs + 1;
>+
>+        for (j = 1; j < nb_segs; j++) {
>+            temp->next = rte_pktmbuf_alloc(dev->dpdk_mp->mp);
>+            if (!temp->next) {
>+                rte_pktmbuf_free(pkts[newcnt]);
>+                pkts[newcnt] = NULL;
>+                break;
>+            }
>+            temp = temp->next;
>+        }
>
>         if (!pkts[newcnt]) {
>             dropped += batch->count - i;
>@@ -1801,10 +1820,34 @@ dpdk_do_tx_copy(struct netdev *netdev, int qid, struct
>dp_packet_batch *batch)
>         }
>
>         /* We have to do a copy for now */
>-        memcpy(rte_pktmbuf_mtod(pkts[newcnt], void *),
>-               dp_packet_data(batch->packets[i]), size);
>+        rte_pktmbuf_pkt_len(pkts[newcnt]) = size;
>+        temp = pkts[newcnt];
>+        tmp_len = size < max_data_len ? size: max_data_len;
>+        if (batch->packets[i]->source == DPBUF_DPDK) {

Could you please add a comment explaining in which scenario this section is executed?

From what I can tell, dpdk_do_tx_copy is invoked only from either netdev_dpdk_send__ and/or netdev_dpdk_vhost_send (as a result of the OUTPUT action) if one of the following two conditions are met:
	1. the packet's 'source' != DPBUF_DPDK (N/A in this case)
	2. 'may_steal' == false

The only case I've observed in which the output action is invoked and where 'may_steal' is false is in the context of dpif_execute; however, in this case, I'm unsure if the packet's source is DPBUF_DPDK.

Thanks in advance.

>+            head = &(batch->packets[i]->mbuf);
>+            while (temp && head && size > 0) {
>+                rte_memcpy(rte_pktmbuf_mtod(temp, void*),
>dp_packet_data((struct dp_packet *)head),tmp_len);
>+                rte_pktmbuf_data_len(temp) = tmp_len;
>+                head = head->next;
>+                size = size - tmp_len;
>+                tmp_len =  size < max_data_len ? size: max_data_len;
>+                temp = temp->next;
>+            }
>+        } else {
>+            int offset = 0;
>+            while (temp && size > 0) {
>+                memcpy(rte_pktmbuf_mtod(temp, void *),
>+                    dp_packet_at(batch->packets[i], offset,tmp_len),
>tmp_len);
>+                rte_pktmbuf_data_len(temp) = tmp_len;
>+                temp = temp->next;
>+                size = size - tmp_len;
>+                offset +=tmp_len;
>+                tmp_len =  size < max_data_len ? size: max_data_len;
>+            }
>+        }
>+
>
>-        pkts[newcnt]->nb_segs = batch->packets[i]->mbuf.nb_segs;
>+        pkts[newcnt]->nb_segs = nb_segs;
>         pkts[newcnt]->ol_flags = batch->packets[i]->mbuf.ol_flags;
>         pkts[newcnt]->packet_type = batch->packets[i]->mbuf.packet_type;
>         pkts[newcnt]->tx_offload = batch->packets[i]->mbuf.tx_offload;
>--
>1.8.3.1