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:59 PM, Ferruh Yigit wrote:
> 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

Hi Pavan, Andrew, Jerin,

All this bonding issue, tespmd rx_offload copy not being correct, the fail log
on each app start because PMD force enables this config.

I remember I have requested it to keep the device configuration correct, but it
is causing more troubles than expected.

Instead we can remove those PMD updates, so when application not requested
RSS_HAS, PMD will say OK and update the configuration according, but indeed it
will continue to update mbuf:rss:hash fields, since PMD is doing more than
requested this shouldn't be a problem in application side.
The behavior can be fixed when PMD implements the disabling RSS_HASH but this is
most probably won't happen for some PMDs because of performance reasons. So the
device configuration may remain wrong for those PMDs.

Overall, since that was the case in prev versions, I believe you are OK but can
you confirm again please?

> 
>> 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;
>>>>   +      }
>>
>