Re: [dpdk-dev] [PATCH v10] ethdev: new Rx/Tx offloads API

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

On 5/10/2018 10:25 AM, Andrew Rybchenko wrote:
> On 05/10/2018 03:56 AM, 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 triger an error log.
>> 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>
>>
>> ---
>> v10:
>> sorry, miss the code change, fix the buidling error
>>
>> v9:
>> replace RTE_PMD_DEBUG_TRACE with ethdev_log(ERR, in ethdev
>> to avoid failure of application which hasn't been completely
>> converted to new offload API.
> 
> [...]
> 
>> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
>> index e560524..5baa2aa 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) {
>> +		ethdev_log(ERR, "ethdev port_id=%d requested Rx offloads "
>> +				"0x%" PRIx64 " doesn't match Rx offloads "
>> +				"capabilities 0x%" PRIx64 " in %s( )\n",
>> +				port_id,
>> +				local_conf.rxmode.offloads,
>> +				dev_info.rx_offload_capa,
>> +				__func__);
> 
> Why is return -EINVAL removed here?
> If application is not updated to use offloads, offloads is 0 and everything is OK.
> If application is updated to use offloads, its behaviour must be consistent.
> Same below for Tx device offloads.

To be cautious to not break apps for the cases we have missed. For example
testpmd was giving error with virtual PMDs because of CRC_STRIP, it is easy to
fix testpmd but other applications too may have similar problem.

Overall agree that error should be return, for this release it will only print
error log, next release we can add return back. Next release applications will
be switched to new offloading API so they will already need to be updated, and
hopefully that change will be before rc stage.

> 
>> +	}
>> +	if ((local_conf.txmode.offloads & dev_info.tx_offload_capa) !=
>> +	     local_conf.txmode.offloads) {
>> +		ethdev_log(ERR, "ethdev port_id=%d requested Tx offloads "
>> +				"0x%" PRIx64 " doesn't match Tx offloads "
>> +				"capabilities 0x%" PRIx64 " in %s( )\n",
>> +				port_id,
>> +				local_conf.txmode.offloads,
>> +				dev_info.tx_offload_capa,
>> +				__func__);
>> +	}
>> +
>>  	/* 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,38 @@ 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;
>> +
>> +	/*
>> +	 * 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) {
>> +		ethdev_log(ERR, "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__);
> 
> May be it is really a good tradeoff to remove error return here.
> Ideally it would be nice to see explanation here why.
> 
>> +	}
>> +
>>  	ret = (*dev->dev_ops->rx_queue_setup)(dev, rx_queue_id, nb_rx_desc,
>>  					      socket_id, &local_conf, mp);
>>  	if (!ret) {
>> @@ -1612,6 +1666,38 @@ rte_eth_tx_queue_setup(uint16_t port_id, uint16_t tx_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.txmode.offloads;
>> +
>> +	/*
>> +	 * 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.tx_queue_offload_capa) !=
>> +	     local_conf.offloads) {
>> +		ethdev_log(ERR, "Ethdev port_id=%d tx_queue_id=%d, new "
>> +				"added offloads 0x%" PRIx64 " must be "
>> +				"within pre-queue offload capabilities 0x%"
>> +				PRIx64 " in %s( )\n",
>> +				port_id,
>> +				tx_queue_id,
>> +				local_conf.offloads,
>> +				dev_info.tx_queue_offload_capa,
>> +				__func__);
>> +	}
>> +
>>  	return eth_err(port_id, (*dev->dev_ops->tx_queue_setup)(dev,
>>  		       tx_queue_id, nb_tx_desc, socket_id, &local_conf));
>>  }
>