Re: [dpdk-dev] [PATCH v8] ethdev: check Rx/Tx offloads

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

On 5/9/2018 3:11 PM, Ferruh Yigit wrote:
> On 5/8/2018 6:51 PM, Andrew Rybchenko wrote:
>> On 05/08/2018 01:10 PM, Wei Dai wrote:
>>> This patch check if a input requested offloading is valid or not.
>>> Any reuqested offloading must be supported in the device capabilities.
>>> Any offloading is disabled by default if it is not set in the parameter
>>> dev_conf->[rt]xmode.offloads to rte_eth_dev_configure( ) and
>>> [rt]x_conf->offloads to rte_eth_[rt]x_queue_setup( ).
>>> If any offloading is enabled in rte_eth_dev_configure( ) by application,
>>> it is enabled on all queues no matter whether it is per-queue or
>>> per-port type and no matter whether it is set or cleared in
>>> [rt]x_conf->offloads to rte_eth_[rt]x_queue_setup( ).
>>> If a per-queue offloading hasn't be enabled in rte_eth_dev_configure( ),
>>> it can be enabled or disabled for individual queue in
>>> ret_eth_[rt]x_queue_setup( ).
>>> A new added offloading is the one which hasn't been enabled in
>>> rte_eth_dev_configure( ) and is reuqested to be enabled in
>>> rte_eth_[rt]x_queue_setup( ), it must be per-queue type,
>>> otherwise return error.
>>> The underlying PMD must be aware that the requested offloadings
>>> to PMD specific queue_setup( ) function only carries those
>>> new added offloadings of per-queue type.
>>>
>>> This patch can make above such checking in a common way in rte_ethdev
>>> layer to avoid same checking in underlying PMD.
>>>
>>> This patch assumes that all PMDs in 18.05-rc2 have already
>>> converted to offload API defined in 17.11 . It also assumes
>>> that all PMDs can return correct offloading capabilities
>>> in rte_eth_dev_infos_get( ).
>>>
>>> In the beginning of [rt]x_queue_setup( ) of underlying PMD,
>>> add offloads = [rt]xconf->offloads |
>>> dev->data->dev_conf.[rt]xmode.offloads; to keep same as offload API
>>> defined in 17.11 to avoid upper application broken due to offload
>>> API change.
>>> PMD can use the info that input [rt]xconf->offloads only carry
>>> the new added per-queue offloads to do some optimization or some
>>> code change on base of this patch.
>>>
>>> Signed-off-by: Wei Dai <wei.dai@intel.com>
>>> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
>>> Signed-off-by: Qi Zhang <qi.z.zhang@intel.com>
>>
>> [...]
>>
>>> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
>>> index e560524..523a07b 100644
>>> --- a/lib/librte_ethdev/rte_ethdev.c
>>> +++ b/lib/librte_ethdev/rte_ethdev.c
>>> @@ -1139,6 +1139,28 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
>>>   							ETHER_MAX_LEN;
>>>   	}
>>>   
>>> +	/* Any requested offloading must be within its device capabilities */
>>> +	if ((local_conf.rxmode.offloads & dev_info.rx_offload_capa) !=
>>> +	     local_conf.rxmode.offloads) {
>>> +		RTE_PMD_DEBUG_TRACE("ethdev port_id=%d requested Rx offloads "
>>> +				    "0x%" PRIx64 " doesn't match Rx offloads "
>>> +				    "capabilities 0x%" PRIx64 "\n",
>>> +				    port_id,
>>> +				    local_conf.rxmode.offloads,
>>> +				    dev_info.rx_offload_capa);
>>> +		return -EINVAL;
>>> +	}
>>> +	if ((local_conf.txmode.offloads & dev_info.tx_offload_capa) !=
>>> +	     local_conf.txmode.offloads) {
>>> +		RTE_PMD_DEBUG_TRACE("ethdev port_id=%d requested Tx offloads "
>>> +				    "0x%" PRIx64 " doesn't match Tx offloads "
>>> +				    "capabilities 0x%" PRIx64 "\n",
>>> +				    port_id,
>>> +				    local_conf.txmode.offloads,
>>> +				    dev_info.tx_offload_capa);
>>> +		return -EINVAL;
>>> +	}
>>> +
>>>   	/* Check that device supports requested rss hash functions. */
>>>   	if ((dev_info.flow_type_rss_offloads |
>>>   	     dev_conf->rx_adv_conf.rss_conf.rss_hf) !=
>>> @@ -1504,6 +1526,39 @@ rte_eth_rx_queue_setup(uint16_t port_id, uint16_t rx_queue_id,
>>>   						    &local_conf.offloads);
>>>   	}
>>>   
>>> +	/*
>>> +	 * If an offloading has already been enabled in
>>> +	 * rte_eth_dev_configure(), it has been enabled on all queues,
>>> +	 * so there is no need to enable it in this queue again.
>>> +	 * The local_conf.offloads input to underlying PMD only carries
>>> +	 * those offloadings which are only enabled on this queue and
>>> +	 * not enabled on all queues.
>>> +	 * The underlying PMD must be aware of this point.
>>> +	 */
>>> +	local_conf.offloads &= ~dev->data->dev_conf.rxmode.offloads;
>>
>> In fact it has a problem with MULTI_SEG.
>> It is a new offload which was introduced to substitute NOMULTISEG TxQ flag.
>> If PMD reports the offload on device level, but application does not use
>> the new interface and request the offload on queue level using txq_flags
>> (absent NOMULTISEG flag). As the result below check fails.
>> Sounds like it requires dedicated code to handle it. Anything else?
> 
> Old applications don't set Tx offloads on configure() at all. But they set them
> in tx_queue_setup().
> For the PMDs that support only port offloads old applications will fail in
> tx_queue_setup(), if I don't miss anything.
> 
> I suggest keeping the log but remove the error return until all applications
> switch to new offloading API which should be on next release. What do you think?

Also virtual devices will give an error with this because of current testpmd
implementation [1].

We may hit similar unexpected issues, as Shahaf voiced same concern.

What do you think removing error return from all three locations,
rte_eth_dev_configure(), rte_eth_rx_queue_setup(), rte_eth_tx_queue_setup() and
just keep error log, for this release? Next release we will already update the
application interface, can add error returns next release.

[1]
https://dpdk.org/dev/patchwork/patch/39643/

> 
> 
> btw, the log macro "RTE_PMD_DEBUG_TRACE" is in ERR log type but it controlled by
> CONFIG_RTE_LIBRTE_ETHDEV_DEBUG config option which is disabled by default. So by
> default we don't get the error log but the error only. I suggest switching to
> new dynamic logging ethdev_log()
> 
> 
>>
>>> +
>>> +	/*
>>> +	 * New added offloadings for this queue are those not enabled in
>>> +	 * rte_eth_dev_configure( ) and they must be per-queue type.
>>> +	 * A pure per-port offloading can't be enabled on a queue while
>>> +	 * disabled on another queue. A pure per-port offloading can't
>>> +	 * be enabled for any queue as new added one if it hasn't been
>>> +	 * enabled in rte_eth_dev_configure( ).
>>> +	 */
>>> +	if ((local_conf.offloads & dev_info.rx_queue_offload_capa) !=
>>> +	     local_conf.offloads) {
>>> +		RTE_PMD_DEBUG_TRACE("Ethdev port_id=%d rx_queue_id=%d, new "
>>> +				    "added offloads 0x" PRIx64 " must be "
>>> +				    "within pre-queue offload capabilities 0x"
>>> +				    PRIx64 " in %s\n",
>>> +				    port_id,
>>> +				    rx_queue_id,
>>> +				    local_conf.offloads,
>>> +				    dev_info.rx_queue_offload_capa,
>>> +				    __func__);
>>> +		return -EINVAL;
>>> +	}
>>> +
>>>   	ret = (*dev->dev_ops->rx_queue_setup)(dev, rx_queue_id, nb_rx_desc,
>>>   					      socket_id, &local_conf, mp);
>>>   	if (!ret) {
>>
>> [...]
>>
>