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

[Pavan Nikhilesh Bhagavatula]

>-----Original Message-----
>From: dev <dev-bounces at dpdk.org> On Behalf Of Andrew Rybchenko
>Sent: Tuesday, October 29, 2019 12:36 PM
>To: Pavan Nikhilesh Bhagavatula <pbhagavatula at marvell.com>;
>ferruh.yigit at intel.com; Jerin Jacob Kollanukkaran
><jerinj at marvell.com>; Thomas Monjalon <thomas at monjalon.net>
>Cc: dev at dpdk.org
>Subject: Re: [dpdk-dev] [PATCH v14 3/7] ethdev: log offloads that can't
>be disabled by PMD
>
>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)

Isn't the above already taken care through
"
        /* Any requested offloading must be within its device capabilities */
        if ((dev_conf->rxmode.offloads & dev_info.rx_offload_capa) !=
             dev_conf->rxmode.offloads) {
                RTE_ETHDEV_LOG(ERR,
                        "Ethdev port_id=%u requested Rx offloads 0x%"PRIx64" doesn't match Rx offloads "
                        "capabilities 0x%"PRIx64" in %s()\n",
                        port_id, dev_conf->rxmode.offloads,
                        dev_info.rx_offload_capa,
                        __func__);
                ret = -EINVAL;
                goto rollback;
        }
        if ((dev_conf->txmode.offloads & dev_info.tx_offload_capa) !=
             dev_conf->txmode.offloads) {
                RTE_ETHDEV_LOG(ERR,
                        "Ethdev port_id=%u requested Tx offloads 0x%"PRIx64" doesn't match Tx offloads "
                        "capabilities 0x%"PRIx64" in %s()\n",
                        port_id, dev_conf->txmode.offloads,
                        dev_info.tx_offload_capa,
                        __func__);
                ret = -EINVAL;
                goto rollback;
        }
"

Do you think PMDs will advertise but not enable?

>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.

Yes, we can skip using __builtin_popcount.

>
>> +		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.
>

I will move it to static function in next iteration.

>> +		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: