>-----Original Message-----
>From: dev <dev-boun...@dpdk.org> On Behalf Of Andrew Rybchenko
>Sent: Tuesday, October 29, 2019 12:36 PM
>To: Pavan Nikhilesh Bhagavatula <pbhagavat...@marvell.com>;
>ferruh.yi...@intel.com; Jerin Jacob Kollanukkaran
><jer...@marvell.com>; Thomas Monjalon <tho...@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, pbhagavat...@marvell.com wrote:
>> From: Pavan Nikhilesh <pbhagavat...@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 <arybche...@solarflare.com>
>> Signed-off-by: Pavan Nikhilesh <pbhagavat...@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:
