[dpdk-dev,V5,2/2] net/tap: use new Rx offloads API
Checks
Commit Message
Ethdev Rx offloads API has changed since:
commit ce17eddefc20 ("ethdev: introduce Rx queue offloads API")
This commit adds support for the new Rx offloads API.
Signed-off-by: Moti Haimovsky <motih@mellanox.com>
---
V5:
* Fixed compilation errors caused by not using PRIx64 in log messages
when displaying uint64_t values.
* Modified patch name
V4:
Modifications according to inputs from Pascal Mazon
* Removed extra braces.
V3:
* Fixed coding style warnings
V2:
* Fixed coding style warnings
---
drivers/net/tap/rte_eth_tap.c | 67 +++++++++++++++++++++++++++++++++++++------
1 file changed, 59 insertions(+), 8 deletions(-)
Comments
On 1/17/2018 2:04 PM, Moti Haimovsky wrote:
> Ethdev Rx offloads API has changed since:
> commit ce17eddefc20 ("ethdev: introduce Rx queue offloads API")
> This commit adds support for the new Rx offloads API.
>
> Signed-off-by: Moti Haimovsky <motih@mellanox.com>
<...>
> +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;
Hi Moti,
I am getting following error when tried to use tap with bonding:
"Rx queue offloads 0x0 don't match port offloads 0x1000 or supported offloads
0x300e"
What is the intention here? I guess it tries to be sure requested queue offloads
is subsets of port_offloads and offload_capability.
If so not requesting any queue offload should be valid, isn't it?
<...>
--Shahaf
> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ferruh Yigit
> Sent: Friday, March 2, 2018 11:44 PM
> To: Mordechay Haimovsky <motih@mellanox.com>;
> pascal.mazon@6wind.com
> Cc: dev@dpdk.org; Shahaf Shuler <shahafs@mellanox.com>
> Subject: Re: [dpdk-dev] [PATCH V5 2/2] net/tap: use new Rx offloads API
>
> On 1/17/2018 2:04 PM, Moti Haimovsky wrote:
> > Ethdev Rx offloads API has changed since:
> > commit ce17eddefc20 ("ethdev: introduce Rx queue offloads API") This
> > commit adds support for the new Rx offloads API.
> >
> > Signed-off-by: Moti Haimovsky <motih@mellanox.com>
>
> <...>
>
> > +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;
>
> Hi Moti,
>
> I am getting following error when tried to use tap with bonding:
> "Rx queue offloads 0x0 don't match port offloads 0x1000 or supported
> offloads 0x300e"
>
> What is the intention here? I guess it tries to be sure requested queue
> offloads is subsets of port_offloads and offload_capability.
> If so not requesting any queue offload should be valid, isn't it?
Port offload should be enabled on both port and queue configuration. This is part of the documentation [1], section "8.4.7.1. Per-Port and Per-Queue Offloads"
On the above case, you try to enable the DEV_RX_OFFLOAD_CRC_STRIP on the port without enabling it on the queues. I guess DEV_RX_OFFLOAD_CRC_STRIP is reported by the tap PMD as port (and not queue) offload.
To answer some question asked privately by Ferruh:
>Why queue offload values should be set to enable per-port offload? I think this is wrong. We don't do queue offload at all, don't use that value, but this suggest that value should be some specific value to work, why?
The logic behind it was to make the offloads enablement more explicit.
Consider port offload is set on device configuration, but not set on the queue. there is an uncertainty whether the application wanted to "not enable" this offload or it was just not set (like in your case).
There other way around also makes sense: if offload was set on the port configuration then it is applied to each of the queues, with no way for the application to disable it through the queue configuration.
The current API choose option #1, we can discuss to do small change to move to #2.
>
>I believe other way around makes sense, to be able to set queue offload param, device should announce that offloading as supported. Queue is subset of the device, why you ignore device offload param to set queue offload param?
>
>What makes sense to me:
>"queue offload" is subset of "device offload" is subset of "device supported offload"
[1]
http://dpdk.org/doc/guides/prog_guide/poll_mode_drv.html
>
> <...>
On 3/12/2018 2:20 PM, Shahaf Shuler wrote:
>
>
> --Shahaf
>
>
>> -----Original Message-----
>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ferruh Yigit
>> Sent: Friday, March 2, 2018 11:44 PM
>> To: Mordechay Haimovsky <motih@mellanox.com>;
>> pascal.mazon@6wind.com
>> Cc: dev@dpdk.org; Shahaf Shuler <shahafs@mellanox.com>
>> Subject: Re: [dpdk-dev] [PATCH V5 2/2] net/tap: use new Rx offloads API
>>
>> On 1/17/2018 2:04 PM, Moti Haimovsky wrote:
>>> Ethdev Rx offloads API has changed since:
>>> commit ce17eddefc20 ("ethdev: introduce Rx queue offloads API") This
>>> commit adds support for the new Rx offloads API.
>>>
>>> Signed-off-by: Moti Haimovsky <motih@mellanox.com>
>>
>> <...>
>>
>>> +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;
>>
>> Hi Moti,
>>
>> I am getting following error when tried to use tap with bonding:
>> "Rx queue offloads 0x0 don't match port offloads 0x1000 or supported
>> offloads 0x300e"
>>
>> What is the intention here? I guess it tries to be sure requested queue
>> offloads is subsets of port_offloads and offload_capability.
>> If so not requesting any queue offload should be valid, isn't it?
>
>
> Port offload should be enabled on both port and queue configuration. This is part of the documentation [1], section "8.4.7.1. Per-Port and Per-Queue Offloads"
> On the above case, you try to enable the DEV_RX_OFFLOAD_CRC_STRIP on the port without enabling it on the queues. I guess DEV_RX_OFFLOAD_CRC_STRIP is reported by the tap PMD as port (and not queue) offload.
>
> To answer some question asked privately by Ferruh:
>> Why queue offload values should be set to enable per-port offload? I think this is wrong. We don't do queue offload at all, don't use that value, but this suggest that value should be some specific value to work, why?
>
> The logic behind it was to make the offloads enablement more explicit.
> Consider port offload is set on device configuration, but not set on the queue. there is an uncertainty whether the application wanted to "not enable" this offload or it was just not set (like in your case).
>
> There other way around also makes sense: if offload was set on the port configuration then it is applied to each of the queues, with no way for the application to disable it through the queue configuration.
>
> The current API choose option #1, we can discuss to do small change to move to #2.
There are some devices supports queue level offloads and there are some devices
supports port level offloads.
Values queue offload = 0x0 and port offload = 0x1000, for the device that
support queue level offloads may mean disabling all offloads for that specific
queue and this is valid value. For the device that support port level offloads
this is an error.
What about using "rx/tx_queue_offload_capa" field to help application to detect
if device supports queue level or port level offloads?
If device reports 0x0 "rx_queue_offload_capa", application will know device
doesn't support queue level offloads and PMD just ignore all provided queue
offloads.
If device reports a "rx_queue_offload_capa" other than 0x0, app will know that
device _supports_ queue offloads and will set offloads according and PMD will
verify and apply queue specific offloads according.
For above tap specific case, is tap PMD supports queue specific offloads?
And there should be some restrictions on offloading values:
1- requested port offloads should be subset of supported port offloads
2- supported queue offloads should be subset of supported port offloads
3- requested queue offloads should be subset of supported queue offloads
And since these information is part of dev_info, these can be managed in the
ethdev layer, instead of checked in each PMD (as done in tap).
According above, would you mind walk-trough how application can set offloads:
A) New application that implements new offloading APIs:
- Get dev_info
- Configure Rx/Tx offloads based on rx_offload_capa / tx_offload_capa
- If rx_queue_offload_capa / tx_queue_offload_capa is other than 0, setup RxQ /
TxQ offloads based on these values.
- If rx_queue_offload_capa / tx_queue_offload_capa is 0, queue level offlaods
are not supported by this device, ignore offloads during RxQ / TxQ setup.
All look OK here.
B) Old application with old offloading API
- Get dev_info, which provides only rx_offload_capa, tx_offload_capa and txq_flags
- set rte_eth_rxmode->bitfield_values ==> ethdev will convert them to port
level Rx offloads.
- port level offloads are empty!!
- ethdev will set queue level Rx offloads to be same as port level Rx offloads.
- ethdev will set txq_flags values for Tx offloads to queue level Tx offloads.
Things should work well for PMDs with old offloading API.
For the PMDs that support new offloading API, port level Tx offload values are
missing and Queue level and Port level Tx offloads mismatch. Am I missing
something here, if not how can we solve this issue in PMDs?
>
>>
>> I believe other way around makes sense, to be able to set queue offload param, device should announce that offloading as supported. Queue is subset of the device, why you ignore device offload param to set queue offload param?
>>
>> What makes sense to me:
>> "queue offload" is subset of "device offload" is subset of "device supported offload"
>
>
>
>
> [1]
> http://dpdk.org/doc/guides/prog_guide/poll_mode_drv.html
>
>
>>
>> <...>
Monday, March 12, 2018 7:00 PM, Ferruh Yigit:
> There are some devices supports queue level offloads and there are some
> devices supports port level offloads.
>
> Values queue offload = 0x0 and port offload = 0x1000, for the device that
> support queue level offloads may mean disabling all offloads for that specific
> queue and this is valid value. For the device that support port level offloads
> this is an error.
device which don't support port level offloads should not be configured with port offloads.
Well implemented PMDs will fail the configuration with port offload = 0x1000.
>
> What about using "rx/tx_queue_offload_capa" field to help application to
> detect if device supports queue level or port level offloads?
Yes. this is their purpose.
>
> If device reports 0x0 "rx_queue_offload_capa", application will know device
> doesn't support queue level offloads and PMD just ignore all provided queue
> offloads.
>
> If device reports a "rx_queue_offload_capa" other than 0x0, app will know
> that device _supports_ queue offloads and will set offloads according and
> PMD will verify and apply queue specific offloads according.
>
> For above tap specific case, is tap PMD supports queue specific offloads?
Am not sure. Moty?
>
> And there should be some restrictions on offloading values:
> 1- requested port offloads should be subset of supported port offloads
> 2- supported queue offloads should be subset of supported port offloads
> 3- requested queue offloads should be subset of supported queue offloads
This is correct.
>
> And since these information is part of dev_info, these can be managed in the
> ethdev layer, instead of checked in each PMD (as done in tap).
>
>
> According above, would you mind walk-trough how application can set
> offloads:
>
> A) New application that implements new offloading APIs:
> - Get dev_info
> - Configure Rx/Tx offloads based on rx_offload_capa / tx_offload_capa
> - If rx_queue_offload_capa / tx_queue_offload_capa is other than 0, setup
> RxQ / TxQ offloads based on these values.
> - If rx_queue_offload_capa / tx_queue_offload_capa is 0, queue level
> offlaods are not supported by this device, ignore offloads during RxQ / TxQ
> setup.
With the current API it is not correct. queue offloads should be at least the port offloads. If the application wants to enable more queue specific offloads it can as long as those are supported.
The following pseudo code demonstrate above:
Dev_configure(port, port_offloads)
Rx_queue_offloads = port_offloads
If (rx_queue_offload_capa != 0)
Rx_queue_offloads |= rx_queue_offload_capa
>
> All look OK here.
>
>
> B) Old application with old offloading API
> - Get dev_info, which provides only rx_offload_capa, tx_offload_capa and
> txq_flags
> - set rte_eth_rxmode->bitfield_values ==> ethdev will convert them to port
> level Rx offloads.
> - port level offloads are empty!!
> - ethdev will set queue level Rx offloads to be same as port level Rx offloads.
> - ethdev will set txq_flags values for Tx offloads to queue level Tx offloads.
>
> Things should work well for PMDs with old offloading API.
>
> For the PMDs that support new offloading API, port level Tx offload values
> are missing and Queue level and Port level Tx offloads mismatch. Am I
> missing something here, if not how can we solve this issue in PMDs?
Those PMDs (new PMD for old application) can use the ETH_TXQ_FLAGS_IGNORE which must be set for application which uses the new API..
see snipped code from mlx5 PMD:
[1]
/*
* Don't verify port offloads for application which
* use the old API.
*/
if (!!(conf->txq_flags & ETH_TXQ_FLAGS_IGNORE) &&
!priv_is_tx_queue_offloads_allowed(priv, conf->offloads)) {
ret = ENOTSUP;
ERROR("%p: Tx queue offloads 0x%" PRIx64 " don't match port "
"offloads 0x%" PRIx64 " or supported offloads 0x%" PRIx64,
(void *)dev, conf->offloads,
dev->data->dev_conf.txmode.offloads,
mlx5_priv_get_tx_port_offloads(priv));
goto out;
}
>
>
> >
> >>
> >> I believe other way around makes sense, to be able to set queue offload
> param, device should announce that offloading as supported. Queue is
> subset of the device, why you ignore device offload param to set queue
> offload param?
> >>
> >> What makes sense to me:
> >> "queue offload" is subset of "device offload" is subset of "device
> supported offload"
> >
> >
> >
> >
> > [1]
> >
> https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdpd
> k
> >
> .org%2Fdoc%2Fguides%2Fprog_guide%2Fpoll_mode_drv.html&data=02%7C
> 01%7Cs
> >
> hahafs%40mellanox.com%7C2fc3763767f041846bdd08d5883aa76a%7Ca65297
> 1c7d2
> >
> e4d9ba6a4d149256f461b%7C0%7C0%7C636564707857610335&sdata=Uavzne
> YAsXf2M
> > SdJWSp6i1fvRmCRyx6pWwLuzUCqOLA%3D&reserved=0
> >
> >
> >>
> >> <...>
On 3/12/2018 5:58 PM, Shahaf Shuler wrote:
> Monday, March 12, 2018 7:00 PM, Ferruh Yigit:
>> There are some devices supports queue level offloads and there are some
>> devices supports port level offloads.
>>
>> Values queue offload = 0x0 and port offload = 0x1000, for the device that
>> support queue level offloads may mean disabling all offloads for that specific
>> queue and this is valid value. For the device that support port level offloads
>> this is an error.
>
> device which don't support port level offloads should not be configured with port offloads.
> Well implemented PMDs will fail the configuration with port offload = 0x1000.
For this particular error in tap:
Rx queue offloads = 0x0,
requested port offloads = 0x1000,
supported offloads = 0x300e
Since supported offloads reported, and requested is subset of it I think port
level offloads are OK, problem is in queue level offloads.
>
>>
>> What about using "rx/tx_queue_offload_capa" field to help application to
>> detect if device supports queue level or port level offloads?
>
> Yes. this is their purpose.
>
>>
>> If device reports 0x0 "rx_queue_offload_capa", application will know device
>> doesn't support queue level offloads and PMD just ignore all provided queue
>> offloads.
>>
>> If device reports a "rx_queue_offload_capa" other than 0x0, app will know
>> that device _supports_ queue offloads and will set offloads according and
>> PMD will verify and apply queue specific offloads according.
>>
>> For above tap specific case, is tap PMD supports queue specific offloads?
>
> Am not sure. Moty?
>
>>
>> And there should be some restrictions on offloading values:
>> 1- requested port offloads should be subset of supported port offloads
>> 2- supported queue offloads should be subset of supported port offloads
>> 3- requested queue offloads should be subset of supported queue offloads
>
> This is correct.
>
>>
>> And since these information is part of dev_info, these can be managed in the
>> ethdev layer, instead of checked in each PMD (as done in tap).
>>
>>
>> According above, would you mind walk-trough how application can set
>> offloads:
>>
>> A) New application that implements new offloading APIs:
>> - Get dev_info
>> - Configure Rx/Tx offloads based on rx_offload_capa / tx_offload_capa
>> - If rx_queue_offload_capa / tx_queue_offload_capa is other than 0, setup
>> RxQ / TxQ offloads based on these values.
>> - If rx_queue_offload_capa / tx_queue_offload_capa is 0, queue level
>> offlaods are not supported by this device, ignore offloads during RxQ / TxQ
>> setup.
>
> With the current API it is not correct. queue offloads should be at least the port offloads. If the application wants to enable more queue specific offloads it can as long as those are supported.
So above statement 2 is wrong?
Just to confirm, a queue can not disable an offload configured for port but can
enable more offloads?
> The following pseudo code demonstrate above:
>
> Dev_configure(port, port_offloads)
> Rx_queue_offloads = port_offloads
> If (rx_queue_offload_capa != 0)
> Rx_queue_offloads |= rx_queue_offload_capa
Again to confirm, a device that supports port level offloads free to ignore TxQ
/ RxQ offload values, right? So why we need "Rx_queue_offloads = port_offloads",
will following be true?
Dev_configure(port, port_offloads)
If (rx_queue_offload_capa != 0)
Rx_queue_offloads = port_offloads | rx_queue_offload_capa
>
>>
>> All look OK here.
>>
>>
>> B) Old application with old offloading API
>> - Get dev_info, which provides only rx_offload_capa, tx_offload_capa and
>> txq_flags
>> - set rte_eth_rxmode->bitfield_values ==> ethdev will convert them to port
>> level Rx offloads.
>> - port level offloads are empty!!
>> - ethdev will set queue level Rx offloads to be same as port level Rx offloads.
>> - ethdev will set txq_flags values for Tx offloads to queue level Tx offloads.
>>
>> Things should work well for PMDs with old offloading API.
>>
>> For the PMDs that support new offloading API, port level Tx offload values
>> are missing and Queue level and Port level Tx offloads mismatch. Am I
>> missing something here, if not how can we solve this issue in PMDs?
>
> Those PMDs (new PMD for old application) can use the ETH_TXQ_FLAGS_IGNORE which must be set for application which uses the new API..
> see snipped code from mlx5 PMD:
>
> [1]
> /*
> * Don't verify port offloads for application which
> * use the old API.
> */
> if (!!(conf->txq_flags & ETH_TXQ_FLAGS_IGNORE) &&
> !priv_is_tx_queue_offloads_allowed(priv, conf->offloads)) {
> ret = ENOTSUP;
> ERROR("%p: Tx queue offloads 0x%" PRIx64 " don't match port "
> "offloads 0x%" PRIx64 " or supported offloads 0x%" PRIx64,
> (void *)dev, conf->offloads,
> dev->data->dev_conf.txmode.offloads,
> mlx5_priv_get_tx_port_offloads(priv));
> goto out;
> }
What this code does is: "if new offload API is used and queue offload is not
valid, return error", this is completely different than what I say.
My concern is how new PMD will handle old application because of missing port
level Tx offloads.
I guess each time PMD needs to use a Tx offload, it needs to check
ETH_TXQ_FLAGS_IGNORE. If IGNORE is set PMD will use txmode->offloads, if not it
will use txq->offloads. Do you think this solves the issue?
>
>
>
>
>
>>
>>
>>>
>>>>
>>>> I believe other way around makes sense, to be able to set queue offload
>> param, device should announce that offloading as supported. Queue is
>> subset of the device, why you ignore device offload param to set queue
>> offload param?
>>>>
>>>> What makes sense to me:
>>>> "queue offload" is subset of "device offload" is subset of "device
>> supported offload"
>>>
>>>
>>>
>>>
>>> [1]
>>>
>> https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdpd
>> k
>>>
>> .org%2Fdoc%2Fguides%2Fprog_guide%2Fpoll_mode_drv.html&data=02%7C
>> 01%7Cs
>>>
>> hahafs%40mellanox.com%7C2fc3763767f041846bdd08d5883aa76a%7Ca65297
>> 1c7d2
>>>
>> e4d9ba6a4d149256f461b%7C0%7C0%7C636564707857610335&sdata=Uavzne
>> YAsXf2M
>>> SdJWSp6i1fvRmCRyx6pWwLuzUCqOLA%3D&reserved=0
>>>
>>>
>>>>
>>>> <...>
>
Monday, March 12, 2018 9:05 PM, Ferruh Yigit:
> On 3/12/2018 5:58 PM, Shahaf Shuler wrote:
> > Monday, March 12, 2018 7:00 PM, Ferruh Yigit:
> >> There are some devices supports queue level offloads and there are
> >> some devices supports port level offloads.
> >>
> >> Values queue offload = 0x0 and port offload = 0x1000, for the device
> >> that support queue level offloads may mean disabling all offloads for
> >> that specific queue and this is valid value. For the device that
> >> support port level offloads this is an error.
> >
> > device which don't support port level offloads should not be configured
> with port offloads.
> > Well implemented PMDs will fail the configuration with port offload =
> 0x1000.
>
> For this particular error in tap:
> Rx queue offloads = 0x0,
> requested port offloads = 0x1000,
> supported offloads = 0x300e
>
> Since supported offloads reported, and requested is subset of it I think port
> level offloads are OK, problem is in queue level offloads.
>
> >
> >>
> >> And there should be some restrictions on offloading values:
> >> 1- requested port offloads should be subset of supported port
> >> offloads
> >> 2- supported queue offloads should be subset of supported port
> >> offloads
> >> 3- requested queue offloads should be subset of supported queue
> >> offloads
> >
> > This is correct.
> >
> >>
> >> And since these information is part of dev_info, these can be managed
> >> in the ethdev layer, instead of checked in each PMD (as done in tap).
> >>
> >>
> >> According above, would you mind walk-trough how application can set
> >> offloads:
> >>
> >> A) New application that implements new offloading APIs:
> >> - Get dev_info
> >> - Configure Rx/Tx offloads based on rx_offload_capa / tx_offload_capa
> >> - If rx_queue_offload_capa / tx_queue_offload_capa is other than 0,
> >> setup RxQ / TxQ offloads based on these values.
> >> - If rx_queue_offload_capa / tx_queue_offload_capa is 0, queue level
> >> offlaods are not supported by this device, ignore offloads during RxQ
> >> / TxQ setup.
> >
> > With the current API it is not correct. queue offloads should be at least the
> port offloads. If the application wants to enable more queue specific offloads
> it can as long as those are supported.
>
> So above statement 2 is wrong?
If you mean this one:
> >> 2- supported queue offloads should be subset of supported port
> >> offloads
It is correct. every queue offload can be a port offload. Since the simple case of enabling the offload on each of the queues is exactly the same as enabling it for the port.
>
> Just to confirm, a queue can not disable an offload configured for port but
> can enable more offloads?
Yes. the check in the Tap PMD verifies it.
>
> > The following pseudo code demonstrate above:
> >
> > Dev_configure(port, port_offloads)
> > Rx_queue_offloads = port_offloads
> > If (rx_queue_offload_capa != 0)
> > Rx_queue_offloads |= rx_queue_offload_capa
>
> Again to confirm, a device that supports port level offloads free to ignore TxQ
> / RxQ offload values, right?
you actually have 2 types of devices:
1. device which supports only port offloads
2. device which support both port and queue offloads.
Device of type #1 *may* ignore the queue offloads, and also will not need to verify each port offload is set also on the queue.
The expectation from application is to always follow the API (enable port offload on both port and queue) regardless how the PMD is implemented.
So why we need "Rx_queue_offloads =
> port_offloads", will following be true?
>
> Dev_configure(port, port_offloads)
> If (rx_queue_offload_capa != 0)
> Rx_queue_offloads = port_offloads | rx_queue_offload_capa
Again - the application should follow the API which currently dictates how to set port offload. It is not depends on the rx_queue_offloads capabilities.
For example, PMD which don't support queue offloads can still have verification for the API that each port offload is set also on the queue offloads.
>
> >
> >>
> >> All look OK here.
> >>
> >>
> >> B) Old application with old offloading API
> >> - Get dev_info, which provides only rx_offload_capa, tx_offload_capa
> >> and txq_flags
> >> - set rte_eth_rxmode->bitfield_values ==> ethdev will convert them
> >> to port level Rx offloads.
> >> - port level offloads are empty!!
> >> - ethdev will set queue level Rx offloads to be same as port level Rx
> offloads.
> >> - ethdev will set txq_flags values for Tx offloads to queue level Tx
> offloads.
> >>
> >> Things should work well for PMDs with old offloading API.
> >>
> >> For the PMDs that support new offloading API, port level Tx offload
> >> values are missing and Queue level and Port level Tx offloads
> >> mismatch. Am I missing something here, if not how can we solve this issue
> in PMDs?
> >
> > Those PMDs (new PMD for old application) can use the
> ETH_TXQ_FLAGS_IGNORE which must be set for application which uses the
> new API..
> > see snipped code from mlx5 PMD:
> >
> > [1]
> > /*
> > * Don't verify port offloads for application which
> > * use the old API.
> > */
> > if (!!(conf->txq_flags & ETH_TXQ_FLAGS_IGNORE) &&
> > !priv_is_tx_queue_offloads_allowed(priv, conf->offloads)) {
> > ret = ENOTSUP;
> > ERROR("%p: Tx queue offloads 0x%" PRIx64 " don't match port "
> > "offloads 0x%" PRIx64 " or supported offloads 0x%" PRIx64,
> > (void *)dev, conf->offloads,
> > dev->data->dev_conf.txmode.offloads,
> > mlx5_priv_get_tx_port_offloads(priv));
> > goto out;
> > }
>
> What this code does is: "if new offload API is used and queue offload is not
> valid, return error", this is completely different than what I say.
It also says if old API is used for this new PMD ((!!(conf->txq_flags & ETH_TXQ_FLAGS_IGNORE) is 0 ) don't do verification between the port and the queues offloads on the Tx side.
>
> My concern is how new PMD will handle old application because of missing
> port level Tx offloads.
>
> I guess each time PMD needs to use a Tx offload, it needs to check
> ETH_TXQ_FLAGS_IGNORE. If IGNORE is set PMD will use txmode->offloads,
> if not it will use txq->offloads. Do you think this solves the issue?
Old application will know to set only Tx queue offloads (since it will set only the TXQ flags). Before the ethdev offloads rework every Tx offload was queue offload, so compatibility wise it is OK.
Meaning the old PMD which was able to run with the old application should configure only Tx *queue* offloads.
Considering that, the new PMD should consider only the txq->offloads as the queue offloads and the txmode->offloads should be 0. This perfectly match the offloads API.
On 3/13/2018 7:08 AM, Shahaf Shuler wrote:
> Monday, March 12, 2018 9:05 PM, Ferruh Yigit:
>> On 3/12/2018 5:58 PM, Shahaf Shuler wrote:
>>> Monday, March 12, 2018 7:00 PM, Ferruh Yigit:
>>>> There are some devices supports queue level offloads and there are
>>>> some devices supports port level offloads.
>>>>
>>>> Values queue offload = 0x0 and port offload = 0x1000, for the device
>>>> that support queue level offloads may mean disabling all offloads for
>>>> that specific queue and this is valid value. For the device that
>>>> support port level offloads this is an error.
>>>
>>> device which don't support port level offloads should not be configured
>> with port offloads.
>>> Well implemented PMDs will fail the configuration with port offload =
>> 0x1000.
>>
>> For this particular error in tap:
>> Rx queue offloads = 0x0,
>> requested port offloads = 0x1000,
>> supported offloads = 0x300e
>>
>> Since supported offloads reported, and requested is subset of it I think port
>> level offloads are OK, problem is in queue level offloads.
>>
>>>
>>>>
>>>> And there should be some restrictions on offloading values:
>>>> 1- requested port offloads should be subset of supported port
>>>> offloads
>>>> 2- supported queue offloads should be subset of supported port
>>>> offloads
>>>> 3- requested queue offloads should be subset of supported queue
>>>> offloads
>>>
>>> This is correct.
>>>
>>>>
>>>> And since these information is part of dev_info, these can be managed
>>>> in the ethdev layer, instead of checked in each PMD (as done in tap).
>>>>
>>>>
>>>> According above, would you mind walk-trough how application can set
>>>> offloads:
>>>>
>>>> A) New application that implements new offloading APIs:
>>>> - Get dev_info
>>>> - Configure Rx/Tx offloads based on rx_offload_capa / tx_offload_capa
>>>> - If rx_queue_offload_capa / tx_queue_offload_capa is other than 0,
>>>> setup RxQ / TxQ offloads based on these values.
>>>> - If rx_queue_offload_capa / tx_queue_offload_capa is 0, queue level
>>>> offlaods are not supported by this device, ignore offloads during RxQ
>>>> / TxQ setup.
>>>
>>> With the current API it is not correct. queue offloads should be at least the
>> port offloads. If the application wants to enable more queue specific offloads
>> it can as long as those are supported.
>>
>> So above statement 2 is wrong?
>
> If you mean this one:
>>>> 2- supported queue offloads should be subset of supported port
>>>> offloads
>
> It is correct. every queue offload can be a port offload. Since the simple case of enabling the offload on each of the queues is exactly the same as enabling it for the port.
According below comment it is reverse:
"supported port offloads is subset of supported queue offloads"
>
>>
>> Just to confirm, a queue can not disable an offload configured for port but
>> can enable more offloads?
>
> Yes. the check in the Tap PMD verifies it.
>
>>
>>> The following pseudo code demonstrate above:
>>>
>>> Dev_configure(port, port_offloads)
>>> Rx_queue_offloads = port_offloads
>>> If (rx_queue_offload_capa != 0)
>>> Rx_queue_offloads |= rx_queue_offload_capa
>>
>> Again to confirm, a device that supports port level offloads free to ignore TxQ
>> / RxQ offload values, right?
>
> you actually have 2 types of devices:
> 1. device which supports only port offloads
> 2. device which support both port and queue offloads.
>
> Device of type #1 *may* ignore the queue offloads, and also will not need to verify each port offload is set also on the queue.
> The expectation from application is to always follow the API (enable port offload on both port and queue) regardless how the PMD is implemented.
>
> So why we need "Rx_queue_offloads =
>> port_offloads", will following be true?
>>
>> Dev_configure(port, port_offloads)
>> If (rx_queue_offload_capa != 0)
>> Rx_queue_offloads = port_offloads | rx_queue_offload_capa
>
> Again - the application should follow the API which currently dictates how to set port offload. It is not depends on the rx_queue_offloads capabilities.
> For example, PMD which don't support queue offloads can still have verification for the API that each port offload is set also on the queue offloads.
I am not agree with this part, why to dictate application to set queue offloads
if it already knows device doesn't support queue specific offloads?
In some of the existing PMD patches, to switch to new offloading API, PMD sets
[rt]x_queue_offload_capa as same as [rt]x_offload_capa, in that case application
can't know if queue specific offloads are supported or not and application may
try to set queue offloads, this forces PMD to verify them.
You confirmed [rt]x_queue_offload_capa is the way for application to know if
device supports queue specific offloads or not. If these values always set to
[rt]x_offload_capa, application losts this capability.
Instead:
- PMD that doesn't support queue specific offloads should set
[rt]x_queue_offload_capa to 0
- When [rt]x_queue_offload_capa is 0, application should be free to set queue
offloads whatever it wants
- When [rt]x_queue_offload_capa is 0, PMD should be free to verify queue
offloads but most probably shouldn't verify them since we don't know what
application will send.
- When [rt]x_queue_offload_capa is != 0, applications should set queue offloads
at least "[rt]x_queue_offload = [rt]x_offload"
- When [rt]x_queue_offload_capa is != 0, PMD should verify the queue offloads
>
>>
>>>
>>>>
>>>> All look OK here.
>>>>
>>>>
>>>> B) Old application with old offloading API
>>>> - Get dev_info, which provides only rx_offload_capa, tx_offload_capa
>>>> and txq_flags
>>>> - set rte_eth_rxmode->bitfield_values ==> ethdev will convert them
>>>> to port level Rx offloads.
>>>> - port level offloads are empty!!
>>>> - ethdev will set queue level Rx offloads to be same as port level Rx
>> offloads.
>>>> - ethdev will set txq_flags values for Tx offloads to queue level Tx
>> offloads.
>>>>
>>>> Things should work well for PMDs with old offloading API.
>>>>
>>>> For the PMDs that support new offloading API, port level Tx offload
>>>> values are missing and Queue level and Port level Tx offloads
>>>> mismatch. Am I missing something here, if not how can we solve this issue
>> in PMDs?
>>>
>>> Those PMDs (new PMD for old application) can use the
>> ETH_TXQ_FLAGS_IGNORE which must be set for application which uses the
>> new API..
>>> see snipped code from mlx5 PMD:
>>>
>>> [1]
>>> /*
>>> * Don't verify port offloads for application which
>>> * use the old API.
>>> */
>>> if (!!(conf->txq_flags & ETH_TXQ_FLAGS_IGNORE) &&
>>> !priv_is_tx_queue_offloads_allowed(priv, conf->offloads)) {
>>> ret = ENOTSUP;
>>> ERROR("%p: Tx queue offloads 0x%" PRIx64 " don't match port "
>>> "offloads 0x%" PRIx64 " or supported offloads 0x%" PRIx64,
>>> (void *)dev, conf->offloads,
>>> dev->data->dev_conf.txmode.offloads,
>>> mlx5_priv_get_tx_port_offloads(priv));
>>> goto out;
>>> }
>>
>> What this code does is: "if new offload API is used and queue offload is not
>> valid, return error", this is completely different than what I say.
>
> It also says if old API is used for this new PMD ((!!(conf->txq_flags & ETH_TXQ_FLAGS_IGNORE) is 0 ) don't do verification between the port and the queues offloads on the Tx side.
>
>>
>> My concern is how new PMD will handle old application because of missing
>> port level Tx offloads.
>>
>> I guess each time PMD needs to use a Tx offload, it needs to check
>> ETH_TXQ_FLAGS_IGNORE. If IGNORE is set PMD will use txmode->offloads,
>> if not it will use txq->offloads. Do you think this solves the issue?
>
> Old application will know to set only Tx queue offloads (since it will set only the TXQ flags). Before the ethdev offloads rework every Tx offload was queue offload, so compatibility wise it is OK.
> Meaning the old PMD which was able to run with the old application should configure only Tx *queue* offloads.
> Considering that, the new PMD should consider only the txq->offloads as the queue offloads and the txmode->offloads should be 0. This perfectly match the offloads API.
We are saying almost same thing,
New PMD, which supports only port level offloads,
- if dealing with _new_ application, it should use txmode->offloads
- if dealing with _old_ application, it should use txq->offloads (since
txmode->offloads is 0)
So PMD should check ETH_TXQ_FLAGS_IGNORE before every time it wants to check a
Tx offloading value.
>
>
>
>
>
>
>
Tuesday, March 13, 2018 1:57 PM, Ferruh Yigit:
> >
> > Again - the application should follow the API which currently dictates how
> to set port offload. It is not depends on the rx_queue_offloads capabilities.
> > For example, PMD which don't support queue offloads can still have
> verification for the API that each port offload is set also on the queue
> offloads.
>
> I am not agree with this part, why to dictate application to set queue offloads
> if it already knows device doesn't support queue specific offloads?
I agree we can make a small change in the API to not force the application to set the port offloads in the queue configuration. It makes sense.
The change will be:
"port offloads should be set on the port configuration. Queue offloads should be set on the queue configuration"
>
> In some of the existing PMD patches, to switch to new offloading API, PMD
> sets [rt]x_queue_offload_capa as same as [rt]x_offload_capa,
Well this is just wrong. Unless those PMDs support all the offloads in a queue level.
The logic is "every queue offload can be counted as port offload", because such offload can be set on each and every queue.
The other way around is not correct, port offload cannot be counted as queue offload.
So if such PMDs has offloads which are supported only on the port level they cannot be declared as queue offloads.
>in that case
> application can't know if queue specific offloads are supported or not and
> application may try to set queue offloads, this forces PMD to verify them.
>
> You confirmed [rt]x_queue_offload_capa is the way for application to know
> if device supports queue specific offloads or not. If these values always set to
> [rt]x_offload_capa, application losts this capability.
>
> Instead:
> - PMD that doesn't support queue specific offloads should set
> [rt]x_queue_offload_capa to 0
> - When [rt]x_queue_offload_capa is 0, application should be free to set
> queue offloads whatever it wants
I don't agree, when queue_offload_capa is 0 the expected behavior from application is not to set any offload (if we do the change in the API that you are pushing to).
PMDs can verify it or not, but if capability is not set the application should not set the offload. This is how the API should be defined.
> - When [rt]x_queue_offload_capa is 0, PMD should be free to verify queue
> offloads but most probably shouldn't verify them since we don't know what
> application will send.
>
> - When [rt]x_queue_offload_capa is != 0, applications should set queue
> offloads at least "[rt]x_queue_offload = [rt]x_offload"
If we do the change you are pushing it is not needed.
Application will set the port offload in the port configuration, and the queue offload in the queue configuration.
No need to make special treatment based on the offloads_capa.
> - When [rt]x_queue_offload_capa is != 0, PMD should verify the queue
> offloads
>
On 3/14/2018 5:49 AM, Shahaf Shuler wrote:
> Tuesday, March 13, 2018 1:57 PM, Ferruh Yigit:
>>>
>>> Again - the application should follow the API which currently dictates how
>> to set port offload. It is not depends on the rx_queue_offloads capabilities.
>>> For example, PMD which don't support queue offloads can still have
>> verification for the API that each port offload is set also on the queue
>> offloads.
>>
>> I am not agree with this part, why to dictate application to set queue offloads
>> if it already knows device doesn't support queue specific offloads?
>
> I agree we can make a small change in the API to not force the application to set the port offloads in the queue configuration. It makes sense.
> The change will be:
> "port offloads should be set on the port configuration. Queue offloads should be set on the queue configuration"
I am OK to this one, this is more reasonable for devices that support only port
level offloads.
This looks like same as option #2 mentioned in the previous mails.
>
>>
>> In some of the existing PMD patches, to switch to new offloading API, PMD
>> sets [rt]x_queue_offload_capa as same as [rt]x_offload_capa,
>
> Well this is just wrong. Unless those PMDs support all the offloads in a queue level.
>
> The logic is "every queue offload can be counted as port offload", because such offload can be set on each and every queue.
> The other way around is not correct, port offload cannot be counted as queue offload.
>
> So if such PMDs has offloads which are supported only on the port level they cannot be declared as queue offloads.
Thanks for confirming, it would be great if you can help on the PMD new offload
API patch reviews, to catch these kind of issues.
>
>
>> in that case
>> application can't know if queue specific offloads are supported or not and
>> application may try to set queue offloads, this forces PMD to verify them.
>>
>> You confirmed [rt]x_queue_offload_capa is the way for application to know
>> if device supports queue specific offloads or not. If these values always set to
>> [rt]x_offload_capa, application losts this capability.
>>
>> Instead:
>> - PMD that doesn't support queue specific offloads should set
>> [rt]x_queue_offload_capa to 0
>> - When [rt]x_queue_offload_capa is 0, application should be free to set
>> queue offloads whatever it wants
>
> I don't agree, when queue_offload_capa is 0 the expected behavior from application is not to set any offload (if we do the change in the API that you are pushing to).
> PMDs can verify it or not, but if capability is not set the application should not set the offload. This is how the API should be defined.
OK for this one.
>
>> - When [rt]x_queue_offload_capa is 0, PMD should be free to verify queue
>> offloads but most probably shouldn't verify them since we don't know what
>> application will send.
>>
>> - When [rt]x_queue_offload_capa is != 0, applications should set queue
>> offloads at least "[rt]x_queue_offload = [rt]x_offload"
>
> If we do the change you are pushing it is not needed.
> Application will set the port offload in the port configuration, and the queue offload in the queue configuration.
> No need to make special treatment based on the offloads_capa.
Right.
>
>> - When [rt]x_queue_offload_capa is != 0, PMD should verify the queue
>> offloads
>>
Back to initial question J, is tap supports queue level offloads?
If not it shouldn't be reporting or checking queue offloads.
Although it will be changed after above suggested change in API, I think check
in existing tap queue_setup, also same in mlx5, is wrong.
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 ((port_offloads ^ offloads) & port_supp_offloads)
return false;
return true;
}
take the example:
port_supp_offloads = 11111
port_offloads = 111
queue_supp_offloads = 1111
offloads = 1111
(port_offloads ^ offloads) & port_supp_offloads = 1000
Which will return false.
This only works if "port_offloads == offloads" which is practically only
supporting port level offloads.
Thursday, March 15, 2018 12:41 AM, Ferruh Yigit:
> On 3/14/2018 5:49 AM, Shahaf Shuler wrote:
> > Tuesday, March 13, 2018 1:57 PM, Ferruh Yigit:
> >>>
> >>> Again - the application should follow the API which currently
> >>> dictates how
> >> to set port offload. It is not depends on the rx_queue_offloads
> capabilities.
> >>> For example, PMD which don't support queue offloads can still have
> >> verification for the API that each port offload is set also on the
> >> queue offloads.
> >>
> >> I am not agree with this part, why to dictate application to set
> >> queue offloads if it already knows device doesn't support queue specific
> offloads?
> >
> > I agree we can make a small change in the API to not force the application
> to set the port offloads in the queue configuration. It makes sense.
> > The change will be:
> > "port offloads should be set on the port configuration. Queue offloads
> should be set on the queue configuration"
>
> I am OK to this one, this is more reasonable for devices that support only port
> level offloads.
>
> This looks like same as option #2 mentioned in the previous mails.
>
> >
> >>
> >> In some of the existing PMD patches, to switch to new offloading API,
> >> PMD sets [rt]x_queue_offload_capa as same as [rt]x_offload_capa,
> >
> > Well this is just wrong. Unless those PMDs support all the offloads in a
> queue level.
> >
> > The logic is "every queue offload can be counted as port offload", because
> such offload can be set on each and every queue.
> > The other way around is not correct, port offload cannot be counted as
> queue offload.
> >
> > So if such PMDs has offloads which are supported only on the port level
> they cannot be declared as queue offloads.
>
> Thanks for confirming, it would be great if you can help on the PMD new
> offload API patch reviews, to catch these kind of issues.
Sure, have me Cc in the patches so It can pass through my mailbox filters.
>
> >
> >
> >> in that case
> >> application can't know if queue specific offloads are supported or
> >> not and application may try to set queue offloads, this forces PMD to
> verify them.
> >>
> >> You confirmed [rt]x_queue_offload_capa is the way for application to
> >> know if device supports queue specific offloads or not. If these
> >> values always set to [rt]x_offload_capa, application losts this capability.
> >>
> >> Instead:
> >> - PMD that doesn't support queue specific offloads should set
> >> [rt]x_queue_offload_capa to 0
> >> - When [rt]x_queue_offload_capa is 0, application should be free to
> >> set queue offloads whatever it wants
> >
> > I don't agree, when queue_offload_capa is 0 the expected behavior from
> application is not to set any offload (if we do the change in the API that you
> are pushing to).
> > PMDs can verify it or not, but if capability is not set the application should
> not set the offload. This is how the API should be defined.
>
> OK for this one.
>
> >
> >> - When [rt]x_queue_offload_capa is 0, PMD should be free to verify
> >> queue offloads but most probably shouldn't verify them since we don't
> >> know what application will send.
> >>
> >> - When [rt]x_queue_offload_capa is != 0, applications should set
> >> queue offloads at least "[rt]x_queue_offload = [rt]x_offload"
> >
> > If we do the change you are pushing it is not needed.
> > Application will set the port offload in the port configuration, and the
> queue offload in the queue configuration.
> > No need to make special treatment based on the offloads_capa.
>
> Right.
>
> >
> >> - When [rt]x_queue_offload_capa is != 0, PMD should verify the queue
> >> offloads
> >>
>
>
> Back to initial question J, is tap supports queue level offloads?
> If not it shouldn't be reporting or checking queue offloads.
>
>
> Although it will be changed after above suggested change in API, I think
> check in existing tap queue_setup, also same in mlx5, is wrong.
>
> 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 ((port_offloads ^ offloads) & port_supp_offloads)
> return false;
> return true;
>
> }
>
>
> take the example:
> port_supp_offloads = 11111
> port_offloads = 111
> queue_supp_offloads = 1111
> offloads = 1111
>
> (port_offloads ^ offloads) & port_supp_offloads = 1000 Which will return
> false.
>
> This only works if "port_offloads == offloads" which is practically only
> supporting port level offloads.
For mlx5, the port_supp_offloads is internal function which returns **only** the pure port offloads (the port offloads in dev_info are rx_offload_get_queue_capa() | rx_offload_get_port_capa())
That is, offload cannot be in both port and queue offload. So the scenario above is not feasible.
On 3/15/2018 6:16 AM, Shahaf Shuler wrote:
> Thursday, March 15, 2018 12:41 AM, Ferruh Yigit:
>> On 3/14/2018 5:49 AM, Shahaf Shuler wrote:
>>> Tuesday, March 13, 2018 1:57 PM, Ferruh Yigit:
>>>>>
>>>>> Again - the application should follow the API which currently
>>>>> dictates how
>>>> to set port offload. It is not depends on the rx_queue_offloads
>> capabilities.
>>>>> For example, PMD which don't support queue offloads can still have
>>>> verification for the API that each port offload is set also on the
>>>> queue offloads.
>>>>
>>>> I am not agree with this part, why to dictate application to set
>>>> queue offloads if it already knows device doesn't support queue specific
>> offloads?
>>>
>>> I agree we can make a small change in the API to not force the application
>> to set the port offloads in the queue configuration. It makes sense.
>>> The change will be:
>>> "port offloads should be set on the port configuration. Queue offloads
>> should be set on the queue configuration"
>>
>> I am OK to this one, this is more reasonable for devices that support only port
>> level offloads.
>>
>> This looks like same as option #2 mentioned in the previous mails.
>>
>>>
>>>>
>>>> In some of the existing PMD patches, to switch to new offloading API,
>>>> PMD sets [rt]x_queue_offload_capa as same as [rt]x_offload_capa,
>>>
>>> Well this is just wrong. Unless those PMDs support all the offloads in a
>> queue level.
>>>
>>> The logic is "every queue offload can be counted as port offload", because
>> such offload can be set on each and every queue.
>>> The other way around is not correct, port offload cannot be counted as
>> queue offload.
>>>
>>> So if such PMDs has offloads which are supported only on the port level
>> they cannot be declared as queue offloads.
>>
>> Thanks for confirming, it would be great if you can help on the PMD new
>> offload API patch reviews, to catch these kind of issues.
>
> Sure, have me Cc in the patches so It can pass through my mailbox filters.
>
>>
>>>
>>>
>>>> in that case
>>>> application can't know if queue specific offloads are supported or
>>>> not and application may try to set queue offloads, this forces PMD to
>> verify them.
>>>>
>>>> You confirmed [rt]x_queue_offload_capa is the way for application to
>>>> know if device supports queue specific offloads or not. If these
>>>> values always set to [rt]x_offload_capa, application losts this capability.
>>>>
>>>> Instead:
>>>> - PMD that doesn't support queue specific offloads should set
>>>> [rt]x_queue_offload_capa to 0
>>>> - When [rt]x_queue_offload_capa is 0, application should be free to
>>>> set queue offloads whatever it wants
>>>
>>> I don't agree, when queue_offload_capa is 0 the expected behavior from
>> application is not to set any offload (if we do the change in the API that you
>> are pushing to).
>>> PMDs can verify it or not, but if capability is not set the application should
>> not set the offload. This is how the API should be defined.
>>
>> OK for this one.
>>
>>>
>>>> - When [rt]x_queue_offload_capa is 0, PMD should be free to verify
>>>> queue offloads but most probably shouldn't verify them since we don't
>>>> know what application will send.
>>>>
>>>> - When [rt]x_queue_offload_capa is != 0, applications should set
>>>> queue offloads at least "[rt]x_queue_offload = [rt]x_offload"
>>>
>>> If we do the change you are pushing it is not needed.
>>> Application will set the port offload in the port configuration, and the
>> queue offload in the queue configuration.
>>> No need to make special treatment based on the offloads_capa.
>>
>> Right.
>>
>>>
>>>> - When [rt]x_queue_offload_capa is != 0, PMD should verify the queue
>>>> offloads
>>>>
>>
>>
>> Back to initial question J, is tap supports queue level offloads?
>> If not it shouldn't be reporting or checking queue offloads.
>>
>>
>> Although it will be changed after above suggested change in API, I think
>> check in existing tap queue_setup, also same in mlx5, is wrong.
>>
>> 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 ((port_offloads ^ offloads) & port_supp_offloads)
>> return false;
>> return true;
>>
>> }
>>
>>
>> take the example:
>> port_supp_offloads = 11111
>> port_offloads = 111
>> queue_supp_offloads = 1111
>> offloads = 1111
>>
>> (port_offloads ^ offloads) & port_supp_offloads = 1000 Which will return
>> false.
>>
>> This only works if "port_offloads == offloads" which is practically only
>> supporting port level offloads.
>
> For mlx5, the port_supp_offloads is internal function which returns **only** the pure port offloads (the port offloads in dev_info are rx_offload_get_queue_capa() | rx_offload_get_port_capa())
> That is, offload cannot be in both port and queue offload. So the scenario above is not feasible.
Right, so only tap is broken J
Also, can you please verify following with mlx5:
port_supp_offloads = 10000
port_offloads = 111
queue_supp_offloads = 1111
offloads = 110
Since "offloads" is missing one of the "port_offloads" it should return error
but it doesn't. (111 ^ 110) & 10000 = 0
It can be helpful to comment these lines about the intention, otherwise hard to
understand what exactly checked from bitwise ops.
@@ -253,6 +253,43 @@ enum ioctl_mode {
}
}
+static uint64_t
+tap_rx_offload_get_port_capa(void)
+{
+ /*
+ * In order to support legacy apps,
+ * report capabilities also as port capabilities.
+ */
+ return DEV_RX_OFFLOAD_SCATTER |
+ DEV_RX_OFFLOAD_IPV4_CKSUM |
+ DEV_RX_OFFLOAD_UDP_CKSUM |
+ DEV_RX_OFFLOAD_TCP_CKSUM;
+}
+
+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;
+}
+
+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.
*/
@@ -277,8 +314,9 @@ enum ioctl_mode {
int len;
len = readv(rxq->fd, *rxq->iovecs,
- 1 + (rxq->rxmode->enable_scatter ?
- rxq->nb_rx_desc : 1));
+ 1 +
+ (rxq->rxmode->offloads & DEV_RX_OFFLOAD_SCATTER ?
+ rxq->nb_rx_desc : 1));
if (len < (int)sizeof(struct tun_pi))
break;
@@ -333,7 +371,7 @@ enum ioctl_mode {
seg->next = NULL;
mbuf->packet_type = rte_net_get_ptype(mbuf, NULL,
RTE_PTYPE_ALL_MASK);
- if (rxq->rxmode->hw_ip_checksum)
+ if (rxq->rxmode->offloads & DEV_RX_OFFLOAD_CHECKSUM)
tap_verify_csum(mbuf);
/* account for the receive frame */
@@ -694,12 +732,12 @@ enum ioctl_mode {
dev_info->min_rx_bufsize = 0;
dev_info->pci_dev = NULL;
dev_info->speed_capa = tap_dev_speed_capa();
- dev_info->rx_offload_capa = (DEV_RX_OFFLOAD_IPV4_CKSUM |
- DEV_RX_OFFLOAD_UDP_CKSUM |
- DEV_RX_OFFLOAD_TCP_CKSUM);
+ 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 = dev_info->tx_queue_offload_capa |
- tap_tx_offload_get_port_capa();
+ dev_info->tx_offload_capa = tap_tx_offload_get_port_capa() |
+ dev_info->tx_queue_offload_capa;
}
static int
@@ -1015,6 +1053,19 @@ enum ioctl_mode {
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;