On 1/16/2018 9:32 AM, Maciej Czekaj wrote:
>
>
>
>
> -- Oryginal message --
>> On 1/15/2018 2:49 PM, Maciej Czekaj wrote:
>>>
>>>
>>>
>>> -- Oryginal message --
>>>> On 1/3/2018 1:12 PM, maciej.cze...@caviumnetworks.com wrote:
>>>>> From: Maciej Czekaj <maciej.cze...@caviumnetworks.com>
>>>>>
>>>>> This patch removes all references to old-style offload API
>>>>> replacing them with new offload flags.
>>>>>
>>>>> Signed-off-by: Maciej Czekaj <maciej.cze...@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.
Yes, thank you.
>
> 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.
>>>
>>>
>>>
>
