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

[Andrew Rybchenko <arybchenko@solarflare.com>]

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.

> +	}
> +	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));
>   }