[dpdk-dev] [PATCH v14 3/7] ethdev: log offloads that can't be disabled by PMD

[Andrew Rybchenko]

Hi Pavan,

thanks for the patch. Please, see my review notes below.

On 10/29/19 8:03 AM, pbhagavatula at marvell.com wrote:
> From: Pavan Nikhilesh <pbhagavatula at marvell.com>
>
> Some PMD can't work when certain offloads are disabled, to work around
> this the PMD auto enable the offloads internally and expose it through
> dev->data->dev_conf.rxmode.offloads.
> After dev_configure is called compare the requested offloads to the
> enabled offloads and log any offloads that have been enabled by the PMD.
>
> Suggested-by: Andrew Rybchenko <arybchenko at solarflare.com>
> Signed-off-by: Pavan Nikhilesh <pbhagavatula at marvell.com>
> ---
>   lib/librte_ethdev/rte_ethdev.c | 22 ++++++++++++++++++++++
>   1 file changed, 22 insertions(+)
>
> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
> index fef1dbb61..7dfc2f691 100644
> --- a/lib/librte_ethdev/rte_ethdev.c
> +++ b/lib/librte_ethdev/rte_ethdev.c
> @@ -1142,6 +1142,8 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
>   	struct rte_eth_dev *dev;
>   	struct rte_eth_dev_info dev_info;
>   	struct rte_eth_conf orig_conf;
> +	uint64_t offloads_force_ena;
> +	uint64_t offload;
>   	int diag;
>   	int ret;
>   
> @@ -1357,6 +1359,26 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
>   		goto rollback;
>   	}
>   
> +	/* Extract Rx offload bits that can't be disabled and log them */
> +	offloads_force_ena = dev_conf->rxmode.offloads ^
> +			dev->data->dev_conf.rxmode.offloads;

Strictly speaking XOR returns diff and in theory the diff could
catch requested but not enabled offload in fact.
So, I think the variable name should be offloads_diff.
Yes, it is unexpected and very bad, but it adds even more
value to the check.
May be requested but not enabled offloads should be checked first:
((dev_conf->rxmode.offloads & ~dev->data->dev_conf.rxmode.offloads) != 0)
but I think it would be useful to log these offloads as well to help 
debugging,
so, it should be handled below.

> +	while (__builtin_popcount(offloads_force_ena)) {

If we really need it, __builtin_popcountll() should be used, but I think we
don't need it here in fact since all we want to know if offloads_diff is 
0 or not.
So, comparison to 0 will do the job without any builtins.

> +		offload = 1ULL << __builtin_ctzll(offloads_force_ena);
> +		offloads_force_ena &= ~offload;

Below we should differentiate if the offload is requested but not 
enabled (error)
and if the offload is not requested but enabled (info or warning as Ferruh
suggested).
I think ret should be set to some error if we find any requested, but not
enabled offload and finally configure should fail (after logging of all
violations) since it is a strong violation.

Same for Tx below.

Also I think that it is better to factor out these checks into a separate
function since  rte_eth_dev_configure() is already too long.
It looks like that parameters should port ID, requested and
result offloads.

> +		RTE_ETHDEV_LOG(INFO, "Port %u can't disable Rx offload %s\n",
> +			       port_id, rte_eth_dev_rx_offload_name(offload));
> +	}
> +
> +	/* Extract Tx offload bits that can't be disabled and log them */
> +	offloads_force_ena = dev_conf->txmode.offloads ^
> +				    dev->data->dev_conf.txmode.offloads;
> +	while (__builtin_popcount(offloads_force_ena)) {
> +		offload = 1ULL << __builtin_ctzll(offloads_force_ena);
> +		offloads_force_ena &= ~offload;
> +		RTE_ETHDEV_LOG(INFO, "Port %u can't disable Tx offload %s\n",
> +			       port_id, rte_eth_dev_tx_offload_name(offload));
> +	}
> +
>   	return 0;
>   
>   rollback: