Re: [dpdk-dev] [PATCH v15 3/7] ethdev: add validation to offloads set by PMD

[Andrew Rybchenko <arybchenko@solarflare.com>]

On 10/29/19 6:37 PM, pbhagavatula@marvell.com wrote:
> From: Pavan Nikhilesh <pbhagavatula@marvell.com>
>
> Some PMDs cannot work when certain offloads are enable/disabled, as a
> workaround PMDs auto enable/disable offloads internally and expose it
> through dev->data->dev_conf.rxmode.offloads.
>
> After device specific dev_configure is called compare the requested
> offloads to the offloads exposed by the PMD and, if the PMD failed
> to enable a given offload then log it and return -EINVAL from
> rte_eth_dev_configure, else if the PMD failed to disable a given offload
> log and continue with rte_eth_dev_configure.
>
> Suggested-by: Andrew Rybchenko <arybchenko@solarflare.com>
> Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com>

Few minor notes below, many thanks
Reviewed-by: Andrew Rybchenko <arybchenko@solarflare.com>

> ---
>   lib/librte_ethdev/rte_ethdev.c | 59 ++++++++++++++++++++++++++++++++++
>   1 file changed, 59 insertions(+)
>
> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
> index 3f45b9e9c..8c58da91c 100644
> --- a/lib/librte_ethdev/rte_ethdev.c
> +++ b/lib/librte_ethdev/rte_ethdev.c
> @@ -1135,6 +1135,41 @@ rte_eth_dev_tx_offload_name(uint64_t offload)
>   	return name;
>   }
>
> +static int
> +_rte_eth_dev_validate_offloads(uint16_t port_id, uint64_t req_offloads,

I'm not sure about underscore in function name. May be Thomas or
Ferruh can comment.

> +			       uint64_t set_offloads,
> +			       const char *(*f)(uint64_t))
> +{
> +	uint64_t offloads_diff = req_offloads ^ set_offloads;
> +	uint64_t offloads_req_diff, offloads_set_diff;
> +	uint64_t offload;
> +	uint8_t err = 0;
> +
> +	/* Check if any offload is advertised but not enabled. */
> +	offloads_req_diff = offloads_diff & req_offloads;
> +	while (offloads_req_diff) {
> +		offload = 1ULL << __builtin_ctzll(offloads_req_diff);
> +		offloads_req_diff &= ~offload;
> +		RTE_ETHDEV_LOG(ERR, "Port %u failed to enable %s offload",

Offload name does not include Rx/Tx, so IPV4_CKSUM is identical
and it is required to log if it is Rx or Tx offload separately.
Sounds like one more parameter.

> +			       port_id, f(offload));
> +		err = 1;
> +	}
> +
> +	if (err)
> +		return -EINVAL;
> +
> +	/* Chech if any offload couldn't be disabled. */
> +	offloads_set_diff = offloads_diff & set_offloads;
> +	while (offloads_set_diff) {
> +		offload = 1ULL << __builtin_ctzll(offloads_set_diff);
> +		offloads_set_diff &= ~offload;
> +		RTE_ETHDEV_LOG(INFO, "Port %u failed to disable %s offload",
> +			       port_id, f(offload));
> +	}

Consider to do it in one loop, something like:

         int rc = 0;

      while (offloads_diff != 0) {

		offload = 1ULL << __builtin_ctzll(offloads_diff);
		offloads_diff &= ~offload;
                 if (offload & req_offload) {
                         RTE_ETHDEV_LOG(INFO,
                                 "Port %u failed to enable %s %s offload",
                                 port_id, f(offload), rxtx);
                         rc = -EINVAL;
                 } else {
                         RTE_ETHDEV_LOG(INFO,
                                 "Port %u failed to disable %s %s offload",
	    		        port_id, f(offload), rxtx);
                 }
         }
         
         return rc;


BTW, I'm not sure that -EINVAL is good here, but right now
can't suggest anything better.

> +
> +	return 0;
> +}
> +
>   int
>   rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
>   		      const struct rte_eth_conf *dev_conf)
> @@ -1358,6 +1393,30 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
>   		goto rollback;
>   	}
>
> +	/* Validate Rx offloads. */
> +	diag = _rte_eth_dev_validate_offloads(port_id,
> +			dev_conf->rxmode.offloads,
> +			dev->data->dev_conf.rxmode.offloads,
> +			rte_eth_dev_rx_offload_name);
> +	if (diag != 0) {
> +		rte_eth_dev_rx_queue_config(dev, 0);
> +		rte_eth_dev_tx_queue_config(dev, 0);

May be it is a good moment to make one more rollback
label with queues release and avoid duplicating it.

> +		ret = diag;
> +		goto rollback;
> +	}
> +
> +	/* Validate Tx offloads. */
> +	diag = _rte_eth_dev_validate_offloads(port_id,
> +			dev_conf->txmode.offloads,
> +			dev->data->dev_conf.txmode.offloads,
> +			rte_eth_dev_tx_offload_name);
> +	if (diag != 0) {
> +		rte_eth_dev_rx_queue_config(dev, 0);
> +		rte_eth_dev_tx_queue_config(dev, 0);
> +		ret = diag;
> +		goto rollback;
> +	}
> +
>   	return 0;
>
>   rollback:
> --
> 2.17.1
>