Re: [dpdk-dev] [PATCH V5 2/2] net/tap: use new Rx offloads API

[Ferruh Yigit <ferruh.yigit@intel.com>]

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