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

[Maciej Czekaj <maciej.czekaj@caviumnetworks.com>]

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

So you suggest that dev_info callback should provide default txq_flags 
for a moment?

E.g.

.txq_flags =
	ETH_TXQ_FLAGS_NOMULTSEGS  |
	ETH_TXQ_FLAGS_NOREFCOUNT  |
	ETH_TXQ_FLAGS_NOMULTMEMP  |
	ETH_TXQ_FLAGS_NOVLANOFFL  |
	ETH_TXQ_FLAGS_NOXSUMSCTP,


That is OK with me. We'll wipe it out later whet it all go away.

Will fix in v2.

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