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

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

[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>
---
Cc: Shahaf Shuler <shahafs@mellanox.com>
---
 drivers/net/tap/rte_eth_tap.c | 101 +++---------------------------------------
 1 file changed, 6 insertions(+), 95 deletions(-)

diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
index c9b86a8f0..67ed9d466 100644
--- a/drivers/net/tap/rte_eth_tap.c
+++ b/drivers/net/tap/rte_eth_tap.c
@@ -269,33 +269,6 @@ tap_rx_offload_get_port_capa(void)
 	       DEV_RX_OFFLOAD_CRC_STRIP;
 }
 
-static uint64_t
-tap_rx_offload_get_queue_capa(void)
-{
-	return DEV_RX_OFFLOAD_SCATTER |
-	       DEV_RX_OFFLOAD_IPV4_CKSUM |
-	       DEV_RX_OFFLOAD_UDP_CKSUM |
-	       DEV_RX_OFFLOAD_TCP_CKSUM |
-	       DEV_RX_OFFLOAD_CRC_STRIP;
-}
-
-static bool
-tap_rxq_are_offloads_valid(struct rte_eth_dev *dev, uint64_t offloads)
-{
-	uint64_t port_offloads = dev->data->dev_conf.rxmode.offloads;
-	uint64_t queue_supp_offloads = tap_rx_offload_get_queue_capa();
-	uint64_t port_supp_offloads = tap_rx_offload_get_port_capa();
-
-	if ((offloads & (queue_supp_offloads | port_supp_offloads)) !=
-	    offloads)
-		return false;
-	if ((offloads & port_offloads) ^ offloads)
-		return false;
-	if ((offloads & port_supp_offloads) ^ offloads)
-		return false;
-	return true;
-}
-
 /* Callback to handle the rx burst of packets to the correct interface and
  * file descriptor(s) in a multi-queue setup.
  */
@@ -404,31 +377,6 @@ tap_tx_offload_get_port_capa(void)
 	       DEV_TX_OFFLOAD_TCP_CKSUM;
 }
 
-static uint64_t
-tap_tx_offload_get_queue_capa(void)
-{
-	return DEV_TX_OFFLOAD_MULTI_SEGS |
-	       DEV_TX_OFFLOAD_IPV4_CKSUM |
-	       DEV_TX_OFFLOAD_UDP_CKSUM |
-	       DEV_TX_OFFLOAD_TCP_CKSUM;
-}
-
-static bool
-tap_txq_are_offloads_valid(struct rte_eth_dev *dev, uint64_t offloads)
-{
-	uint64_t port_offloads = dev->data->dev_conf.txmode.offloads;
-	uint64_t queue_supp_offloads = tap_tx_offload_get_queue_capa();
-	uint64_t port_supp_offloads = tap_tx_offload_get_port_capa();
-
-	if ((offloads & (queue_supp_offloads | port_supp_offloads)) !=
-	    offloads)
-		return false;
-	/* Verify we have no conflict with port offloads */
-	if ((port_offloads ^ offloads) & port_supp_offloads)
-		return false;
-	return true;
-}
-
 static void
 tap_tx_offload(char *packet, uint64_t ol_flags, unsigned int l2_len,
 	       unsigned int l3_len)
@@ -742,12 +690,10 @@ tap_dev_info(struct rte_eth_dev *dev, struct rte_eth_dev_info *dev_info)
 	dev_info->min_rx_bufsize = 0;
 	dev_info->pci_dev = NULL;
 	dev_info->speed_capa = tap_dev_speed_capa();
-	dev_info->rx_queue_offload_capa = tap_rx_offload_get_queue_capa();
-	dev_info->rx_offload_capa = tap_rx_offload_get_port_capa() |
-				    dev_info->rx_queue_offload_capa;
-	dev_info->tx_queue_offload_capa = tap_tx_offload_get_queue_capa();
-	dev_info->tx_offload_capa = tap_tx_offload_get_port_capa() |
-				    dev_info->tx_queue_offload_capa;
+	dev_info->rx_offload_capa = tap_rx_offload_get_port_capa();
+	dev_info->tx_offload_capa = tap_tx_offload_get_port_capa();
+	dev_info->rx_queue_offload_capa = 0;
+	dev_info->tx_queue_offload_capa = 0;
 }
 
 static int
@@ -1063,19 +1009,6 @@ tap_rx_queue_setup(struct rte_eth_dev *dev,
 		return -1;
 	}
 
-	/* Verify application offloads are valid for our port and queue. */
-	if (!tap_rxq_are_offloads_valid(dev, rx_conf->offloads)) {
-		rte_errno = ENOTSUP;
-		RTE_LOG(ERR, PMD,
-			"%p: Rx queue offloads 0x%" PRIx64
-			" don't match port offloads 0x%" PRIx64
-			" or supported offloads 0x%" PRIx64 "\n",
-			(void *)dev, rx_conf->offloads,
-			dev->data->dev_conf.rxmode.offloads,
-			(tap_rx_offload_get_port_capa() |
-			 tap_rx_offload_get_queue_capa()));
-		return -rte_errno;
-	}
 	rxq->mp = mp;
 	rxq->trigger_seen = 1; /* force initial burst */
 	rxq->in_port = dev->data->port_id;
@@ -1134,7 +1067,7 @@ tap_tx_queue_setup(struct rte_eth_dev *dev,
 		   uint16_t tx_queue_id,
 		   uint16_t nb_tx_desc __rte_unused,
 		   unsigned int socket_id __rte_unused,
-		   const struct rte_eth_txconf *tx_conf)
+		   const struct rte_eth_txconf *tx_conf __rte_unused)
 {
 	struct pmd_internals *internals = dev->data->dev_private;
 	struct tx_queue *txq;
@@ -1144,29 +1077,7 @@ tap_tx_queue_setup(struct rte_eth_dev *dev,
 		return -1;
 	dev->data->tx_queues[tx_queue_id] = &internals->txq[tx_queue_id];
 	txq = dev->data->tx_queues[tx_queue_id];
-	/*
-	 * Don't verify port offloads for application which
-	 * use the old API.
-	 */
-	if (tx_conf != NULL &&
-	    !!(tx_conf->txq_flags & ETH_TXQ_FLAGS_IGNORE)) {
-		if (tap_txq_are_offloads_valid(dev, tx_conf->offloads)) {
-			txq->csum = !!(tx_conf->offloads &
-					(DEV_TX_OFFLOAD_IPV4_CKSUM |
-					 DEV_TX_OFFLOAD_UDP_CKSUM |
-					 DEV_TX_OFFLOAD_TCP_CKSUM));
-		} else {
-			rte_errno = ENOTSUP;
-			RTE_LOG(ERR, PMD,
-				"%p: Tx queue offloads 0x%" PRIx64
-				" don't match port offloads 0x%" PRIx64
-				" or supported offloads 0x%" PRIx64,
-				(void *)dev, tx_conf->offloads,
-				dev->data->dev_conf.txmode.offloads,
-				tap_tx_offload_get_port_capa());
-			return -rte_errno;
-		}
-	}
+
 	ret = tap_setup_queue(dev, internals, tx_queue_id, 0);
 	if (ret == -1)
 		return -1;
-- 
2.13.6

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

[Thomas Monjalon]

Pascal, Moti, Ophir,
please comment.

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>

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

[Ferruh Yigit]

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.

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

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

[Ferruh Yigit]

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

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

[Ophir Munk]

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?

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

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

[Ferruh Yigit]

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

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

[Ophir Munk]

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

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

[Ophir Munk]

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

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

[Ophir Munk]

Hi Ferruh,
A quick feedback to your patch on top of next-net/master: the IP and TCP offloaded checksums are turned incorrect.

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

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

[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>
---
v2:
* rebased
---
 drivers/net/tap/rte_eth_tap.c | 99 +++----------------------------------------
 1 file changed, 6 insertions(+), 93 deletions(-)

diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
index 66e026f6d..a0b8922e5 100644
--- a/drivers/net/tap/rte_eth_tap.c
+++ b/drivers/net/tap/rte_eth_tap.c
@@ -278,31 +278,6 @@ tap_rx_offload_get_port_capa(void)
 	       DEV_RX_OFFLOAD_CRC_STRIP;
 }
 
-static uint64_t
-tap_rx_offload_get_queue_capa(void)
-{
-	return DEV_RX_OFFLOAD_SCATTER |
-	       DEV_RX_OFFLOAD_IPV4_CKSUM |
-	       DEV_RX_OFFLOAD_UDP_CKSUM |
-	       DEV_RX_OFFLOAD_TCP_CKSUM |
-	       DEV_RX_OFFLOAD_CRC_STRIP;
-}
-
-static bool
-tap_rxq_are_offloads_valid(struct rte_eth_dev *dev, uint64_t offloads)
-{
-	uint64_t port_offloads = dev->data->dev_conf.rxmode.offloads;
-	uint64_t queue_supp_offloads = tap_rx_offload_get_queue_capa();
-	uint64_t port_supp_offloads = tap_rx_offload_get_port_capa();
-
-	if ((offloads & (queue_supp_offloads | port_supp_offloads)) !=
-	    offloads)
-		return false;
-	if ((port_offloads ^ offloads) & port_supp_offloads)
-		return false;
-	return true;
-}
-
 /* Callback to handle the rx burst of packets to the correct interface and
  * file descriptor(s) in a multi-queue setup.
  */
@@ -411,31 +386,6 @@ tap_tx_offload_get_port_capa(void)
 	       DEV_TX_OFFLOAD_TCP_CKSUM;
 }
 
-static uint64_t
-tap_tx_offload_get_queue_capa(void)
-{
-	return DEV_TX_OFFLOAD_MULTI_SEGS |
-	       DEV_TX_OFFLOAD_IPV4_CKSUM |
-	       DEV_TX_OFFLOAD_UDP_CKSUM |
-	       DEV_TX_OFFLOAD_TCP_CKSUM;
-}
-
-static bool
-tap_txq_are_offloads_valid(struct rte_eth_dev *dev, uint64_t offloads)
-{
-	uint64_t port_offloads = dev->data->dev_conf.txmode.offloads;
-	uint64_t queue_supp_offloads = tap_tx_offload_get_queue_capa();
-	uint64_t port_supp_offloads = tap_tx_offload_get_port_capa();
-
-	if ((offloads & (queue_supp_offloads | port_supp_offloads)) !=
-	    offloads)
-		return false;
-	/* Verify we have no conflict with port offloads */
-	if ((port_offloads ^ offloads) & port_supp_offloads)
-		return false;
-	return true;
-}
-
 static void
 tap_tx_offload(char *packet, uint64_t ol_flags, unsigned int l2_len,
 	       unsigned int l3_len)
@@ -761,12 +711,10 @@ tap_dev_info(struct rte_eth_dev *dev, struct rte_eth_dev_info *dev_info)
 	dev_info->max_tx_queues = RTE_PMD_TAP_MAX_QUEUES;
 	dev_info->min_rx_bufsize = 0;
 	dev_info->speed_capa = tap_dev_speed_capa();
-	dev_info->rx_queue_offload_capa = tap_rx_offload_get_queue_capa();
-	dev_info->rx_offload_capa = tap_rx_offload_get_port_capa() |
-				    dev_info->rx_queue_offload_capa;
-	dev_info->tx_queue_offload_capa = tap_tx_offload_get_queue_capa();
-	dev_info->tx_offload_capa = tap_tx_offload_get_port_capa() |
-				    dev_info->tx_queue_offload_capa;
+	dev_info->rx_offload_capa = tap_rx_offload_get_port_capa();
+	dev_info->tx_offload_capa = tap_tx_offload_get_port_capa();
+	dev_info->rx_queue_offload_capa = 0;
+	dev_info->tx_queue_offload_capa = 0;
 }
 
 static int
@@ -1092,19 +1040,6 @@ tap_rx_queue_setup(struct rte_eth_dev *dev,
 		return -1;
 	}
 
-	/* Verify application offloads are valid for our port and queue. */
-	if (!tap_rxq_are_offloads_valid(dev, rx_conf->offloads)) {
-		rte_errno = ENOTSUP;
-		RTE_LOG(ERR, PMD,
-			"%p: Rx queue offloads 0x%" PRIx64
-			" don't match port offloads 0x%" PRIx64
-			" or supported offloads 0x%" PRIx64 "\n",
-			(void *)dev, rx_conf->offloads,
-			dev->data->dev_conf.rxmode.offloads,
-			(tap_rx_offload_get_port_capa() |
-			 tap_rx_offload_get_queue_capa()));
-		return -rte_errno;
-	}
 	rxq->mp = mp;
 	rxq->trigger_seen = 1; /* force initial burst */
 	rxq->in_port = dev->data->port_id;
@@ -1163,7 +1098,7 @@ tap_tx_queue_setup(struct rte_eth_dev *dev,
 		   uint16_t tx_queue_id,
 		   uint16_t nb_tx_desc __rte_unused,
 		   unsigned int socket_id __rte_unused,
-		   const struct rte_eth_txconf *tx_conf)
+		   const struct rte_eth_txconf *tx_conf __rte_unused)
 {
 	struct pmd_internals *internals = dev->data->dev_private;
 	struct tx_queue *txq;
@@ -1173,29 +1108,7 @@ tap_tx_queue_setup(struct rte_eth_dev *dev,
 		return -1;
 	dev->data->tx_queues[tx_queue_id] = &internals->txq[tx_queue_id];
 	txq = dev->data->tx_queues[tx_queue_id];
-	/*
-	 * Don't verify port offloads for application which
-	 * use the old API.
-	 */
-	if (tx_conf != NULL &&
-	    !!(tx_conf->txq_flags & ETH_TXQ_FLAGS_IGNORE)) {
-		if (tap_txq_are_offloads_valid(dev, tx_conf->offloads)) {
-			txq->csum = !!(tx_conf->offloads &
-					(DEV_TX_OFFLOAD_IPV4_CKSUM |
-					 DEV_TX_OFFLOAD_UDP_CKSUM |
-					 DEV_TX_OFFLOAD_TCP_CKSUM));
-		} else {
-			rte_errno = ENOTSUP;
-			RTE_LOG(ERR, PMD,
-				"%p: Tx queue offloads 0x%" PRIx64
-				" don't match port offloads 0x%" PRIx64
-				" or supported offloads 0x%" PRIx64,
-				(void *)dev, tx_conf->offloads,
-				dev->data->dev_conf.txmode.offloads,
-				tap_tx_offload_get_port_capa());
-			return -rte_errno;
-		}
-	}
+
 	ret = tap_setup_queue(dev, internals, tx_queue_id, 0);
 	if (ret == -1)
 		return -1;
-- 
2.14.3

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

[Ferruh Yigit]

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

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

[Ophir Munk]

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

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

[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>
---
Cc: Ophir Munk <ophirmu@mellanox.com>

v2:
* rebased

v3:
* txq->csum restored,
  - ETH_TXQ_FLAGS_IGNORE check removed since ethdev layer takes care of it
  - tx_conf != NULL check removed, this is internal api who calls this is
  ethdev and it doesn't pass null tx_conf
---
 drivers/net/tap/rte_eth_tap.c | 102 +++++-------------------------------------
 1 file changed, 10 insertions(+), 92 deletions(-)

diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
index ef33aace9..61b4b5df3 100644
--- a/drivers/net/tap/rte_eth_tap.c
+++ b/drivers/net/tap/rte_eth_tap.c
@@ -278,31 +278,6 @@ tap_rx_offload_get_port_capa(void)
 	       DEV_RX_OFFLOAD_CRC_STRIP;
 }
 
-static uint64_t
-tap_rx_offload_get_queue_capa(void)
-{
-	return DEV_RX_OFFLOAD_SCATTER |
-	       DEV_RX_OFFLOAD_IPV4_CKSUM |
-	       DEV_RX_OFFLOAD_UDP_CKSUM |
-	       DEV_RX_OFFLOAD_TCP_CKSUM |
-	       DEV_RX_OFFLOAD_CRC_STRIP;
-}
-
-static bool
-tap_rxq_are_offloads_valid(struct rte_eth_dev *dev, uint64_t offloads)
-{
-	uint64_t port_offloads = dev->data->dev_conf.rxmode.offloads;
-	uint64_t queue_supp_offloads = tap_rx_offload_get_queue_capa();
-	uint64_t port_supp_offloads = tap_rx_offload_get_port_capa();
-
-	if ((offloads & (queue_supp_offloads | port_supp_offloads)) !=
-	    offloads)
-		return false;
-	if ((port_offloads ^ offloads) & port_supp_offloads)
-		return false;
-	return true;
-}
-
 /* Callback to handle the rx burst of packets to the correct interface and
  * file descriptor(s) in a multi-queue setup.
  */
@@ -411,31 +386,6 @@ tap_tx_offload_get_port_capa(void)
 	       DEV_TX_OFFLOAD_TCP_CKSUM;
 }
 
-static uint64_t
-tap_tx_offload_get_queue_capa(void)
-{
-	return DEV_TX_OFFLOAD_MULTI_SEGS |
-	       DEV_TX_OFFLOAD_IPV4_CKSUM |
-	       DEV_TX_OFFLOAD_UDP_CKSUM |
-	       DEV_TX_OFFLOAD_TCP_CKSUM;
-}
-
-static bool
-tap_txq_are_offloads_valid(struct rte_eth_dev *dev, uint64_t offloads)
-{
-	uint64_t port_offloads = dev->data->dev_conf.txmode.offloads;
-	uint64_t queue_supp_offloads = tap_tx_offload_get_queue_capa();
-	uint64_t port_supp_offloads = tap_tx_offload_get_port_capa();
-
-	if ((offloads & (queue_supp_offloads | port_supp_offloads)) !=
-	    offloads)
-		return false;
-	/* Verify we have no conflict with port offloads */
-	if ((port_offloads ^ offloads) & port_supp_offloads)
-		return false;
-	return true;
-}
-
 static void
 tap_tx_offload(char *packet, uint64_t ol_flags, unsigned int l2_len,
 	       unsigned int l3_len)
@@ -763,12 +713,10 @@ tap_dev_info(struct rte_eth_dev *dev, struct rte_eth_dev_info *dev_info)
 	dev_info->max_tx_queues = RTE_PMD_TAP_MAX_QUEUES;
 	dev_info->min_rx_bufsize = 0;
 	dev_info->speed_capa = tap_dev_speed_capa();
-	dev_info->rx_queue_offload_capa = tap_rx_offload_get_queue_capa();
-	dev_info->rx_offload_capa = tap_rx_offload_get_port_capa() |
-				    dev_info->rx_queue_offload_capa;
-	dev_info->tx_queue_offload_capa = tap_tx_offload_get_queue_capa();
-	dev_info->tx_offload_capa = tap_tx_offload_get_port_capa() |
-				    dev_info->tx_queue_offload_capa;
+	dev_info->rx_offload_capa = tap_rx_offload_get_port_capa();
+	dev_info->tx_offload_capa = tap_tx_offload_get_port_capa();
+	dev_info->rx_queue_offload_capa = 0;
+	dev_info->tx_queue_offload_capa = 0;
 }
 
 static int
@@ -1094,19 +1042,6 @@ tap_rx_queue_setup(struct rte_eth_dev *dev,
 		return -1;
 	}
 
-	/* Verify application offloads are valid for our port and queue. */
-	if (!tap_rxq_are_offloads_valid(dev, rx_conf->offloads)) {
-		rte_errno = ENOTSUP;
-		RTE_LOG(ERR, PMD,
-			"%p: Rx queue offloads 0x%" PRIx64
-			" don't match port offloads 0x%" PRIx64
-			" or supported offloads 0x%" PRIx64 "\n",
-			(void *)dev, rx_conf->offloads,
-			dev->data->dev_conf.rxmode.offloads,
-			(tap_rx_offload_get_port_capa() |
-			 tap_rx_offload_get_queue_capa()));
-		return -rte_errno;
-	}
 	rxq->mp = mp;
 	rxq->trigger_seen = 1; /* force initial burst */
 	rxq->in_port = dev->data->port_id;
@@ -1175,29 +1110,12 @@ tap_tx_queue_setup(struct rte_eth_dev *dev,
 		return -1;
 	dev->data->tx_queues[tx_queue_id] = &internals->txq[tx_queue_id];
 	txq = dev->data->tx_queues[tx_queue_id];
-	/*
-	 * Don't verify port offloads for application which
-	 * use the old API.
-	 */
-	if (tx_conf != NULL &&
-	    !!(tx_conf->txq_flags & ETH_TXQ_FLAGS_IGNORE)) {
-		if (tap_txq_are_offloads_valid(dev, tx_conf->offloads)) {
-			txq->csum = !!(tx_conf->offloads &
-					(DEV_TX_OFFLOAD_IPV4_CKSUM |
-					 DEV_TX_OFFLOAD_UDP_CKSUM |
-					 DEV_TX_OFFLOAD_TCP_CKSUM));
-		} else {
-			rte_errno = ENOTSUP;
-			RTE_LOG(ERR, PMD,
-				"%p: Tx queue offloads 0x%" PRIx64
-				" don't match port offloads 0x%" PRIx64
-				" or supported offloads 0x%" PRIx64,
-				(void *)dev, tx_conf->offloads,
-				dev->data->dev_conf.txmode.offloads,
-				tap_tx_offload_get_port_capa());
-			return -rte_errno;
-		}
-	}
+
+	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;
-- 
2.14.3

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

[Ferruh Yigit]

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

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

[Ophir Munk]

Hi Ferruh,

I should have mentioned earlier that TAP does support queue specific capabilities. 
Please look in tap_queue_setup() and note that each TAP queue is created with a distinct file descriptor (fd).
Then supporting an offload capability is just implementing it in SW (e.g. calculating IP checksum).

If the main assumption of this patch was that TAP does not support queue specific offloads - then please consider this patch again.

On the other hand there is no port specific capability supported by TAP. However, in order to support legacy applications, port capabilities are usually reported as the OR operation between queue & port capabilities. 
TAP currently clones the queue capabilities to port capabilities. We could optimize this cloning by always return queue capabilities when queried about queues or ports. In this case - tap_rx_offload_get_port_capa() and tap_tx_offload_get_port_capa() could be removed.

Please find more comments inline.

> -----Original Message-----
> From: Ferruh Yigit [mailto:ferruh.yigit@intel.com]
> Sent: Tuesday, April 24, 2018 8:54 PM
> To: Pascal Mazon <pascal.mazon@6wind.com>
> Cc: dev@dpdk.org; Ferruh Yigit <ferruh.yigit@intel.com>; Mordechay
> Haimovsky <motih@mellanox.com>; Ophir Munk <ophirmu@mellanox.com>
> Subject: [PATCH v3] net/tap: remove queue specific offload support
> 
> 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>
> ---
> Cc: Ophir Munk <ophirmu@mellanox.com>
> 
> v2:
> * rebased
> 
> v3:
> * txq->csum restored,
>   - ETH_TXQ_FLAGS_IGNORE check removed since ethdev layer takes care of
> it
>   - tx_conf != NULL check removed, this is internal api who calls this is
>   ethdev and it doesn't pass null tx_conf
> ---
>  drivers/net/tap/rte_eth_tap.c | 102 +++++-------------------------------------
>  1 file changed, 10 insertions(+), 92 deletions(-)
> 
> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
> index ef33aace9..61b4b5df3 100644
> --- a/drivers/net/tap/rte_eth_tap.c
> +++ b/drivers/net/tap/rte_eth_tap.c
> @@ -278,31 +278,6 @@ tap_rx_offload_get_port_capa(void)
>  	       DEV_RX_OFFLOAD_CRC_STRIP;
>  }
> 
> -static uint64_t
> -tap_rx_offload_get_queue_capa(void)
> -{
> -	return DEV_RX_OFFLOAD_SCATTER |
> -	       DEV_RX_OFFLOAD_IPV4_CKSUM |
> -	       DEV_RX_OFFLOAD_UDP_CKSUM |
> -	       DEV_RX_OFFLOAD_TCP_CKSUM |
> -	       DEV_RX_OFFLOAD_CRC_STRIP;
> -}
> -

TAP PMD supports all of these RX queue specific offloads. I suggest to leave this function in place.

> -static bool
> -tap_rxq_are_offloads_valid(struct rte_eth_dev *dev, uint64_t offloads) -{
> -	uint64_t port_offloads = dev->data->dev_conf.rxmode.offloads;
> -	uint64_t queue_supp_offloads = tap_rx_offload_get_queue_capa();
> -	uint64_t port_supp_offloads = tap_rx_offload_get_port_capa();
> -
> -	if ((offloads & (queue_supp_offloads | port_supp_offloads)) !=
> -	    offloads)
> -		return false;
> -	if ((port_offloads ^ offloads) & port_supp_offloads)
> -		return false;
> -	return true;
> -}
> -

Putting aside the fact that queue offloads equals port offloads (so could ignore "port_supp_offload" variable) - this function is essential to validate that the configured Rx offloads are supported by TAP. I suggest to leave this function in place.
Without it - testpmd falsely confirms non supported offloads.
For example before this patch: offloading "hw-vlan-filter" will fail as expected:

testpmd> port config all
testpmd> port config all hw-vlan-filter on
testpmd> port start all
Configuring Port 0 (socket 0)
PMD: net_tap0: 0x1209fc0: TX configured queues number: 1
PMD: net_tap0: 0x1209fc0: RX configured queues number: 1
PMD: 0x1209fc0: Rx queue offloads 0x120e don't match port offloads 0x120e or supported offloads 0x300e
Fail to configure port 0 rx queues

However, with this patch this configuration is falsely accepted.

>  /* Callback to handle the rx burst of packets to the correct interface and
>   * file descriptor(s) in a multi-queue setup.
>   */
> @@ -411,31 +386,6 @@ tap_tx_offload_get_port_capa(void)
>  	       DEV_TX_OFFLOAD_TCP_CKSUM;
>  }
> 
> -static uint64_t
> -tap_tx_offload_get_queue_capa(void)
> -{
> -	return DEV_TX_OFFLOAD_MULTI_SEGS |
> -	       DEV_TX_OFFLOAD_IPV4_CKSUM |
> -	       DEV_TX_OFFLOAD_UDP_CKSUM |
> -	       DEV_TX_OFFLOAD_TCP_CKSUM;
> -}
> -

TAP PMD supports all of these TX queue specific offloads. I suggest to leave this function in place.

> -static bool
> -tap_txq_are_offloads_valid(struct rte_eth_dev *dev, uint64_t offloads) -{
> -	uint64_t port_offloads = dev->data->dev_conf.txmode.offloads;
> -	uint64_t queue_supp_offloads = tap_tx_offload_get_queue_capa();
> -	uint64_t port_supp_offloads = tap_tx_offload_get_port_capa();
> -
> -	if ((offloads & (queue_supp_offloads | port_supp_offloads)) !=
> -	    offloads)
> -		return false;
> -	/* Verify we have no conflict with port offloads */
> -	if ((port_offloads ^ offloads) & port_supp_offloads)
> -		return false;
> -	return true;
> -}
> -

This function is essential to validate that the configured Tx offloads are supported by TAP. 
I suggest to leave this function in place.

>  static void
>  tap_tx_offload(char *packet, uint64_t ol_flags, unsigned int l2_len,
>  	       unsigned int l3_len)
> @@ -763,12 +713,10 @@ tap_dev_info(struct rte_eth_dev *dev, struct
> rte_eth_dev_info *dev_info)
>  	dev_info->max_tx_queues = RTE_PMD_TAP_MAX_QUEUES;
>  	dev_info->min_rx_bufsize = 0;
>  	dev_info->speed_capa = tap_dev_speed_capa();
> -	dev_info->rx_queue_offload_capa =
> tap_rx_offload_get_queue_capa();
> -	dev_info->rx_offload_capa = tap_rx_offload_get_port_capa() |
> -				    dev_info->rx_queue_offload_capa;
> -	dev_info->tx_queue_offload_capa =
> tap_tx_offload_get_queue_capa();
> -	dev_info->tx_offload_capa = tap_tx_offload_get_port_capa() |
> -				    dev_info->tx_queue_offload_capa;
> +	dev_info->rx_offload_capa = tap_rx_offload_get_port_capa();
> +	dev_info->tx_offload_capa = tap_tx_offload_get_port_capa();
> +	dev_info->rx_queue_offload_capa = 0;
> +	dev_info->tx_queue_offload_capa = 0;
>  }
> 

Rx_queue_offloads_capa should be reported as before:
dev_info->tx_queue_offload_capa = tap_tx_offload_get_queue_capa();
Same for TX offloads.

Port capabilities could return queue capabilities:

Instead of:

dev_info->rx_offload_capa = tap_rx_offload_get_port_capa() |
				    dev_info->rx_queue_offload_capa;

We could return:

dev_info->rx_offload_capa = dev_info->rx_queue_offload_capa;

The same argument is valid for Tx as well.

>  static int
> @@ -1094,19 +1042,6 @@ tap_rx_queue_setup(struct rte_eth_dev *dev,
>  		return -1;
>  	}
> 
> -	/* Verify application offloads are valid for our port and queue. */
> -	if (!tap_rxq_are_offloads_valid(dev, rx_conf->offloads)) {
> -		rte_errno = ENOTSUP;
> -		RTE_LOG(ERR, PMD,
> -			"%p: Rx queue offloads 0x%" PRIx64
> -			" don't match port offloads 0x%" PRIx64
> -			" or supported offloads 0x%" PRIx64 "\n",
> -			(void *)dev, rx_conf->offloads,
> -			dev->data->dev_conf.rxmode.offloads,
> -			(tap_rx_offload_get_port_capa() |
> -			 tap_rx_offload_get_queue_capa()));
> -		return -rte_errno;
> -	}

The tap_rxq_are_offloads_valid() call is essential. I suggest to leave it in place.
The RTE_LOG could drop port references to become:

		RTE_LOG(ERR, PMD,
			"%p: Rx queue offloads 0x%" PRIx64
			" don't match"
			" supported offloads 0x%" PRIx64 "\n",
			(void *)dev, rx_conf->offloads,
			 tap_rx_offload_get_queue_capa()));


>  	rxq->mp = mp;
>  	rxq->trigger_seen = 1; /* force initial burst */
>  	rxq->in_port = dev->data->port_id;
> @@ -1175,29 +1110,12 @@ tap_tx_queue_setup(struct rte_eth_dev *dev,
>  		return -1;
>  	dev->data->tx_queues[tx_queue_id] = &internals->txq[tx_queue_id];
>  	txq = dev->data->tx_queues[tx_queue_id];
> -	/*
> -	 * Don't verify port offloads for application which
> -	 * use the old API.
> -	 */
> -	if (tx_conf != NULL &&
> -	    !!(tx_conf->txq_flags & ETH_TXQ_FLAGS_IGNORE)) {
> -		if (tap_txq_are_offloads_valid(dev, tx_conf->offloads)) {
> -			txq->csum = !!(tx_conf->offloads &
> -					(DEV_TX_OFFLOAD_IPV4_CKSUM |
> -					 DEV_TX_OFFLOAD_UDP_CKSUM |
> -					 DEV_TX_OFFLOAD_TCP_CKSUM));
> -		} else {
> -			rte_errno = ENOTSUP;
> -			RTE_LOG(ERR, PMD,
> -				"%p: Tx queue offloads 0x%" PRIx64
> -				" don't match port offloads 0x%" PRIx64
> -				" or supported offloads 0x%" PRIx64,
> -				(void *)dev, tx_conf->offloads,
> -				dev->data->dev_conf.txmode.offloads,
> -				tap_tx_offload_get_port_capa());
> -			return -rte_errno;
> -		}
> -	}
> +

The tap_txq_are_offloads_valid() call is essential. I suggest to leave it in place.
The RTE_LOG message could drop comparison between queue and port capabilities:

			RTE_LOG(ERR, PMD,
				"%p: Tx queue offloads 0x%" PRIx64
				" don't match"
				" supported offloads 0x%" PRIx64,
				(void *)dev, tx_conf->offloads,
				tap_tx_offload_get_queue_capa());

> +	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;
> --
> 2.14.3

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

[Ferruh Yigit]

On 4/25/2018 10:18 AM, Ophir Munk wrote:
> Hi Ferruh,
> 
> I should have mentioned earlier that TAP does support queue specific capabilities. 
> Please look in tap_queue_setup() and note that each TAP queue is created with a distinct file descriptor (fd).
> Then supporting an offload capability is just implementing it in SW (e.g. calculating IP checksum).
> 
> If the main assumption of this patch was that TAP does not support queue specific offloads - then please consider this patch again.

Yes that was the initial question, is tap supports queue specific offloads or
not. Thanks for the answer.

> 
> On the other hand there is no port specific capability supported by TAP. 

If so verify functions are wrong, that was the error I got.
It seems copy/paste of mlx one but the port_supp_offloads has different meaning
there.

> However, in order to support legacy applications, port capabilities are usually reported as the OR operation between queue & port capabilities. 
> TAP currently clones the queue capabilities to port capabilities. We could optimize this cloning by always return queue capabilities when queried about queues or ports. In this case - tap_rx_offload_get_port_capa() and tap_tx_offload_get_port_capa() could be removed.

Instead of removing the functions I think you can keep them but return correct
values, in this case return empty, this will make the exiting validation
functions correct.

Can you send a fix for that?
If no fix sent, I suggest going with this patch to remove queue level offload
support until it is fixed.

> 
> Please find more comments inline.
> 
>> -----Original Message-----
>> From: Ferruh Yigit [mailto:ferruh.yigit@intel.com]
>> Sent: Tuesday, April 24, 2018 8:54 PM
>> To: Pascal Mazon <pascal.mazon@6wind.com>
>> Cc: dev@dpdk.org; Ferruh Yigit <ferruh.yigit@intel.com>; Mordechay
>> Haimovsky <motih@mellanox.com>; Ophir Munk <ophirmu@mellanox.com>
>> Subject: [PATCH v3] net/tap: remove queue specific offload support
>>
>> 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>
>> ---
>> Cc: Ophir Munk <ophirmu@mellanox.com>
>>
>> v2:
>> * rebased
>>
>> v3:
>> * txq->csum restored,
>>   - ETH_TXQ_FLAGS_IGNORE check removed since ethdev layer takes care of
>> it
>>   - tx_conf != NULL check removed, this is internal api who calls this is
>>   ethdev and it doesn't pass null tx_conf
>> ---
>>  drivers/net/tap/rte_eth_tap.c | 102 +++++-------------------------------------
>>  1 file changed, 10 insertions(+), 92 deletions(-)
>>
>> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
>> index ef33aace9..61b4b5df3 100644
>> --- a/drivers/net/tap/rte_eth_tap.c
>> +++ b/drivers/net/tap/rte_eth_tap.c
>> @@ -278,31 +278,6 @@ tap_rx_offload_get_port_capa(void)
>>  	       DEV_RX_OFFLOAD_CRC_STRIP;
>>  }
>>
>> -static uint64_t
>> -tap_rx_offload_get_queue_capa(void)
>> -{
>> -	return DEV_RX_OFFLOAD_SCATTER |
>> -	       DEV_RX_OFFLOAD_IPV4_CKSUM |
>> -	       DEV_RX_OFFLOAD_UDP_CKSUM |
>> -	       DEV_RX_OFFLOAD_TCP_CKSUM |
>> -	       DEV_RX_OFFLOAD_CRC_STRIP;
>> -}
>> -
> 
> TAP PMD supports all of these RX queue specific offloads. I suggest to leave this function in place.
> 
>> -static bool
>> -tap_rxq_are_offloads_valid(struct rte_eth_dev *dev, uint64_t offloads) -{
>> -	uint64_t port_offloads = dev->data->dev_conf.rxmode.offloads;
>> -	uint64_t queue_supp_offloads = tap_rx_offload_get_queue_capa();
>> -	uint64_t port_supp_offloads = tap_rx_offload_get_port_capa();
>> -
>> -	if ((offloads & (queue_supp_offloads | port_supp_offloads)) !=
>> -	    offloads)
>> -		return false;
>> -	if ((port_offloads ^ offloads) & port_supp_offloads)
>> -		return false;
>> -	return true;
>> -}
>> -
> 
> Putting aside the fact that queue offloads equals port offloads (so could ignore "port_supp_offload" variable) - this function is essential to validate that the configured Rx offloads are supported by TAP. I suggest to leave this function in place.
> Without it - testpmd falsely confirms non supported offloads.
> For example before this patch: offloading "hw-vlan-filter" will fail as expected:
> 
> testpmd> port config all
> testpmd> port config all hw-vlan-filter on
> testpmd> port start all
> Configuring Port 0 (socket 0)
> PMD: net_tap0: 0x1209fc0: TX configured queues number: 1
> PMD: net_tap0: 0x1209fc0: RX configured queues number: 1
> PMD: 0x1209fc0: Rx queue offloads 0x120e don't match port offloads 0x120e or supported offloads 0x300e
> Fail to configure port 0 rx queues
> 
> However, with this patch this configuration is falsely accepted.
> 
>>  /* Callback to handle the rx burst of packets to the correct interface and
>>   * file descriptor(s) in a multi-queue setup.
>>   */
>> @@ -411,31 +386,6 @@ tap_tx_offload_get_port_capa(void)
>>  	       DEV_TX_OFFLOAD_TCP_CKSUM;
>>  }
>>
>> -static uint64_t
>> -tap_tx_offload_get_queue_capa(void)
>> -{
>> -	return DEV_TX_OFFLOAD_MULTI_SEGS |
>> -	       DEV_TX_OFFLOAD_IPV4_CKSUM |
>> -	       DEV_TX_OFFLOAD_UDP_CKSUM |
>> -	       DEV_TX_OFFLOAD_TCP_CKSUM;
>> -}
>> -
> 
> TAP PMD supports all of these TX queue specific offloads. I suggest to leave this function in place.
> 
>> -static bool
>> -tap_txq_are_offloads_valid(struct rte_eth_dev *dev, uint64_t offloads) -{
>> -	uint64_t port_offloads = dev->data->dev_conf.txmode.offloads;
>> -	uint64_t queue_supp_offloads = tap_tx_offload_get_queue_capa();
>> -	uint64_t port_supp_offloads = tap_tx_offload_get_port_capa();
>> -
>> -	if ((offloads & (queue_supp_offloads | port_supp_offloads)) !=
>> -	    offloads)
>> -		return false;
>> -	/* Verify we have no conflict with port offloads */
>> -	if ((port_offloads ^ offloads) & port_supp_offloads)
>> -		return false;
>> -	return true;
>> -}
>> -
> 
> This function is essential to validate that the configured Tx offloads are supported by TAP. 
> I suggest to leave this function in place.
> 
>>  static void
>>  tap_tx_offload(char *packet, uint64_t ol_flags, unsigned int l2_len,
>>  	       unsigned int l3_len)
>> @@ -763,12 +713,10 @@ tap_dev_info(struct rte_eth_dev *dev, struct
>> rte_eth_dev_info *dev_info)
>>  	dev_info->max_tx_queues = RTE_PMD_TAP_MAX_QUEUES;
>>  	dev_info->min_rx_bufsize = 0;
>>  	dev_info->speed_capa = tap_dev_speed_capa();
>> -	dev_info->rx_queue_offload_capa =
>> tap_rx_offload_get_queue_capa();
>> -	dev_info->rx_offload_capa = tap_rx_offload_get_port_capa() |
>> -				    dev_info->rx_queue_offload_capa;
>> -	dev_info->tx_queue_offload_capa =
>> tap_tx_offload_get_queue_capa();
>> -	dev_info->tx_offload_capa = tap_tx_offload_get_port_capa() |
>> -				    dev_info->tx_queue_offload_capa;
>> +	dev_info->rx_offload_capa = tap_rx_offload_get_port_capa();
>> +	dev_info->tx_offload_capa = tap_tx_offload_get_port_capa();
>> +	dev_info->rx_queue_offload_capa = 0;
>> +	dev_info->tx_queue_offload_capa = 0;
>>  }
>>
> 
> Rx_queue_offloads_capa should be reported as before:
> dev_info->tx_queue_offload_capa = tap_tx_offload_get_queue_capa();
> Same for TX offloads.
> 
> Port capabilities could return queue capabilities:
> 
> Instead of:
> 
> dev_info->rx_offload_capa = tap_rx_offload_get_port_capa() |
> 				    dev_info->rx_queue_offload_capa;
> 
> We could return:
> 
> dev_info->rx_offload_capa = dev_info->rx_queue_offload_capa;
> 
> The same argument is valid for Tx as well.
> 
>>  static int
>> @@ -1094,19 +1042,6 @@ tap_rx_queue_setup(struct rte_eth_dev *dev,
>>  		return -1;
>>  	}
>>
>> -	/* Verify application offloads are valid for our port and queue. */
>> -	if (!tap_rxq_are_offloads_valid(dev, rx_conf->offloads)) {
>> -		rte_errno = ENOTSUP;
>> -		RTE_LOG(ERR, PMD,
>> -			"%p: Rx queue offloads 0x%" PRIx64
>> -			" don't match port offloads 0x%" PRIx64
>> -			" or supported offloads 0x%" PRIx64 "\n",
>> -			(void *)dev, rx_conf->offloads,
>> -			dev->data->dev_conf.rxmode.offloads,
>> -			(tap_rx_offload_get_port_capa() |
>> -			 tap_rx_offload_get_queue_capa()));
>> -		return -rte_errno;
>> -	}
> 
> The tap_rxq_are_offloads_valid() call is essential. I suggest to leave it in place.
> The RTE_LOG could drop port references to become:
> 
> 		RTE_LOG(ERR, PMD,
> 			"%p: Rx queue offloads 0x%" PRIx64
> 			" don't match"
> 			" supported offloads 0x%" PRIx64 "\n",
> 			(void *)dev, rx_conf->offloads,
> 			 tap_rx_offload_get_queue_capa()));
> 
> 
>>  	rxq->mp = mp;
>>  	rxq->trigger_seen = 1; /* force initial burst */
>>  	rxq->in_port = dev->data->port_id;
>> @@ -1175,29 +1110,12 @@ tap_tx_queue_setup(struct rte_eth_dev *dev,
>>  		return -1;
>>  	dev->data->tx_queues[tx_queue_id] = &internals->txq[tx_queue_id];
>>  	txq = dev->data->tx_queues[tx_queue_id];
>> -	/*
>> -	 * Don't verify port offloads for application which
>> -	 * use the old API.
>> -	 */
>> -	if (tx_conf != NULL &&
>> -	    !!(tx_conf->txq_flags & ETH_TXQ_FLAGS_IGNORE)) {
>> -		if (tap_txq_are_offloads_valid(dev, tx_conf->offloads)) {
>> -			txq->csum = !!(tx_conf->offloads &
>> -					(DEV_TX_OFFLOAD_IPV4_CKSUM |
>> -					 DEV_TX_OFFLOAD_UDP_CKSUM |
>> -					 DEV_TX_OFFLOAD_TCP_CKSUM));
>> -		} else {
>> -			rte_errno = ENOTSUP;
>> -			RTE_LOG(ERR, PMD,
>> -				"%p: Tx queue offloads 0x%" PRIx64
>> -				" don't match port offloads 0x%" PRIx64
>> -				" or supported offloads 0x%" PRIx64,
>> -				(void *)dev, tx_conf->offloads,
>> -				dev->data->dev_conf.txmode.offloads,
>> -				tap_tx_offload_get_port_capa());
>> -			return -rte_errno;
>> -		}
>> -	}
>> +
> 
> The tap_txq_are_offloads_valid() call is essential. I suggest to leave it in place.
> The RTE_LOG message could drop comparison between queue and port capabilities:
> 
> 			RTE_LOG(ERR, PMD,
> 				"%p: Tx queue offloads 0x%" PRIx64
> 				" don't match"
> 				" supported offloads 0x%" PRIx64,
> 				(void *)dev, tx_conf->offloads,
> 				tap_tx_offload_get_queue_capa());
> 
>> +	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;
>> --
>> 2.14.3
> 

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

[Ophir Munk]

> -----Original Message-----
> From: Ferruh Yigit [mailto:ferruh.yigit@intel.com]
> Sent: Wednesday, April 25, 2018 12:48 PM
> To: Ophir Munk <ophirmu@mellanox.com>; Pascal Mazon
> <pascal.mazon@6wind.com>
> Cc: dev@dpdk.org; Mordechay Haimovsky <motih@mellanox.com>; Olga
> Shern <olgas@mellanox.com>; Thomas Monjalon <thomas@monjalon.net>;
> Raslan Darawsheh <rasland@mellanox.com>; Shahaf Shuler
> <shahafs@mellanox.com>
> Subject: Re: [PATCH v3] net/tap: remove queue specific offload support
> 
> On 4/25/2018 10:18 AM, Ophir Munk wrote:
> > Hi Ferruh,
> >
> > I should have mentioned earlier that TAP does support queue specific
> capabilities.
> > Please look in tap_queue_setup() and note that each TAP queue is created
> with a distinct file descriptor (fd).
> > Then supporting an offload capability is just implementing it in SW (e.g.
> calculating IP checksum).
> >
> > If the main assumption of this patch was that TAP does not support queue
> specific offloads - then please consider this patch again.
> 
> Yes that was the initial question, is tap supports queue specific offloads or
> not. Thanks for the answer.
> 
> >
> > On the other hand there is no port specific capability supported by TAP.
> 
> If so verify functions are wrong, that was the error I got.

Can you please specify the test you did what error you got? 
If I fix something I want to verify what I am fixing.

> It seems copy/paste of mlx one but the port_supp_offloads has different
> meaning there.
> 
> > However, in order to support legacy applications, port capabilities are
> usually reported as the OR operation between queue & port capabilities.
> > TAP currently clones the queue capabilities to port capabilities. We could
> optimize this cloning by always return queue capabilities when queried about
> queues or ports. In this case - tap_rx_offload_get_port_capa() and
> tap_tx_offload_get_port_capa() could be removed.
> 
> Instead of removing the functions I think you can keep them but return
> correct values, in this case return empty, this will make the exiting validation
> functions correct.
> 
> Can you send a fix for that?
> If no fix sent, I suggest going with this patch to remove queue level offload
> support until it is fixed.
> 
> >
> > Please find more comments inline.
> >
> >> -----Original Message-----
> >> From: Ferruh Yigit [mailto:ferruh.yigit@intel.com]
> >> Sent: Tuesday, April 24, 2018 8:54 PM
> >> To: Pascal Mazon <pascal.mazon@6wind.com>
> >> Cc: dev@dpdk.org; Ferruh Yigit <ferruh.yigit@intel.com>; Mordechay
> >> Haimovsky <motih@mellanox.com>; Ophir Munk
> <ophirmu@mellanox.com>
> >> Subject: [PATCH v3] net/tap: remove queue specific offload support
> >>
> >> 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>
> >> ---
> >> Cc: Ophir Munk <ophirmu@mellanox.com>
> >>
> >> v2:
> >> * rebased
> >>
> >> v3:
> >> * txq->csum restored,
> >>   - ETH_TXQ_FLAGS_IGNORE check removed since ethdev layer takes care
> >> of it
> >>   - tx_conf != NULL check removed, this is internal api who calls this is
> >>   ethdev and it doesn't pass null tx_conf
> >> ---
> >>  drivers/net/tap/rte_eth_tap.c | 102
> >> +++++-------------------------------------
> >>  1 file changed, 10 insertions(+), 92 deletions(-)
> >>
> >> diff --git a/drivers/net/tap/rte_eth_tap.c
> >> b/drivers/net/tap/rte_eth_tap.c index ef33aace9..61b4b5df3 100644
> >> --- a/drivers/net/tap/rte_eth_tap.c
> >> +++ b/drivers/net/tap/rte_eth_tap.c
> >> @@ -278,31 +278,6 @@ tap_rx_offload_get_port_capa(void)
> >>  	       DEV_RX_OFFLOAD_CRC_STRIP;
> >>  }
> >>
> >> -static uint64_t
> >> -tap_rx_offload_get_queue_capa(void)
> >> -{
> >> -	return DEV_RX_OFFLOAD_SCATTER |
> >> -	       DEV_RX_OFFLOAD_IPV4_CKSUM |
> >> -	       DEV_RX_OFFLOAD_UDP_CKSUM |
> >> -	       DEV_RX_OFFLOAD_TCP_CKSUM |
> >> -	       DEV_RX_OFFLOAD_CRC_STRIP;
> >> -}
> >> -
> >
> > TAP PMD supports all of these RX queue specific offloads. I suggest to
> leave this function in place.
> >
> >> -static bool
> >> -tap_rxq_are_offloads_valid(struct rte_eth_dev *dev, uint64_t offloads) -
> {
> >> -	uint64_t port_offloads = dev->data->dev_conf.rxmode.offloads;
> >> -	uint64_t queue_supp_offloads = tap_rx_offload_get_queue_capa();
> >> -	uint64_t port_supp_offloads = tap_rx_offload_get_port_capa();
> >> -
> >> -	if ((offloads & (queue_supp_offloads | port_supp_offloads)) !=
> >> -	    offloads)
> >> -		return false;
> >> -	if ((port_offloads ^ offloads) & port_supp_offloads)
> >> -		return false;
> >> -	return true;
> >> -}
> >> -
> >
> > Putting aside the fact that queue offloads equals port offloads (so could
> ignore "port_supp_offload" variable) - this function is essential to validate
> that the configured Rx offloads are supported by TAP. I suggest to leave this
> function in place.
> > Without it - testpmd falsely confirms non supported offloads.
> > For example before this patch: offloading "hw-vlan-filter" will fail as
> expected:
> >
> > testpmd> port config all
> > testpmd> port config all hw-vlan-filter on port start all
> > Configuring Port 0 (socket 0)
> > PMD: net_tap0: 0x1209fc0: TX configured queues number: 1
> > PMD: net_tap0: 0x1209fc0: RX configured queues number: 1
> > PMD: 0x1209fc0: Rx queue offloads 0x120e don't match port offloads
> > 0x120e or supported offloads 0x300e Fail to configure port 0 rx queues
> >
> > However, with this patch this configuration is falsely accepted.
> >
> >>  /* Callback to handle the rx burst of packets to the correct interface and
> >>   * file descriptor(s) in a multi-queue setup.
> >>   */
> >> @@ -411,31 +386,6 @@ tap_tx_offload_get_port_capa(void)
> >>  	       DEV_TX_OFFLOAD_TCP_CKSUM;
> >>  }
> >>
> >> -static uint64_t
> >> -tap_tx_offload_get_queue_capa(void)
> >> -{
> >> -	return DEV_TX_OFFLOAD_MULTI_SEGS |
> >> -	       DEV_TX_OFFLOAD_IPV4_CKSUM |
> >> -	       DEV_TX_OFFLOAD_UDP_CKSUM |
> >> -	       DEV_TX_OFFLOAD_TCP_CKSUM;
> >> -}
> >> -
> >
> > TAP PMD supports all of these TX queue specific offloads. I suggest to
> leave this function in place.
> >
> >> -static bool
> >> -tap_txq_are_offloads_valid(struct rte_eth_dev *dev, uint64_t offloads) -
> {
> >> -	uint64_t port_offloads = dev->data->dev_conf.txmode.offloads;
> >> -	uint64_t queue_supp_offloads = tap_tx_offload_get_queue_capa();
> >> -	uint64_t port_supp_offloads = tap_tx_offload_get_port_capa();
> >> -
> >> -	if ((offloads & (queue_supp_offloads | port_supp_offloads)) !=
> >> -	    offloads)
> >> -		return false;
> >> -	/* Verify we have no conflict with port offloads */
> >> -	if ((port_offloads ^ offloads) & port_supp_offloads)
> >> -		return false;
> >> -	return true;
> >> -}
> >> -
> >
> > This function is essential to validate that the configured Tx offloads are
> supported by TAP.
> > I suggest to leave this function in place.
> >
> >>  static void
> >>  tap_tx_offload(char *packet, uint64_t ol_flags, unsigned int l2_len,
> >>  	       unsigned int l3_len)
> >> @@ -763,12 +713,10 @@ tap_dev_info(struct rte_eth_dev *dev, struct
> >> rte_eth_dev_info *dev_info)
> >>  	dev_info->max_tx_queues = RTE_PMD_TAP_MAX_QUEUES;
> >>  	dev_info->min_rx_bufsize = 0;
> >>  	dev_info->speed_capa = tap_dev_speed_capa();
> >> -	dev_info->rx_queue_offload_capa =
> >> tap_rx_offload_get_queue_capa();
> >> -	dev_info->rx_offload_capa = tap_rx_offload_get_port_capa() |
> >> -				    dev_info->rx_queue_offload_capa;
> >> -	dev_info->tx_queue_offload_capa =
> >> tap_tx_offload_get_queue_capa();
> >> -	dev_info->tx_offload_capa = tap_tx_offload_get_port_capa() |
> >> -				    dev_info->tx_queue_offload_capa;
> >> +	dev_info->rx_offload_capa = tap_rx_offload_get_port_capa();
> >> +	dev_info->tx_offload_capa = tap_tx_offload_get_port_capa();
> >> +	dev_info->rx_queue_offload_capa = 0;
> >> +	dev_info->tx_queue_offload_capa = 0;
> >>  }
> >>
> >
> > Rx_queue_offloads_capa should be reported as before:
> > dev_info->tx_queue_offload_capa = tap_tx_offload_get_queue_capa();
> > Same for TX offloads.
> >
> > Port capabilities could return queue capabilities:
> >
> > Instead of:
> >
> > dev_info->rx_offload_capa = tap_rx_offload_get_port_capa() |
> > 				    dev_info->rx_queue_offload_capa;
> >
> > We could return:
> >
> > dev_info->rx_offload_capa = dev_info->rx_queue_offload_capa;
> >
> > The same argument is valid for Tx as well.
> >
> >>  static int
> >> @@ -1094,19 +1042,6 @@ tap_rx_queue_setup(struct rte_eth_dev *dev,
> >>  		return -1;
> >>  	}
> >>
> >> -	/* Verify application offloads are valid for our port and queue. */
> >> -	if (!tap_rxq_are_offloads_valid(dev, rx_conf->offloads)) {
> >> -		rte_errno = ENOTSUP;
> >> -		RTE_LOG(ERR, PMD,
> >> -			"%p: Rx queue offloads 0x%" PRIx64
> >> -			" don't match port offloads 0x%" PRIx64
> >> -			" or supported offloads 0x%" PRIx64 "\n",
> >> -			(void *)dev, rx_conf->offloads,
> >> -			dev->data->dev_conf.rxmode.offloads,
> >> -			(tap_rx_offload_get_port_capa() |
> >> -			 tap_rx_offload_get_queue_capa()));
> >> -		return -rte_errno;
> >> -	}
> >
> > The tap_rxq_are_offloads_valid() call is essential. I suggest to leave it in
> place.
> > The RTE_LOG could drop port references to become:
> >
> > 		RTE_LOG(ERR, PMD,
> > 			"%p: Rx queue offloads 0x%" PRIx64
> > 			" don't match"
> > 			" supported offloads 0x%" PRIx64 "\n",
> > 			(void *)dev, rx_conf->offloads,
> > 			 tap_rx_offload_get_queue_capa()));
> >
> >
> >>  	rxq->mp = mp;
> >>  	rxq->trigger_seen = 1; /* force initial burst */
> >>  	rxq->in_port = dev->data->port_id;
> >> @@ -1175,29 +1110,12 @@ tap_tx_queue_setup(struct rte_eth_dev
> *dev,
> >>  		return -1;
> >>  	dev->data->tx_queues[tx_queue_id] = &internals->txq[tx_queue_id];
> >>  	txq = dev->data->tx_queues[tx_queue_id];
> >> -	/*
> >> -	 * Don't verify port offloads for application which
> >> -	 * use the old API.
> >> -	 */
> >> -	if (tx_conf != NULL &&
> >> -	    !!(tx_conf->txq_flags & ETH_TXQ_FLAGS_IGNORE)) {
> >> -		if (tap_txq_are_offloads_valid(dev, tx_conf->offloads)) {
> >> -			txq->csum = !!(tx_conf->offloads &
> >> -					(DEV_TX_OFFLOAD_IPV4_CKSUM |
> >> -					 DEV_TX_OFFLOAD_UDP_CKSUM |
> >> -					 DEV_TX_OFFLOAD_TCP_CKSUM));
> >> -		} else {
> >> -			rte_errno = ENOTSUP;
> >> -			RTE_LOG(ERR, PMD,
> >> -				"%p: Tx queue offloads 0x%" PRIx64
> >> -				" don't match port offloads 0x%" PRIx64
> >> -				" or supported offloads 0x%" PRIx64,
> >> -				(void *)dev, tx_conf->offloads,
> >> -				dev->data->dev_conf.txmode.offloads,
> >> -				tap_tx_offload_get_port_capa());
> >> -			return -rte_errno;
> >> -		}
> >> -	}
> >> +
> >
> > The tap_txq_are_offloads_valid() call is essential. I suggest to leave it in
> place.
> > The RTE_LOG message could drop comparison between queue and port
> capabilities:
> >
> > 			RTE_LOG(ERR, PMD,
> > 				"%p: Tx queue offloads 0x%" PRIx64
> > 				" don't match"
> > 				" supported offloads 0x%" PRIx64,
> > 				(void *)dev, tx_conf->offloads,
> > 				tap_tx_offload_get_queue_capa());
> >
> >> +	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;
> >> --
> >> 2.14.3
> >

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

[Ophir Munk]

Hi Ferruh,
I started working on a patch.
No need for your test example.

> -----Original Message-----
> From: Ophir Munk
> Sent: Wednesday, April 25, 2018 3:00 PM
> To: 'Ferruh Yigit' <ferruh.yigit@intel.com>; Pascal Mazon
> <pascal.mazon@6wind.com>
> Cc: dev@dpdk.org; Mordechay Haimovsky <motih@mellanox.com>; Olga
> Shern <olgas@mellanox.com>; Thomas Monjalon <thomas@monjalon.net>;
> Raslan Darawsheh <rasland@mellanox.com>; Shahaf Shuler
> <shahafs@mellanox.com>
> Subject: RE: [PATCH v3] net/tap: remove queue specific offload support
> 
> 
> 
> > -----Original Message-----
> > From: Ferruh Yigit [mailto:ferruh.yigit@intel.com]
> > Sent: Wednesday, April 25, 2018 12:48 PM
> > To: Ophir Munk <ophirmu@mellanox.com>; Pascal Mazon
> > <pascal.mazon@6wind.com>
> > Cc: dev@dpdk.org; Mordechay Haimovsky <motih@mellanox.com>; Olga
> Shern
> > <olgas@mellanox.com>; Thomas Monjalon <thomas@monjalon.net>;
> Raslan
> > Darawsheh <rasland@mellanox.com>; Shahaf Shuler
> <shahafs@mellanox.com>
> > Subject: Re: [PATCH v3] net/tap: remove queue specific offload support
> >
> > On 4/25/2018 10:18 AM, Ophir Munk wrote:
> > > Hi Ferruh,
> > >
> > > I should have mentioned earlier that TAP does support queue specific
> > capabilities.
> > > Please look in tap_queue_setup() and note that each TAP queue is
> > > created
> > with a distinct file descriptor (fd).
> > > Then supporting an offload capability is just implementing it in SW (e.g.
> > calculating IP checksum).
> > >
> > > If the main assumption of this patch was that TAP does not support
> > > queue
> > specific offloads - then please consider this patch again.
> >
> > Yes that was the initial question, is tap supports queue specific
> > offloads or not. Thanks for the answer.
> >
> > >
> > > On the other hand there is no port specific capability supported by TAP.
> >
> > If so verify functions are wrong, that was the error I got.
> 
> Can you please specify the test you did what error you got?
> If I fix something I want to verify what I am fixing.
> 
> > It seems copy/paste of mlx one but the port_supp_offloads has
> > different meaning there.
> >
> > > However, in order to support legacy applications, port capabilities
> > > are
> > usually reported as the OR operation between queue & port capabilities.
> > > TAP currently clones the queue capabilities to port capabilities. We
> > > could
> > optimize this cloning by always return queue capabilities when queried
> > about queues or ports. In this case - tap_rx_offload_get_port_capa()
> > and
> > tap_tx_offload_get_port_capa() could be removed.
> >
> > Instead of removing the functions I think you can keep them but return
> > correct values, in this case return empty, this will make the exiting
> > validation functions correct.
> >
> > Can you send a fix for that?
> > If no fix sent, I suggest going with this patch to remove queue level
> > offload support until it is fixed.
> >
> > >
> > > Please find more comments inline.
> > >
> > >> -----Original Message-----
> > >> From: Ferruh Yigit [mailto:ferruh.yigit@intel.com]
> > >> Sent: Tuesday, April 24, 2018 8:54 PM
> > >> To: Pascal Mazon <pascal.mazon@6wind.com>
> > >> Cc: dev@dpdk.org; Ferruh Yigit <ferruh.yigit@intel.com>; Mordechay
> > >> Haimovsky <motih@mellanox.com>; Ophir Munk
> > <ophirmu@mellanox.com>
> > >> Subject: [PATCH v3] net/tap: remove queue specific offload support
> > >>
> > >> 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>
> > >> ---
> > >> Cc: Ophir Munk <ophirmu@mellanox.com>
> > >>
> > >> v2:
> > >> * rebased
> > >>
> > >> v3:
> > >> * txq->csum restored,
> > >>   - ETH_TXQ_FLAGS_IGNORE check removed since ethdev layer takes
> > >> care of it
> > >>   - tx_conf != NULL check removed, this is internal api who calls this is
> > >>   ethdev and it doesn't pass null tx_conf
> > >> ---
> > >>  drivers/net/tap/rte_eth_tap.c | 102
> > >> +++++-------------------------------------
> > >>  1 file changed, 10 insertions(+), 92 deletions(-)
> > >>
> > >> diff --git a/drivers/net/tap/rte_eth_tap.c
> > >> b/drivers/net/tap/rte_eth_tap.c index ef33aace9..61b4b5df3 100644
> > >> --- a/drivers/net/tap/rte_eth_tap.c
> > >> +++ b/drivers/net/tap/rte_eth_tap.c
> > >> @@ -278,31 +278,6 @@ tap_rx_offload_get_port_capa(void)
> > >>  	       DEV_RX_OFFLOAD_CRC_STRIP;
> > >>  }
> > >>
> > >> -static uint64_t
> > >> -tap_rx_offload_get_queue_capa(void)
> > >> -{
> > >> -	return DEV_RX_OFFLOAD_SCATTER |
> > >> -	       DEV_RX_OFFLOAD_IPV4_CKSUM |
> > >> -	       DEV_RX_OFFLOAD_UDP_CKSUM |
> > >> -	       DEV_RX_OFFLOAD_TCP_CKSUM |
> > >> -	       DEV_RX_OFFLOAD_CRC_STRIP;
> > >> -}
> > >> -
> > >
> > > TAP PMD supports all of these RX queue specific offloads. I suggest
> > > to
> > leave this function in place.
> > >
> > >> -static bool
> > >> -tap_rxq_are_offloads_valid(struct rte_eth_dev *dev, uint64_t
> > >> offloads) -
> > {
> > >> -	uint64_t port_offloads = dev->data->dev_conf.rxmode.offloads;
> > >> -	uint64_t queue_supp_offloads = tap_rx_offload_get_queue_capa();
> > >> -	uint64_t port_supp_offloads = tap_rx_offload_get_port_capa();
> > >> -
> > >> -	if ((offloads & (queue_supp_offloads | port_supp_offloads)) !=
> > >> -	    offloads)
> > >> -		return false;
> > >> -	if ((port_offloads ^ offloads) & port_supp_offloads)
> > >> -		return false;
> > >> -	return true;
> > >> -}
> > >> -
> > >
> > > Putting aside the fact that queue offloads equals port offloads (so
> > > could
> > ignore "port_supp_offload" variable) - this function is essential to
> > validate that the configured Rx offloads are supported by TAP. I
> > suggest to leave this function in place.
> > > Without it - testpmd falsely confirms non supported offloads.
> > > For example before this patch: offloading "hw-vlan-filter" will fail
> > > as
> > expected:
> > >
> > > testpmd> port config all
> > > testpmd> port config all hw-vlan-filter on port start all
> > > Configuring Port 0 (socket 0)
> > > PMD: net_tap0: 0x1209fc0: TX configured queues number: 1
> > > PMD: net_tap0: 0x1209fc0: RX configured queues number: 1
> > > PMD: 0x1209fc0: Rx queue offloads 0x120e don't match port offloads
> > > 0x120e or supported offloads 0x300e Fail to configure port 0 rx
> > > queues
> > >
> > > However, with this patch this configuration is falsely accepted.
> > >
> > >>  /* Callback to handle the rx burst of packets to the correct interface
> and
> > >>   * file descriptor(s) in a multi-queue setup.
> > >>   */
> > >> @@ -411,31 +386,6 @@ tap_tx_offload_get_port_capa(void)
> > >>  	       DEV_TX_OFFLOAD_TCP_CKSUM;
> > >>  }
> > >>
> > >> -static uint64_t
> > >> -tap_tx_offload_get_queue_capa(void)
> > >> -{
> > >> -	return DEV_TX_OFFLOAD_MULTI_SEGS |
> > >> -	       DEV_TX_OFFLOAD_IPV4_CKSUM |
> > >> -	       DEV_TX_OFFLOAD_UDP_CKSUM |
> > >> -	       DEV_TX_OFFLOAD_TCP_CKSUM;
> > >> -}
> > >> -
> > >
> > > TAP PMD supports all of these TX queue specific offloads. I suggest
> > > to
> > leave this function in place.
> > >
> > >> -static bool
> > >> -tap_txq_are_offloads_valid(struct rte_eth_dev *dev, uint64_t
> > >> offloads) -
> > {
> > >> -	uint64_t port_offloads = dev->data->dev_conf.txmode.offloads;
> > >> -	uint64_t queue_supp_offloads = tap_tx_offload_get_queue_capa();
> > >> -	uint64_t port_supp_offloads = tap_tx_offload_get_port_capa();
> > >> -
> > >> -	if ((offloads & (queue_supp_offloads | port_supp_offloads)) !=
> > >> -	    offloads)
> > >> -		return false;
> > >> -	/* Verify we have no conflict with port offloads */
> > >> -	if ((port_offloads ^ offloads) & port_supp_offloads)
> > >> -		return false;
> > >> -	return true;
> > >> -}
> > >> -
> > >
> > > This function is essential to validate that the configured Tx
> > > offloads are
> > supported by TAP.
> > > I suggest to leave this function in place.
> > >
> > >>  static void
> > >>  tap_tx_offload(char *packet, uint64_t ol_flags, unsigned int l2_len,
> > >>  	       unsigned int l3_len)
> > >> @@ -763,12 +713,10 @@ tap_dev_info(struct rte_eth_dev *dev, struct
> > >> rte_eth_dev_info *dev_info)
> > >>  	dev_info->max_tx_queues = RTE_PMD_TAP_MAX_QUEUES;
> > >>  	dev_info->min_rx_bufsize = 0;
> > >>  	dev_info->speed_capa = tap_dev_speed_capa();
> > >> -	dev_info->rx_queue_offload_capa =
> > >> tap_rx_offload_get_queue_capa();
> > >> -	dev_info->rx_offload_capa = tap_rx_offload_get_port_capa() |
> > >> -				    dev_info->rx_queue_offload_capa;
> > >> -	dev_info->tx_queue_offload_capa =
> > >> tap_tx_offload_get_queue_capa();
> > >> -	dev_info->tx_offload_capa = tap_tx_offload_get_port_capa() |
> > >> -				    dev_info->tx_queue_offload_capa;
> > >> +	dev_info->rx_offload_capa = tap_rx_offload_get_port_capa();
> > >> +	dev_info->tx_offload_capa = tap_tx_offload_get_port_capa();
> > >> +	dev_info->rx_queue_offload_capa = 0;
> > >> +	dev_info->tx_queue_offload_capa = 0;
> > >>  }
> > >>
> > >
> > > Rx_queue_offloads_capa should be reported as before:
> > > dev_info->tx_queue_offload_capa = tap_tx_offload_get_queue_capa();
> > > Same for TX offloads.
> > >
> > > Port capabilities could return queue capabilities:
> > >
> > > Instead of:
> > >
> > > dev_info->rx_offload_capa = tap_rx_offload_get_port_capa() |
> > > 				    dev_info->rx_queue_offload_capa;
> > >
> > > We could return:
> > >
> > > dev_info->rx_offload_capa = dev_info->rx_queue_offload_capa;
> > >
> > > The same argument is valid for Tx as well.
> > >
> > >>  static int
> > >> @@ -1094,19 +1042,6 @@ tap_rx_queue_setup(struct rte_eth_dev
> *dev,
> > >>  		return -1;
> > >>  	}
> > >>
> > >> -	/* Verify application offloads are valid for our port and queue. */
> > >> -	if (!tap_rxq_are_offloads_valid(dev, rx_conf->offloads)) {
> > >> -		rte_errno = ENOTSUP;
> > >> -		RTE_LOG(ERR, PMD,
> > >> -			"%p: Rx queue offloads 0x%" PRIx64
> > >> -			" don't match port offloads 0x%" PRIx64
> > >> -			" or supported offloads 0x%" PRIx64 "\n",
> > >> -			(void *)dev, rx_conf->offloads,
> > >> -			dev->data->dev_conf.rxmode.offloads,
> > >> -			(tap_rx_offload_get_port_capa() |
> > >> -			 tap_rx_offload_get_queue_capa()));
> > >> -		return -rte_errno;
> > >> -	}
> > >
> > > The tap_rxq_are_offloads_valid() call is essential. I suggest to
> > > leave it in
> > place.
> > > The RTE_LOG could drop port references to become:
> > >
> > > 		RTE_LOG(ERR, PMD,
> > > 			"%p: Rx queue offloads 0x%" PRIx64
> > > 			" don't match"
> > > 			" supported offloads 0x%" PRIx64 "\n",
> > > 			(void *)dev, rx_conf->offloads,
> > > 			 tap_rx_offload_get_queue_capa()));
> > >
> > >
> > >>  	rxq->mp = mp;
> > >>  	rxq->trigger_seen = 1; /* force initial burst */
> > >>  	rxq->in_port = dev->data->port_id; @@ -1175,29 +1110,12 @@
> > >> tap_tx_queue_setup(struct rte_eth_dev
> > *dev,
> > >>  		return -1;
> > >>  	dev->data->tx_queues[tx_queue_id] = &internals->txq[tx_queue_id];
> > >>  	txq = dev->data->tx_queues[tx_queue_id];
> > >> -	/*
> > >> -	 * Don't verify port offloads for application which
> > >> -	 * use the old API.
> > >> -	 */
> > >> -	if (tx_conf != NULL &&
> > >> -	    !!(tx_conf->txq_flags & ETH_TXQ_FLAGS_IGNORE)) {
> > >> -		if (tap_txq_are_offloads_valid(dev, tx_conf->offloads)) {
> > >> -			txq->csum = !!(tx_conf->offloads &
> > >> -					(DEV_TX_OFFLOAD_IPV4_CKSUM |
> > >> -					 DEV_TX_OFFLOAD_UDP_CKSUM |
> > >> -					 DEV_TX_OFFLOAD_TCP_CKSUM));
> > >> -		} else {
> > >> -			rte_errno = ENOTSUP;
> > >> -			RTE_LOG(ERR, PMD,
> > >> -				"%p: Tx queue offloads 0x%" PRIx64
> > >> -				" don't match port offloads 0x%" PRIx64
> > >> -				" or supported offloads 0x%" PRIx64,
> > >> -				(void *)dev, tx_conf->offloads,
> > >> -				dev->data->dev_conf.txmode.offloads,
> > >> -				tap_tx_offload_get_port_capa());
> > >> -			return -rte_errno;
> > >> -		}
> > >> -	}
> > >> +
> > >
> > > The tap_txq_are_offloads_valid() call is essential. I suggest to
> > > leave it in
> > place.
> > > The RTE_LOG message could drop comparison between queue and port
> > capabilities:
> > >
> > > 			RTE_LOG(ERR, PMD,
> > > 				"%p: Tx queue offloads 0x%" PRIx64
> > > 				" don't match"
> > > 				" supported offloads 0x%" PRIx64,
> > > 				(void *)dev, tx_conf->offloads,
> > > 				tap_tx_offload_get_queue_capa());
> > >
> > >> +	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;
> > >> --
> > >> 2.14.3
> > >

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

[Ophir Munk]

Hi Ferruh,
Patch https://dpdk.org/dev/patchwork/patch/38957/ was submitted.
Can you please review it? 
Please add Suggest-by with your name.

Regards,
Ophir

> -----Original Message-----
> From: Ophir Munk
> Sent: Wednesday, April 25, 2018 3:21 PM
> To: 'Ferruh Yigit' <ferruh.yigit@intel.com>; 'Pascal Mazon'
> <pascal.mazon@6wind.com>
> Cc: 'dev@dpdk.org' <dev@dpdk.org>; Mordechay Haimovsky
> <motih@mellanox.com>; Olga Shern <olgas@mellanox.com>; Thomas
> Monjalon <thomas@monjalon.net>; Raslan Darawsheh
> <rasland@mellanox.com>; Shahaf Shuler <shahafs@mellanox.com>
> Subject: RE: [PATCH v3] net/tap: remove queue specific offload support
> 
> Hi Ferruh,
> I started working on a patch.
> No need for your test example.
> 
> > -----Original Message-----
> > From: Ophir Munk
> > Sent: Wednesday, April 25, 2018 3:00 PM
> > To: 'Ferruh Yigit' <ferruh.yigit@intel.com>; Pascal Mazon
> > <pascal.mazon@6wind.com>
> > Cc: dev@dpdk.org; Mordechay Haimovsky <motih@mellanox.com>; Olga
> Shern
> > <olgas@mellanox.com>; Thomas Monjalon <thomas@monjalon.net>;
> Raslan
> > Darawsheh <rasland@mellanox.com>; Shahaf Shuler
> <shahafs@mellanox.com>
> > Subject: RE: [PATCH v3] net/tap: remove queue specific offload support
> >
> >
> >
> > > -----Original Message-----
> > > From: Ferruh Yigit [mailto:ferruh.yigit@intel.com]
> > > Sent: Wednesday, April 25, 2018 12:48 PM
> > > To: Ophir Munk <ophirmu@mellanox.com>; Pascal Mazon
> > > <pascal.mazon@6wind.com>
> > > Cc: dev@dpdk.org; Mordechay Haimovsky <motih@mellanox.com>; Olga
> > Shern
> > > <olgas@mellanox.com>; Thomas Monjalon <thomas@monjalon.net>;
> > Raslan
> > > Darawsheh <rasland@mellanox.com>; Shahaf Shuler
> > <shahafs@mellanox.com>
> > > Subject: Re: [PATCH v3] net/tap: remove queue specific offload
> > > support
> > >
> > > On 4/25/2018 10:18 AM, Ophir Munk wrote:
> > > > Hi Ferruh,
> > > >
> > > > I should have mentioned earlier that TAP does support queue
> > > > specific
> > > capabilities.
> > > > Please look in tap_queue_setup() and note that each TAP queue is
> > > > created
> > > with a distinct file descriptor (fd).
> > > > Then supporting an offload capability is just implementing it in SW (e.g.
> > > calculating IP checksum).
> > > >
> > > > If the main assumption of this patch was that TAP does not support
> > > > queue
> > > specific offloads - then please consider this patch again.
> > >
> > > Yes that was the initial question, is tap supports queue specific
> > > offloads or not. Thanks for the answer.
> > >
> > > >
> > > > On the other hand there is no port specific capability supported by TAP.
> > >
> > > If so verify functions are wrong, that was the error I got.
> >
> > Can you please specify the test you did what error you got?
> > If I fix something I want to verify what I am fixing.
> >
> > > It seems copy/paste of mlx one but the port_supp_offloads has
> > > different meaning there.
> > >
> > > > However, in order to support legacy applications, port
> > > > capabilities are
> > > usually reported as the OR operation between queue & port capabilities.
> > > > TAP currently clones the queue capabilities to port capabilities.
> > > > We could
> > > optimize this cloning by always return queue capabilities when
> > > queried about queues or ports. In this case -
> > > tap_rx_offload_get_port_capa() and
> > > tap_tx_offload_get_port_capa() could be removed.
> > >
> > > Instead of removing the functions I think you can keep them but
> > > return correct values, in this case return empty, this will make the
> > > exiting validation functions correct.
> > >
> > > Can you send a fix for that?
> > > If no fix sent, I suggest going with this patch to remove queue
> > > level offload support until it is fixed.
> > >
> > > >
> > > > Please find more comments inline.
> > > >
> > > >> -----Original Message-----
> > > >> From: Ferruh Yigit [mailto:ferruh.yigit@intel.com]
> > > >> Sent: Tuesday, April 24, 2018 8:54 PM
> > > >> To: Pascal Mazon <pascal.mazon@6wind.com>
> > > >> Cc: dev@dpdk.org; Ferruh Yigit <ferruh.yigit@intel.com>;
> > > >> Mordechay Haimovsky <motih@mellanox.com>; Ophir Munk
> > > <ophirmu@mellanox.com>
> > > >> Subject: [PATCH v3] net/tap: remove queue specific offload
> > > >> support
> > > >>
> > > >> 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>
> > > >> ---
> > > >> Cc: Ophir Munk <ophirmu@mellanox.com>
> > > >>
> > > >> v2:
> > > >> * rebased
> > > >>
> > > >> v3:
> > > >> * txq->csum restored,
> > > >>   - ETH_TXQ_FLAGS_IGNORE check removed since ethdev layer takes
> > > >> care of it
> > > >>   - tx_conf != NULL check removed, this is internal api who calls this is
> > > >>   ethdev and it doesn't pass null tx_conf
> > > >> ---
> > > >>  drivers/net/tap/rte_eth_tap.c | 102
> > > >> +++++-------------------------------------
> > > >>  1 file changed, 10 insertions(+), 92 deletions(-)
> > > >>
> > > >> diff --git a/drivers/net/tap/rte_eth_tap.c
> > > >> b/drivers/net/tap/rte_eth_tap.c index ef33aace9..61b4b5df3 100644
> > > >> --- a/drivers/net/tap/rte_eth_tap.c
> > > >> +++ b/drivers/net/tap/rte_eth_tap.c
> > > >> @@ -278,31 +278,6 @@ tap_rx_offload_get_port_capa(void)
> > > >>  	       DEV_RX_OFFLOAD_CRC_STRIP;  }
> > > >>
> > > >> -static uint64_t
> > > >> -tap_rx_offload_get_queue_capa(void)
> > > >> -{
> > > >> -	return DEV_RX_OFFLOAD_SCATTER |
> > > >> -	       DEV_RX_OFFLOAD_IPV4_CKSUM |
> > > >> -	       DEV_RX_OFFLOAD_UDP_CKSUM |
> > > >> -	       DEV_RX_OFFLOAD_TCP_CKSUM |
> > > >> -	       DEV_RX_OFFLOAD_CRC_STRIP;
> > > >> -}
> > > >> -
> > > >
> > > > TAP PMD supports all of these RX queue specific offloads. I
> > > > suggest to
> > > leave this function in place.
> > > >
> > > >> -static bool
> > > >> -tap_rxq_are_offloads_valid(struct rte_eth_dev *dev, uint64_t
> > > >> offloads) -
> > > {
> > > >> -	uint64_t port_offloads = dev->data->dev_conf.rxmode.offloads;
> > > >> -	uint64_t queue_supp_offloads = tap_rx_offload_get_queue_capa();
> > > >> -	uint64_t port_supp_offloads = tap_rx_offload_get_port_capa();
> > > >> -
> > > >> -	if ((offloads & (queue_supp_offloads | port_supp_offloads)) !=
> > > >> -	    offloads)
> > > >> -		return false;
> > > >> -	if ((port_offloads ^ offloads) & port_supp_offloads)
> > > >> -		return false;
> > > >> -	return true;
> > > >> -}
> > > >> -
> > > >
> > > > Putting aside the fact that queue offloads equals port offloads
> > > > (so could
> > > ignore "port_supp_offload" variable) - this function is essential to
> > > validate that the configured Rx offloads are supported by TAP. I
> > > suggest to leave this function in place.
> > > > Without it - testpmd falsely confirms non supported offloads.
> > > > For example before this patch: offloading "hw-vlan-filter" will
> > > > fail as
> > > expected:
> > > >
> > > > testpmd> port config all
> > > > testpmd> port config all hw-vlan-filter on port start all
> > > > Configuring Port 0 (socket 0)
> > > > PMD: net_tap0: 0x1209fc0: TX configured queues number: 1
> > > > PMD: net_tap0: 0x1209fc0: RX configured queues number: 1
> > > > PMD: 0x1209fc0: Rx queue offloads 0x120e don't match port offloads
> > > > 0x120e or supported offloads 0x300e Fail to configure port 0 rx
> > > > queues
> > > >
> > > > However, with this patch this configuration is falsely accepted.
> > > >
> > > >>  /* Callback to handle the rx burst of packets to the correct
> > > >> interface
> > and
> > > >>   * file descriptor(s) in a multi-queue setup.
> > > >>   */
> > > >> @@ -411,31 +386,6 @@ tap_tx_offload_get_port_capa(void)
> > > >>  	       DEV_TX_OFFLOAD_TCP_CKSUM;  }
> > > >>
> > > >> -static uint64_t
> > > >> -tap_tx_offload_get_queue_capa(void)
> > > >> -{
> > > >> -	return DEV_TX_OFFLOAD_MULTI_SEGS |
> > > >> -	       DEV_TX_OFFLOAD_IPV4_CKSUM |
> > > >> -	       DEV_TX_OFFLOAD_UDP_CKSUM |
> > > >> -	       DEV_TX_OFFLOAD_TCP_CKSUM;
> > > >> -}
> > > >> -
> > > >
> > > > TAP PMD supports all of these TX queue specific offloads. I
> > > > suggest to
> > > leave this function in place.
> > > >
> > > >> -static bool
> > > >> -tap_txq_are_offloads_valid(struct rte_eth_dev *dev, uint64_t
> > > >> offloads) -
> > > {
> > > >> -	uint64_t port_offloads = dev->data->dev_conf.txmode.offloads;
> > > >> -	uint64_t queue_supp_offloads = tap_tx_offload_get_queue_capa();
> > > >> -	uint64_t port_supp_offloads = tap_tx_offload_get_port_capa();
> > > >> -
> > > >> -	if ((offloads & (queue_supp_offloads | port_supp_offloads)) !=
> > > >> -	    offloads)
> > > >> -		return false;
> > > >> -	/* Verify we have no conflict with port offloads */
> > > >> -	if ((port_offloads ^ offloads) & port_supp_offloads)
> > > >> -		return false;
> > > >> -	return true;
> > > >> -}
> > > >> -
> > > >
> > > > This function is essential to validate that the configured Tx
> > > > offloads are
> > > supported by TAP.
> > > > I suggest to leave this function in place.
> > > >
> > > >>  static void
> > > >>  tap_tx_offload(char *packet, uint64_t ol_flags, unsigned int l2_len,
> > > >>  	       unsigned int l3_len)
> > > >> @@ -763,12 +713,10 @@ tap_dev_info(struct rte_eth_dev *dev,
> > > >> struct rte_eth_dev_info *dev_info)
> > > >>  	dev_info->max_tx_queues = RTE_PMD_TAP_MAX_QUEUES;
> > > >>  	dev_info->min_rx_bufsize = 0;
> > > >>  	dev_info->speed_capa = tap_dev_speed_capa();
> > > >> -	dev_info->rx_queue_offload_capa =
> > > >> tap_rx_offload_get_queue_capa();
> > > >> -	dev_info->rx_offload_capa = tap_rx_offload_get_port_capa() |
> > > >> -				    dev_info->rx_queue_offload_capa;
> > > >> -	dev_info->tx_queue_offload_capa =
> > > >> tap_tx_offload_get_queue_capa();
> > > >> -	dev_info->tx_offload_capa = tap_tx_offload_get_port_capa() |
> > > >> -				    dev_info->tx_queue_offload_capa;
> > > >> +	dev_info->rx_offload_capa =
> tap_rx_offload_get_port_capa();
> > > >> +	dev_info->tx_offload_capa =
> tap_tx_offload_get_port_capa();
> > > >> +	dev_info->rx_queue_offload_capa = 0;
> > > >> +	dev_info->tx_queue_offload_capa = 0;
> > > >>  }
> > > >>
> > > >
> > > > Rx_queue_offloads_capa should be reported as before:
> > > > dev_info->tx_queue_offload_capa =
> tap_tx_offload_get_queue_capa();
> > > > Same for TX offloads.
> > > >
> > > > Port capabilities could return queue capabilities:
> > > >
> > > > Instead of:
> > > >
> > > > dev_info->rx_offload_capa = tap_rx_offload_get_port_capa() |
> > > > 				    dev_info->rx_queue_offload_capa;
> > > >
> > > > We could return:
> > > >
> > > > dev_info->rx_offload_capa = dev_info->rx_queue_offload_capa;
> > > >
> > > > The same argument is valid for Tx as well.
> > > >
> > > >>  static int
> > > >> @@ -1094,19 +1042,6 @@ tap_rx_queue_setup(struct rte_eth_dev
> > *dev,
> > > >>  		return -1;
> > > >>  	}
> > > >>
> > > >> -	/* Verify application offloads are valid for our port and queue. */
> > > >> -	if (!tap_rxq_are_offloads_valid(dev, rx_conf->offloads)) {
> > > >> -		rte_errno = ENOTSUP;
> > > >> -		RTE_LOG(ERR, PMD,
> > > >> -			"%p: Rx queue offloads 0x%" PRIx64
> > > >> -			" don't match port offloads 0x%" PRIx64
> > > >> -			" or supported offloads 0x%" PRIx64 "\n",
> > > >> -			(void *)dev, rx_conf->offloads,
> > > >> -			dev->data->dev_conf.rxmode.offloads,
> > > >> -			(tap_rx_offload_get_port_capa() |
> > > >> -			 tap_rx_offload_get_queue_capa()));
> > > >> -		return -rte_errno;
> > > >> -	}
> > > >
> > > > The tap_rxq_are_offloads_valid() call is essential. I suggest to
> > > > leave it in
> > > place.
> > > > The RTE_LOG could drop port references to become:
> > > >
> > > > 		RTE_LOG(ERR, PMD,
> > > > 			"%p: Rx queue offloads 0x%" PRIx64
> > > > 			" don't match"
> > > > 			" supported offloads 0x%" PRIx64 "\n",
> > > > 			(void *)dev, rx_conf->offloads,
> > > > 			 tap_rx_offload_get_queue_capa()));
> > > >
> > > >
> > > >>  	rxq->mp = mp;
> > > >>  	rxq->trigger_seen = 1; /* force initial burst */
> > > >>  	rxq->in_port = dev->data->port_id; @@ -1175,29 +1110,12 @@
> > > >> tap_tx_queue_setup(struct rte_eth_dev
> > > *dev,
> > > >>  		return -1;
> > > >>  	dev->data->tx_queues[tx_queue_id] = &internals->txq[tx_queue_id];
> > > >>  	txq = dev->data->tx_queues[tx_queue_id];
> > > >> -	/*
> > > >> -	 * Don't verify port offloads for application which
> > > >> -	 * use the old API.
> > > >> -	 */
> > > >> -	if (tx_conf != NULL &&
> > > >> -	    !!(tx_conf->txq_flags & ETH_TXQ_FLAGS_IGNORE)) {
> > > >> -		if (tap_txq_are_offloads_valid(dev, tx_conf->offloads)) {
> > > >> -			txq->csum = !!(tx_conf->offloads &
> > > >> -					(DEV_TX_OFFLOAD_IPV4_CKSUM |
> > > >> -					 DEV_TX_OFFLOAD_UDP_CKSUM |
> > > >> -					 DEV_TX_OFFLOAD_TCP_CKSUM));
> > > >> -		} else {
> > > >> -			rte_errno = ENOTSUP;
> > > >> -			RTE_LOG(ERR, PMD,
> > > >> -				"%p: Tx queue offloads 0x%" PRIx64
> > > >> -				" don't match port offloads 0x%" PRIx64
> > > >> -				" or supported offloads 0x%" PRIx64,
> > > >> -				(void *)dev, tx_conf->offloads,
> > > >> -				dev->data->dev_conf.txmode.offloads,
> > > >> -				tap_tx_offload_get_port_capa());
> > > >> -			return -rte_errno;
> > > >> -		}
> > > >> -	}
> > > >> +
> > > >
> > > > The tap_txq_are_offloads_valid() call is essential. I suggest to
> > > > leave it in
> > > place.
> > > > The RTE_LOG message could drop comparison between queue and port
> > > capabilities:
> > > >
> > > > 			RTE_LOG(ERR, PMD,
> > > > 				"%p: Tx queue offloads 0x%" PRIx64
> > > > 				" don't match"
> > > > 				" supported offloads 0x%" PRIx64,
> > > > 				(void *)dev, tx_conf->offloads,
> > > > 				tap_tx_offload_get_queue_capa());
> > > >
> > > >> +	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;
> > > >> --
> > > >> 2.14.3
> > > >

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

[Ferruh Yigit]

On 4/25/2018 5:17 PM, Ophir Munk wrote:
> Hi Ferruh,
> Patch https://dpdk.org/dev/patchwork/patch/38957/ was submitted.

Thanks Ophir,

Since your patch is out, I am marking this one as rejected.

> Can you please review it? 
> Please add Suggest-by with your name.
> 
> Regards,
> Ophir
> 
>> -----Original Message-----
>> From: Ophir Munk
>> Sent: Wednesday, April 25, 2018 3:21 PM
>> To: 'Ferruh Yigit' <ferruh.yigit@intel.com>; 'Pascal Mazon'
>> <pascal.mazon@6wind.com>
>> Cc: 'dev@dpdk.org' <dev@dpdk.org>; Mordechay Haimovsky
>> <motih@mellanox.com>; Olga Shern <olgas@mellanox.com>; Thomas
>> Monjalon <thomas@monjalon.net>; Raslan Darawsheh
>> <rasland@mellanox.com>; Shahaf Shuler <shahafs@mellanox.com>
>> Subject: RE: [PATCH v3] net/tap: remove queue specific offload support
>>
>> Hi Ferruh,
>> I started working on a patch.
>> No need for your test example.
>>
>>> -----Original Message-----
>>> From: Ophir Munk
>>> Sent: Wednesday, April 25, 2018 3:00 PM
>>> To: 'Ferruh Yigit' <ferruh.yigit@intel.com>; Pascal Mazon
>>> <pascal.mazon@6wind.com>
>>> Cc: dev@dpdk.org; Mordechay Haimovsky <motih@mellanox.com>; Olga
>> Shern
>>> <olgas@mellanox.com>; Thomas Monjalon <thomas@monjalon.net>;
>> Raslan
>>> Darawsheh <rasland@mellanox.com>; Shahaf Shuler
>> <shahafs@mellanox.com>
>>> Subject: RE: [PATCH v3] net/tap: remove queue specific offload support
>>>
>>>
>>>
>>>> -----Original Message-----
>>>> From: Ferruh Yigit [mailto:ferruh.yigit@intel.com]
>>>> Sent: Wednesday, April 25, 2018 12:48 PM
>>>> To: Ophir Munk <ophirmu@mellanox.com>; Pascal Mazon
>>>> <pascal.mazon@6wind.com>
>>>> Cc: dev@dpdk.org; Mordechay Haimovsky <motih@mellanox.com>; Olga
>>> Shern
>>>> <olgas@mellanox.com>; Thomas Monjalon <thomas@monjalon.net>;
>>> Raslan
>>>> Darawsheh <rasland@mellanox.com>; Shahaf Shuler
>>> <shahafs@mellanox.com>
>>>> Subject: Re: [PATCH v3] net/tap: remove queue specific offload
>>>> support
>>>>
>>>> On 4/25/2018 10:18 AM, Ophir Munk wrote:
>>>>> Hi Ferruh,
>>>>>
>>>>> I should have mentioned earlier that TAP does support queue
>>>>> specific
>>>> capabilities.
>>>>> Please look in tap_queue_setup() and note that each TAP queue is
>>>>> created
>>>> with a distinct file descriptor (fd).
>>>>> Then supporting an offload capability is just implementing it in SW (e.g.
>>>> calculating IP checksum).
>>>>>
>>>>> If the main assumption of this patch was that TAP does not support
>>>>> queue
>>>> specific offloads - then please consider this patch again.
>>>>
>>>> Yes that was the initial question, is tap supports queue specific
>>>> offloads or not. Thanks for the answer.
>>>>
>>>>>
>>>>> On the other hand there is no port specific capability supported by TAP.
>>>>
>>>> If so verify functions are wrong, that was the error I got.
>>>
>>> Can you please specify the test you did what error you got?
>>> If I fix something I want to verify what I am fixing.
>>>
>>>> It seems copy/paste of mlx one but the port_supp_offloads has
>>>> different meaning there.
>>>>
>>>>> However, in order to support legacy applications, port
>>>>> capabilities are
>>>> usually reported as the OR operation between queue & port capabilities.
>>>>> TAP currently clones the queue capabilities to port capabilities.
>>>>> We could
>>>> optimize this cloning by always return queue capabilities when
>>>> queried about queues or ports. In this case -
>>>> tap_rx_offload_get_port_capa() and
>>>> tap_tx_offload_get_port_capa() could be removed.
>>>>
>>>> Instead of removing the functions I think you can keep them but
>>>> return correct values, in this case return empty, this will make the
>>>> exiting validation functions correct.
>>>>
>>>> Can you send a fix for that?
>>>> If no fix sent, I suggest going with this patch to remove queue
>>>> level offload support until it is fixed.
>>>>
>>>>>
>>>>> Please find more comments inline.
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Ferruh Yigit [mailto:ferruh.yigit@intel.com]
>>>>>> Sent: Tuesday, April 24, 2018 8:54 PM
>>>>>> To: Pascal Mazon <pascal.mazon@6wind.com>
>>>>>> Cc: dev@dpdk.org; Ferruh Yigit <ferruh.yigit@intel.com>;
>>>>>> Mordechay Haimovsky <motih@mellanox.com>; Ophir Munk
>>>> <ophirmu@mellanox.com>
>>>>>> Subject: [PATCH v3] net/tap: remove queue specific offload
>>>>>> support
>>>>>>
>>>>>> 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>
>>>>>> ---
>>>>>> Cc: Ophir Munk <ophirmu@mellanox.com>
>>>>>>
>>>>>> v2:
>>>>>> * rebased
>>>>>>
>>>>>> v3:
>>>>>> * txq->csum restored,
>>>>>>   - ETH_TXQ_FLAGS_IGNORE check removed since ethdev layer takes
>>>>>> care of it
>>>>>>   - tx_conf != NULL check removed, this is internal api who calls this is
>>>>>>   ethdev and it doesn't pass null tx_conf
>>>>>> ---
>>>>>>  drivers/net/tap/rte_eth_tap.c | 102
>>>>>> +++++-------------------------------------
>>>>>>  1 file changed, 10 insertions(+), 92 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/net/tap/rte_eth_tap.c
>>>>>> b/drivers/net/tap/rte_eth_tap.c index ef33aace9..61b4b5df3 100644
>>>>>> --- a/drivers/net/tap/rte_eth_tap.c
>>>>>> +++ b/drivers/net/tap/rte_eth_tap.c
>>>>>> @@ -278,31 +278,6 @@ tap_rx_offload_get_port_capa(void)
>>>>>>  	       DEV_RX_OFFLOAD_CRC_STRIP;  }
>>>>>>
>>>>>> -static uint64_t
>>>>>> -tap_rx_offload_get_queue_capa(void)
>>>>>> -{
>>>>>> -	return DEV_RX_OFFLOAD_SCATTER |
>>>>>> -	       DEV_RX_OFFLOAD_IPV4_CKSUM |
>>>>>> -	       DEV_RX_OFFLOAD_UDP_CKSUM |
>>>>>> -	       DEV_RX_OFFLOAD_TCP_CKSUM |
>>>>>> -	       DEV_RX_OFFLOAD_CRC_STRIP;
>>>>>> -}
>>>>>> -
>>>>>
>>>>> TAP PMD supports all of these RX queue specific offloads. I
>>>>> suggest to
>>>> leave this function in place.
>>>>>
>>>>>> -static bool
>>>>>> -tap_rxq_are_offloads_valid(struct rte_eth_dev *dev, uint64_t
>>>>>> offloads) -
>>>> {
>>>>>> -	uint64_t port_offloads = dev->data->dev_conf.rxmode.offloads;
>>>>>> -	uint64_t queue_supp_offloads = tap_rx_offload_get_queue_capa();
>>>>>> -	uint64_t port_supp_offloads = tap_rx_offload_get_port_capa();
>>>>>> -
>>>>>> -	if ((offloads & (queue_supp_offloads | port_supp_offloads)) !=
>>>>>> -	    offloads)
>>>>>> -		return false;
>>>>>> -	if ((port_offloads ^ offloads) & port_supp_offloads)
>>>>>> -		return false;
>>>>>> -	return true;
>>>>>> -}
>>>>>> -
>>>>>
>>>>> Putting aside the fact that queue offloads equals port offloads
>>>>> (so could
>>>> ignore "port_supp_offload" variable) - this function is essential to
>>>> validate that the configured Rx offloads are supported by TAP. I
>>>> suggest to leave this function in place.
>>>>> Without it - testpmd falsely confirms non supported offloads.
>>>>> For example before this patch: offloading "hw-vlan-filter" will
>>>>> fail as
>>>> expected:
>>>>>
>>>>> testpmd> port config all
>>>>> testpmd> port config all hw-vlan-filter on port start all
>>>>> Configuring Port 0 (socket 0)
>>>>> PMD: net_tap0: 0x1209fc0: TX configured queues number: 1
>>>>> PMD: net_tap0: 0x1209fc0: RX configured queues number: 1
>>>>> PMD: 0x1209fc0: Rx queue offloads 0x120e don't match port offloads
>>>>> 0x120e or supported offloads 0x300e Fail to configure port 0 rx
>>>>> queues
>>>>>
>>>>> However, with this patch this configuration is falsely accepted.
>>>>>
>>>>>>  /* Callback to handle the rx burst of packets to the correct
>>>>>> interface
>>> and
>>>>>>   * file descriptor(s) in a multi-queue setup.
>>>>>>   */
>>>>>> @@ -411,31 +386,6 @@ tap_tx_offload_get_port_capa(void)
>>>>>>  	       DEV_TX_OFFLOAD_TCP_CKSUM;  }
>>>>>>
>>>>>> -static uint64_t
>>>>>> -tap_tx_offload_get_queue_capa(void)
>>>>>> -{
>>>>>> -	return DEV_TX_OFFLOAD_MULTI_SEGS |
>>>>>> -	       DEV_TX_OFFLOAD_IPV4_CKSUM |
>>>>>> -	       DEV_TX_OFFLOAD_UDP_CKSUM |
>>>>>> -	       DEV_TX_OFFLOAD_TCP_CKSUM;
>>>>>> -}
>>>>>> -
>>>>>
>>>>> TAP PMD supports all of these TX queue specific offloads. I
>>>>> suggest to
>>>> leave this function in place.
>>>>>
>>>>>> -static bool
>>>>>> -tap_txq_are_offloads_valid(struct rte_eth_dev *dev, uint64_t
>>>>>> offloads) -
>>>> {
>>>>>> -	uint64_t port_offloads = dev->data->dev_conf.txmode.offloads;
>>>>>> -	uint64_t queue_supp_offloads = tap_tx_offload_get_queue_capa();
>>>>>> -	uint64_t port_supp_offloads = tap_tx_offload_get_port_capa();
>>>>>> -
>>>>>> -	if ((offloads & (queue_supp_offloads | port_supp_offloads)) !=
>>>>>> -	    offloads)
>>>>>> -		return false;
>>>>>> -	/* Verify we have no conflict with port offloads */
>>>>>> -	if ((port_offloads ^ offloads) & port_supp_offloads)
>>>>>> -		return false;
>>>>>> -	return true;
>>>>>> -}
>>>>>> -
>>>>>
>>>>> This function is essential to validate that the configured Tx
>>>>> offloads are
>>>> supported by TAP.
>>>>> I suggest to leave this function in place.
>>>>>
>>>>>>  static void
>>>>>>  tap_tx_offload(char *packet, uint64_t ol_flags, unsigned int l2_len,
>>>>>>  	       unsigned int l3_len)
>>>>>> @@ -763,12 +713,10 @@ tap_dev_info(struct rte_eth_dev *dev,
>>>>>> struct rte_eth_dev_info *dev_info)
>>>>>>  	dev_info->max_tx_queues = RTE_PMD_TAP_MAX_QUEUES;
>>>>>>  	dev_info->min_rx_bufsize = 0;
>>>>>>  	dev_info->speed_capa = tap_dev_speed_capa();
>>>>>> -	dev_info->rx_queue_offload_capa =
>>>>>> tap_rx_offload_get_queue_capa();
>>>>>> -	dev_info->rx_offload_capa = tap_rx_offload_get_port_capa() |
>>>>>> -				    dev_info->rx_queue_offload_capa;
>>>>>> -	dev_info->tx_queue_offload_capa =
>>>>>> tap_tx_offload_get_queue_capa();
>>>>>> -	dev_info->tx_offload_capa = tap_tx_offload_get_port_capa() |
>>>>>> -				    dev_info->tx_queue_offload_capa;
>>>>>> +	dev_info->rx_offload_capa =
>> tap_rx_offload_get_port_capa();
>>>>>> +	dev_info->tx_offload_capa =
>> tap_tx_offload_get_port_capa();
>>>>>> +	dev_info->rx_queue_offload_capa = 0;
>>>>>> +	dev_info->tx_queue_offload_capa = 0;
>>>>>>  }
>>>>>>
>>>>>
>>>>> Rx_queue_offloads_capa should be reported as before:
>>>>> dev_info->tx_queue_offload_capa =
>> tap_tx_offload_get_queue_capa();
>>>>> Same for TX offloads.
>>>>>
>>>>> Port capabilities could return queue capabilities:
>>>>>
>>>>> Instead of:
>>>>>
>>>>> dev_info->rx_offload_capa = tap_rx_offload_get_port_capa() |
>>>>> 				    dev_info->rx_queue_offload_capa;
>>>>>
>>>>> We could return:
>>>>>
>>>>> dev_info->rx_offload_capa = dev_info->rx_queue_offload_capa;
>>>>>
>>>>> The same argument is valid for Tx as well.
>>>>>
>>>>>>  static int
>>>>>> @@ -1094,19 +1042,6 @@ tap_rx_queue_setup(struct rte_eth_dev
>>> *dev,
>>>>>>  		return -1;
>>>>>>  	}
>>>>>>
>>>>>> -	/* Verify application offloads are valid for our port and queue. */
>>>>>> -	if (!tap_rxq_are_offloads_valid(dev, rx_conf->offloads)) {
>>>>>> -		rte_errno = ENOTSUP;
>>>>>> -		RTE_LOG(ERR, PMD,
>>>>>> -			"%p: Rx queue offloads 0x%" PRIx64
>>>>>> -			" don't match port offloads 0x%" PRIx64
>>>>>> -			" or supported offloads 0x%" PRIx64 "\n",
>>>>>> -			(void *)dev, rx_conf->offloads,
>>>>>> -			dev->data->dev_conf.rxmode.offloads,
>>>>>> -			(tap_rx_offload_get_port_capa() |
>>>>>> -			 tap_rx_offload_get_queue_capa()));
>>>>>> -		return -rte_errno;
>>>>>> -	}
>>>>>
>>>>> The tap_rxq_are_offloads_valid() call is essential. I suggest to
>>>>> leave it in
>>>> place.
>>>>> The RTE_LOG could drop port references to become:
>>>>>
>>>>> 		RTE_LOG(ERR, PMD,
>>>>> 			"%p: Rx queue offloads 0x%" PRIx64
>>>>> 			" don't match"
>>>>> 			" supported offloads 0x%" PRIx64 "\n",
>>>>> 			(void *)dev, rx_conf->offloads,
>>>>> 			 tap_rx_offload_get_queue_capa()));
>>>>>
>>>>>
>>>>>>  	rxq->mp = mp;
>>>>>>  	rxq->trigger_seen = 1; /* force initial burst */
>>>>>>  	rxq->in_port = dev->data->port_id; @@ -1175,29 +1110,12 @@
>>>>>> tap_tx_queue_setup(struct rte_eth_dev
>>>> *dev,
>>>>>>  		return -1;
>>>>>>  	dev->data->tx_queues[tx_queue_id] = &internals->txq[tx_queue_id];
>>>>>>  	txq = dev->data->tx_queues[tx_queue_id];
>>>>>> -	/*
>>>>>> -	 * Don't verify port offloads for application which
>>>>>> -	 * use the old API.
>>>>>> -	 */
>>>>>> -	if (tx_conf != NULL &&
>>>>>> -	    !!(tx_conf->txq_flags & ETH_TXQ_FLAGS_IGNORE)) {
>>>>>> -		if (tap_txq_are_offloads_valid(dev, tx_conf->offloads)) {
>>>>>> -			txq->csum = !!(tx_conf->offloads &
>>>>>> -					(DEV_TX_OFFLOAD_IPV4_CKSUM |
>>>>>> -					 DEV_TX_OFFLOAD_UDP_CKSUM |
>>>>>> -					 DEV_TX_OFFLOAD_TCP_CKSUM));
>>>>>> -		} else {
>>>>>> -			rte_errno = ENOTSUP;
>>>>>> -			RTE_LOG(ERR, PMD,
>>>>>> -				"%p: Tx queue offloads 0x%" PRIx64
>>>>>> -				" don't match port offloads 0x%" PRIx64
>>>>>> -				" or supported offloads 0x%" PRIx64,
>>>>>> -				(void *)dev, tx_conf->offloads,
>>>>>> -				dev->data->dev_conf.txmode.offloads,
>>>>>> -				tap_tx_offload_get_port_capa());
>>>>>> -			return -rte_errno;
>>>>>> -		}
>>>>>> -	}
>>>>>> +
>>>>>
>>>>> The tap_txq_are_offloads_valid() call is essential. I suggest to
>>>>> leave it in
>>>> place.
>>>>> The RTE_LOG message could drop comparison between queue and port
>>>> capabilities:
>>>>>
>>>>> 			RTE_LOG(ERR, PMD,
>>>>> 				"%p: Tx queue offloads 0x%" PRIx64
>>>>> 				" don't match"
>>>>> 				" supported offloads 0x%" PRIx64,
>>>>> 				(void *)dev, tx_conf->offloads,
>>>>> 				tap_tx_offload_get_queue_capa());
>>>>>
>>>>>> +	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;
>>>>>> --
>>>>>> 2.14.3
>>>>>
>