[dpdk-dev,v3] net/tap: remove queue specific offload support
Checks
Commit Message
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(-)
Comments
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
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
>
> -----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
> >
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
> > >
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
> > > >
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
>>>>>
>
@@ -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;