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

[Andrew Rybchenko <arybchenko@solarflare.com>]

On 05/08/2018 01:05 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/drivers/net/sfc/sfc_ethdev.c b/drivers/net/sfc/sfc_ethdev.c
> index e42d553..fc2b254 100644
> --- a/drivers/net/sfc/sfc_ethdev.c
> +++ b/drivers/net/sfc/sfc_ethdev.c
> @@ -413,14 +413,16 @@ sfc_rx_queue_setup(struct rte_eth_dev *dev, uint16_t rx_queue_id,
>   {
>   	struct sfc_adapter *sa = dev->data->dev_private;
>   	int rc;
> +	uint64_t offloads;
>   
>   	sfc_log_init(sa, "RxQ=%u nb_rx_desc=%u socket_id=%u",
>   		     rx_queue_id, nb_rx_desc, socket_id);
>   
>   	sfc_adapter_lock(sa);
>   
> +	offloads = rx_conf->offloads | dev->data->dev_conf.rxmode.offloads;

I'd prefer to see it inside sfc_rx_qinit() function. It would allow to avoid
sfc_rx_qinit() function prototype changes.

>   	rc = sfc_rx_qinit(sa, rx_queue_id, nb_rx_desc, socket_id,
> -			  rx_conf, mb_pool);
> +			  rx_conf, mb_pool, offloads);
>   	if (rc != 0)
>   		goto fail_rx_qinit;
>   
> @@ -469,13 +471,16 @@ sfc_tx_queue_setup(struct rte_eth_dev *dev, uint16_t tx_queue_id,
>   {
>   	struct sfc_adapter *sa = dev->data->dev_private;
>   	int rc;
> +	uint64_t offloads;
>   
>   	sfc_log_init(sa, "TxQ = %u, nb_tx_desc = %u, socket_id = %u",
>   		     tx_queue_id, nb_tx_desc, socket_id);
>   
>   	sfc_adapter_lock(sa);
>   
> -	rc = sfc_tx_qinit(sa, tx_queue_id, nb_tx_desc, socket_id, tx_conf);
> +	offloads = tx_conf->offloads | dev->data->dev_conf.txmode.offloads;

Same as above.

> +	rc = sfc_tx_qinit(sa, tx_queue_id, nb_tx_desc, socket_id,
> +			  tx_conf, offloads);
>   	if (rc != 0)
>   		goto fail_tx_qinit;

[...]

It looks like device level offloads are checked on ethdev layer now.
So, I think check in sfc_rx_check_mode() and sfc_tx_check_mode()
may be removed as well (see offloads_rejected).
I think it will make functions sfc_rx_log_offloads() and
sfc_tx_log_offloads() unused and these functions should be removed.