[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 at intel.com]
> Sent: Tuesday, April 24, 2018 8:54 PM
> To: Pascal Mazon <pascal.mazon at 6wind.com>
> Cc: dev at dpdk.org; Ferruh Yigit <ferruh.yigit at intel.com>; Mordechay
> Haimovsky <motih at mellanox.com>; Ophir Munk <ophirmu at 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 at mellanox.com
> 
> Signed-off-by: Ferruh Yigit <ferruh.yigit at intel.com>
> ---
> Cc: Ophir Munk <ophirmu at 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