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

[Andrew Rybchenko <arybchenko@solarflare.com>]

On 10/29/19 11:33 AM, Pavan Nikhilesh Bhagavatula wrote:
>
>> -----Original Message-----
>> From: dev <dev-bounces@dpdk.org> On Behalf Of Andrew Rybchenko
>> Sent: Tuesday, October 29, 2019 12:36 PM
>> To: Pavan Nikhilesh Bhagavatula <pbhagavatula@marvell.com>;
>> ferruh.yigit@intel.com; Jerin Jacob Kollanukkaran
>> <jerinj@marvell.com>; Thomas Monjalon <thomas@monjalon.net>
>> Cc: dev@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@marvell.com wrote:
>>> From: Pavan Nikhilesh <pbhagavatula@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@solarflare.com>
>>> Signed-off-by: Pavan Nikhilesh <pbhagavatula@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?

Above we check the request for correctness. Here I'd like to check that
the result is correct. The problem here that we make it 100% legal for
PMD to adjust dev->data->dev_conf.rxmode.offloads so it is better
to check the result as well.

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