Re: [dpdk-dev] [PATCH] net/thunderx: Convert ThunderX VNIC PMD to new offload API

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

On 1/15/2018 2:49 PM, Maciej Czekaj wrote:
> 
> 
> 
> 
> -- Oryginal message --
>> On 1/3/2018 1:12 PM, maciej.czekaj@caviumnetworks.com wrote:
>>> From: Maciej Czekaj <maciej.czekaj@caviumnetworks.com>
>>>
>>> This patch removes all references to old-style offload API
>>> replacing them with new offload flags.
>>>
>>> Signed-off-by: Maciej Czekaj <maciej.czekaj@caviumnetworks.com>
>> <...>
>>
>>>   
>>>   	dev_info->default_txconf = (struct rte_eth_txconf) {
>>>   		.tx_free_thresh = NICVF_DEFAULT_TX_FREE_THRESH,
>>> -		.txq_flags =
>>> -			ETH_TXQ_FLAGS_NOMULTSEGS  |
>>> -			ETH_TXQ_FLAGS_NOREFCOUNT  |
>>> -			ETH_TXQ_FLAGS_NOMULTMEMP  |
>>> -			ETH_TXQ_FLAGS_NOVLANOFFL  |
>>> -			ETH_TXQ_FLAGS_NOXSUMSCTP,
>>> +		.txq_flags = ETH_TXQ_FLAGS_IGNORE,
>> I am not sure about this, Shahafs may comment better, shouldn't application
>> decide to set "c" or not, instead of having this in default
>> configuration?
> I think of it as a safeguard against a legacy application that is using 
> old-style txq_flags.
> 
> There is an assymetry in API, since
>   - rte_eth_tx_queue setup() and rte_eth_rx_queue_setup() convert flags
>   - rte_eth_dev_info_get() - does not convert them.
> 
> The scenario leading to issues is as follows:
> 
> 1.Application reads default txq_flags from the rte_eth_dev_info_get().
>   - dev_info.txconf.offloads contains flags but it is ignored by legacy code
>   - dev_info.txq_flags may be:
>        a) txq_flags == 0
>        b) txq_flags ==  ETH_TXQ_FLAGS_IGNORE
>        c) txq_flags == new-style flags converted to old-style flags
> 
> 
> 2. Application uses default txq_flags field to rte_eth_tx_queue_setup(). 
> Now, depending on txq_flags:
> 
>    a) txq_flags == 0, ethdev layer __clears__ all offloads, see *
>    b) txq_flags ==  ETH_TXQ_FLAGS_IGNORE, ethdev layer converts offloads 
> to txq_flags, but leaves orignal offloads, so PMD can still use them
>    c) txq_flags == old-style flags, ethdev layer converts txq_flags to 
> offloads, destroying default offloads
> 
> 
> * relevant code snippet from rte_eth_tx_queue_setup() with comments:
> 
>      if (tx_conf->txq_flags & ETH_TXQ_FLAGS_IGNORE) {
>          // ==> converts offloads to txq_flags but LEAVES offloads, too
>          rte_eth_convert_txq_offloads(tx_conf->offloads,
>                           &local_conf.txq_flags);
>          /* Keep the ignore flag. */
>          local_conf.txq_flags |= ETH_TXQ_FLAGS_IGNORE;
>      } else {
>          // ==> converts txq_flags to offloads but DESTROYS original 
> offloads
> rte_eth_convert_txq_flags(tx_conf->txq_flags,
>                        &local_conf.offloads);
>      }
> 
> 
> So, out of 3 options:
> 
> a) does not work for legacy code
> b) will work for legacy code
> c) will work but defeats the purpose of removing old-style flags, since 
> dev_info() callback has to setup both old-style and new-style default flags
> 
> I chose b) as the simplest way to work-around the issue. I could post a 
> patch to ethdev API if you think it is important.

What I understand is txq_flags should be supported because of legacy apps. That
is why ethdev layer converts old txq_flags to offloads when new
ETH_TXQ_FLAGS_IGNORE flag is missing. So that PMD can only use "offloads"
variable for both legacy and new applications.

To support the case application uses defaults from PMD, the one you mentioned
above, I think we should go with option c). For implementation you can use only
new "offloads", so it is not both old-style and new-style, just new-style, but
as default yes should keep both.

And new applications will set ETH_TXQ_FLAGS_IGNORE to say "offloads" has valid
information. Currently this also converted to the txq_flags but this will go
away when all PMDs support new method.

Think  about a case where a legacy application gets defaults from PMD and sets a
few more values in txq_flags and configure the device. When you set
ETH_TXQ_FLAGS_IGNORE as part of defaults, ethdev layer will think only
"offloads" is valid and it will overwrite the txq_flags value with offloads one
and the updates to the txq_flags will be lost.

At one point ETH_TXQ_FLAGS_IGNORE will also go away and applications also will
need to support new method. When it is removed than we can get rid of the
txq_flags defaults from PMDs until than I guess we need to live with them.

> 
>>
>> <...>
>>
>>> +	if ((conf_tx_offloads & tx_offload_capa) != conf_tx_offloads) {
>>> +		PMD_INIT_LOG(ERR, "Some Tx offloads are not supported "
>>> +		      "requested 0x%lx supported 0x%lx\n",
>>> +		      conf_tx_offloads, tx_offload_capa);
>> This is broken for 32bits, using PRIx64 instead of "lx" makes your code more
>> portable.
> Will fix in v2.
> 
> 
>