Re: [dpdk-dev] [PATCH] net/tap: remove queue specific offload support

[Ophir Munk <ophirmu@mellanox.com>]

Hi Ferruh,
The exact same setup works without your patch (done before sending my first email).
I started debugging your patch and noticed you have dropped the setting of txq->csum which always remains 0 therefore [1] is never executed.

I am adding a patch on top of yours which fixes this issue. 
FYI - I plan doing more tests to confirm that everything works as expected, then will update.

diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
index a0b8922..19c7ba0 100644
--- a/drivers/net/tap/rte_eth_tap.c
+++ b/drivers/net/tap/rte_eth_tap.c
@@ -1108,7 +1108,13 @@ enum ioctl_mode {
                return -1;
        dev->data->tx_queues[tx_queue_id] = &internals->txq[tx_queue_id];
        txq = dev->data->tx_queues[tx_queue_id];
-
+       if (tx_conf != NULL &&
+           !!(tx_conf->txq_flags & ETH_TXQ_FLAGS_IGNORE)) {
+               txq->csum = !!(tx_conf->offloads &
+                               (DEV_TX_OFFLOAD_IPV4_CKSUM |
+                                DEV_TX_OFFLOAD_UDP_CKSUM |
+                                DEV_TX_OFFLOAD_TCP_CKSUM));
+       }
        ret = tap_setup_queue(dev, internals, tx_queue_id, 0);
        if (ret == -1)
                return -1;

Regards,
Ophir

> -----Original Message-----
> From: Ferruh Yigit [mailto:ferruh.yigit@intel.com]
> Sent: Monday, April 23, 2018 1:14 PM
> To: Ophir Munk <ophirmu@mellanox.com>; Thomas Monjalon
> <thomas@monjalon.net>; 'Pascal Mazon' <pascal.mazon@6wind.com>;
> Mordechay Haimovsky <motih@mellanox.com>
> Cc: 'dev@dpdk.org' <dev@dpdk.org>; Shahaf Shuler
> <shahafs@mellanox.com>; Olga Shern <olgas@mellanox.com>; Raslan
> Darawsheh <rasland@mellanox.com>
> Subject: Re: [dpdk-dev] [PATCH] net/tap: remove queue specific offload
> support
> 
> On 4/23/2018 10:17 AM, Ophir Munk wrote:
> > Hi Ferruh,
> > A quick feedback to your patch on top of next-net/master: the IP and TCP
> offloaded checksums are turned incorrect.
> 
> Hi Ophir,
> 
> Thanks for testing.
> This patch removes queue specific offloads for tap but nothing touched on
> port offloads, and in below test you are already using single queue.
> 
> This may mean something is wrong in tap for port offloading configuration.
> 
> Tap does csum calculation in Tx path [1], which does not even checks the
> offloading flags, but mbuf->ol_flags. Any chance that mbuf->ol_flags is not
> set correct? Can you able to make exact same setup work without this
> patch?
> 
> 
> [1]
>     if (txq->csum &&
>         ((mbuf->ol_flags & (PKT_TX_IP_CKSUM | PKT_TX_IPV4) ||
>          (mbuf->ol_flags & PKT_TX_L4_MASK) == PKT_TX_UDP_CKSUM ||
>          (mbuf->ol_flags & PKT_TX_L4_MASK) == PKT_TX_TCP_CKSUM))) {
>             /* Support only packets with all data in the same seg */
>             if (mbuf->nb_segs > 1)
>                     break;
>             /* To change checksums, work on a copy of data. */
>             rte_memcpy(m_copy, rte_pktmbuf_mtod(mbuf, void *),
>                        rte_pktmbuf_data_len(mbuf));
>             tap_tx_offload(m_copy, mbuf->ol_flags,
>                            mbuf->l2_len, mbuf->l3_len);
>             iovecs[1].iov_base = m_copy;
>     }
> 
> 
> >
> > Detailed description
> > ================
> > A traffic generator is sending to a dpdk device one TCP packet and expects
> to receive it back after the IP and TCP checksums have been calculated by
> TAP.
> >
> > Testpmd parameters
> > =================
> > testpmd -c 0x0f -n 4 --vdev="net_tap0,iface=net_vsc0,remote=ens2" -w
> > 0000:00:00.0 -- --burst=64 --mbcache=512 --portmask 0x1 -i --txd=256
> > --rxd=256 --rxq=1 --txq=1 --coremask 0x008  --forward-mode=csum
> > --eth-peer=0,00:15:5d:10:66:02
> >
> > Testpmd CLI commands
> > ===================
> > testpmd> port stop all
> > testpmd> csum set ip hw 0
> > testpmd> csum set tcp hw 0
> > testpmd> port start all
> > testpmd> start
> >
> > On Traffic generator side
> > =====================
> > A traffic generator (scapy) is sending 1261 bytes of a TCP packet
> >
> > Monitoring the traffic:
> >
> > tcpdump -i <interface name> -envvv &
> >
> > The tcpdump output shows the sent packet followed by the received
> packet.
> > Please note the received packet has incorrect IP & TCP checksums (both
> > are 0)
> >
> > 11:51:03.058623 00:15:5d:10:66:02 > f4:52:14:7a:59:81, ethertype IPv4
> (0x0800), length 1261: (tos 0x0, ttl 64, id 1, offset 0, flags [none], proto TCP
> (6), length 1247)
> >     127.0.0.1.1 > 127.0.0.1.1: Flags [S], cksum 0xdba5 (correct), seq
> > 0:1207, win 8192, length 1207
> >
> > 11:51:03.058836 f4:52:14:7a:59:81 > 00:15:5d:10:66:02, ethertype IPv4
> (0x0800), length 1261: (tos 0x0, ttl 64, id 1, offset 0, flags [none], proto TCP
> (6), length 1247, bad cksum 0 (->7816)!)
> >     127.0.0.1.1 > 127.0.0.1.1: Flags [S], cksum 0x0000 (incorrect ->
> > 0xdba5), seq 0:1207, win 8192, length 1207
> >
> > Regards,
> > Ophir
> >
> >> -----Original Message-----
> >> From: Ophir Munk
> >> Sent: Monday, April 23, 2018 11:39 AM
> >> To: 'Ferruh Yigit' <ferruh.yigit@intel.com>; Thomas Monjalon
> >> <thomas@monjalon.net>; Pascal Mazon <pascal.mazon@6wind.com>;
> >> Mordechay Haimovsky <motih@mellanox.com>
> >> Cc: 'dev@dpdk.org' <dev@dpdk.org>; Shahaf Shuler
> >> <shahafs@mellanox.com>; Olga Shern <olgas@mellanox.com>; Raslan
> >> Darawsheh <rasland@mellanox.com>
> >> Subject: RE: [dpdk-dev] [PATCH] net/tap: remove queue specific
> >> offload support
> >>
> >> Hi Ferruh,
> >> I was able to apply your patch with Thomas help:
> >> 1. git am --reject
> >> 2. <Fix code manually using *.rej file> 3. git am --continue
> >>
> >> Regards,
> >> Ophir
> >>
> >>> -----Original Message-----
> >>> From: Ophir Munk
> >>> Sent: Sunday, April 22, 2018 7:05 PM
> >>> To: Ferruh Yigit <ferruh.yigit@intel.com>; Thomas Monjalon
> >>> <thomas@monjalon.net>; Pascal Mazon <pascal.mazon@6wind.com>;
> >>> Mordechay Haimovsky <motih@mellanox.com>
> >>> Cc: dev@dpdk.org; Shahaf Shuler <shahafs@mellanox.com>; Olga Shern
> >>> <olgas@mellanox.com>; Raslan Darawsheh <rasland@mellanox.com>
> >>> Subject: RE: [dpdk-dev] [PATCH] net/tap: remove queue specific
> >>> offload support
> >>>
> >>> Hi Ferruh,
> >>> I am not able to apply your patch on next-net/master branch.
> >>> I am failing to apply it both on latest commit or just before
> >>> 22-Mar-18 (commit's date).
> >>>
> >>> $ git am
> >>> dpdk-dev-net-tap-remove-queue-specific-offload-support.patch
> >>> Applying: net/tap: remove queue specific offload support
> >>> error: patch failed: drivers/net/tap/rte_eth_tap.c:269
> >>> error: drivers/net/tap/rte_eth_tap.c: patch does not apply Patch
> >>> failed at
> >>> 0001 net/tap: remove queue specific offload support
> >>>
> >>> Please advise.
> >>>
> >>> Once this error is fixed I can verify your patch with high priority
> >>> and send you my feedback.
> >>>
> >>> Regards,
> >>> Ophir
> >>>
> >>>> -----Original Message-----
> >>>> From: Ferruh Yigit [mailto:ferruh.yigit@intel.com]
> >>>> Sent: Wednesday, April 18, 2018 1:55 PM
> >>>> To: Ophir Munk <ophirmu@mellanox.com>; Thomas Monjalon
> >>>> <thomas@monjalon.net>; Pascal Mazon <pascal.mazon@6wind.com>;
> >>>> Mordechay Haimovsky <motih@mellanox.com>
> >>>> Cc: dev@dpdk.org; Shahaf Shuler <shahafs@mellanox.com>; Olga
> Shern
> >>>> <olgas@mellanox.com>
> >>>> Subject: Re: [dpdk-dev] [PATCH] net/tap: remove queue specific
> >>>> offload support
> >>>>
> >>>> On 4/18/2018 10:40 AM, Ophir Munk wrote:
> >>>>> Hi Ferruh,
> >>>>> Sorry for the delayed response.
> >>>>>
> >>>>> I would like to verify the correctness of this patch by running
> >>>>> several
> >>>> internal tests.
> >>>>> Is a reply by Monday OK with you?
> >>>>
> >>>> Monday can be late to include patch into rc1, any chance to do earlier?
> >>>>
> >>>>>
> >>>>> Regards,
> >>>>> Ophir
> >>>>>
> >>>>>> -----Original Message-----
> >>>>>> From: Ferruh Yigit [mailto:ferruh.yigit@intel.com]
> >>>>>> Sent: Wednesday, April 18, 2018 11:59 AM
> >>>>>> To: Thomas Monjalon <thomas@monjalon.net>; Pascal Mazon
> >>>>>> <pascal.mazon@6wind.com>; Mordechay Haimovsky
> >>>> <motih@mellanox.com>;
> >>>>>> Ophir Munk <ophirmu@mellanox.com>
> >>>>>> Cc: dev@dpdk.org; Shahaf Shuler <shahafs@mellanox.com>; Olga
> >>>>>> Shern <olgas@mellanox.com>
> >>>>>> Subject: Re: [dpdk-dev] [PATCH] net/tap: remove queue specific
> >>>>>> offload support
> >>>>>>
> >>>>>> On 4/12/2018 5:23 PM, Ferruh Yigit wrote:
> >>>>>>> On 4/5/2018 6:49 PM, Thomas Monjalon wrote:
> >>>>>>>> Pascal, Moti, Ophir,
> >>>>>>>> please comment.
> >>>>>>>
> >>>>>>> Hi Moti,
> >>>>>>>
> >>>>>>> Any comment? This has been asked many times now.
> >>>>>>
> >>>>>> Hi Moti, Ophir,
> >>>>>>
> >>>>>> You have not responded why queue specific offload added in other
> >>>> thread.
> >>>>>> And you are not responding to this patch...
> >>>>>>
> >>>>>> Hi Pascal,
> >>>>>>
> >>>>>> If you also have no objection, this patch is going in.
> >>>>>>
> >>>>>> Thanks,
> >>>>>> ferruh
> >>>>>>
> >>>>>>
> >>>>>>>
> >>>>>>>>
> >>>>>>>> 22/03/2018 19:28, Ferruh Yigit:
> >>>>>>>>> It is not clear if tap PMD supports queue specific offloads,
> >>>>>>>>> removing the related code.
> >>>>>>>>>
> >>>>>>>>> Fixes: 95ae196ae10b ("net/tap: use new Rx offloads API")
> >>>>>>>>> Fixes: 818fe14a9891 ("net/tap: use new Tx offloads API")
> >>>>>>>>> Cc: motih@mellanox.com
> >>>>>>>>>
> >>>>>>>>> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>
> >>>>>
> >