diff mbox

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

Message ID HE1PR0501MB2314CE974124127DC0F54978D1890@HE1PR0501MB2314.eurprd05.prod.outlook.com (mailing list archive)
State Not Applicable, archived
Delegated to: Ferruh Yigit
Headers

Checks

Context Check Description
ci/checkpatch warning coding style issues
ci/Intel-compilation fail apply patch file failure

Commit Message

Ophir Munk April 23, 2018, 11:32 a.m. UTC 
  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.


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>

> >>>>>>>>

> >>>>>>>>

> >>>>>>>>

> >>>>>>>

> >>>>>

> >

Comments

Ferruh Yigit April 24, 2018, 5:57 p.m. UTC | #1 
On 4/23/2018 12:32 PM, Ophir Munk wrote:
> 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 see it now, missed nested if and assumed all block is for verifying offload,
thanks for pointing, I will send a new version soon, and will wait your tests.

> 
> 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
diff mbox

Patch

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;