Re: [dpdk-dev] [PATCH v17 04/10] drivers/net: update Rx RSS hash offload capabilities

[Ferruh Yigit <ferruh.yigit@intel.com>]

On 11/14/2019 1:24 PM, Andrew Rybchenko wrote:
> On 11/14/19 3:57 PM, Jerin Jacob wrote:
>> On Wed, Nov 13, 2019 at 11:32 PM Ferruh Yigit <ferruh.yigit@intel.com> wrote:
>>> On 11/11/2019 1:19 PM, pbhagavatula@marvell.com wrote:
>>>> diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c
>>>> index 30c0379d4..8c1caac18 100644
>>>> --- a/drivers/net/ixgbe/ixgbe_ethdev.c
>>>> +++ b/drivers/net/ixgbe/ixgbe_ethdev.c
>>>> @@ -2402,6 +2402,9 @@ ixgbe_dev_configure(struct rte_eth_dev *dev)
>>>>        int ret;
>>>>
>>>>        PMD_INIT_FUNC_TRACE();
>>>> +
>>>> +     dev->data->dev_conf.rxmode.offloads |= DEV_RX_OFFLOAD_RSS_HASH;
>>>> +
>>> Hi Pavan, Andrew, Jerin,
>>>
>>> This is causing trouble when device is re-configured with its existing data,
>>> because of the check in the ethdev [1].
>>>
>>> This is for the case device configuration get, a few thing changed and
>>> re-configured case, this is what is done in bonding and it is failing now.
>>>
>>> Do you have any suggestion for the solution?
>>>
>>> We can set this offload in PMDs only if 'ETH_MQ_RX_RSS_FLAG' set but same thing
>>> still can caught us, if the initial configuration was with 'ETH_MQ_RX_RSS_FLAG'
>>> but reconfigure is not.
>>>
>>> Or we can relax the check in 'rte_eth_dev_configure()' to log but not return error?
>> IMO, We can relax the check as it is NOT at all critical for this flag.
> 
> Other option is to set RSS_HASH above if (dev_conf->rxmode.mq_mode & 
> ETH_MQ_RX_RSS_FLAG).

+1 to this check in PMDs, even we can disable the RSS_HASH offload when RSS is
not enabled [2], but this won't solve the problem completely. User can do first
configure with RSS enabled and second with RSS disabled, it will be same issue.

[2]
in PMD, configure():
if (dev_conf->rxmode.mq_mode & TH_MQ_RX_RSS_FLAG)
 offload |= RSS_HASH
else
 offload &= ~RSS_HASH

> Should I care about it?
> 
>>> [1]
>>>
>>>   @@ -1305,6 +1306,17 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t
>>> nb_rx_q, uint16_t nb_tx_q,
>>>                  goto rollback;
>>>          }
>>>
>>>   +      /* Check if Rx RSS distribution is disabled but RSS hash is enabled. */
>>>   +      if (((dev_conf->rxmode.mq_mode & ETH_MQ_RX_RSS_FLAG) == 0) &&
>>>   +          (dev_conf->rxmode.offloads & DEV_RX_OFFLOAD_RSS_HASH)) {
>>>   +              RTE_ETHDEV_LOG(ERR,
>>>   +                      "Ethdev port_id=%u config invalid Rx mq_mode without RSS  but %s offload is
>>> requested",
>>>   +                      port_id,
>>>   +                      rte_eth_dev_rx_offload_name(DEV_RX_OFFLOAD_RSS_HASH));
>>>   +              ret = -EINVAL;
>>>   +              goto rollback;
>>>   +      }
>