net/failsafe: report valid RSS RETA size
diff mbox series

Message ID 1590590073-8964-1-git-send-email-arybchenko@solarflare.com
State Accepted
Delegated to: Ferruh Yigit
Headers show
Series
  • net/failsafe: report valid RSS RETA size
Related show

Checks

Context Check Description
ci/Intel-compilation fail apply issues
ci/checkpatch success coding style OK

Commit Message

Andrew Rybchenko May 27, 2020, 2:34 p.m. UTC
From: Ian Dolzhansky <ian.dolzhansky@oktetlabs.ru>

Failsafe driver has been indicating zero for RSS redirection table size
after device info reporting had been reworked. Report proper value.

Fixes: 4586be3743d4 ("net/failsafe: fix reported device info")
Cc: Stephen Hemminger <sthemmin@microsoft.com>
Cc: stable@dpdk.org

Signed-off-by: Ian Dolzhansky <ian.dolzhansky@oktetlabs.ru>
Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
---
 drivers/net/failsafe/failsafe_ops.c | 8 ++++++++
 1 file changed, 8 insertions(+)

Comments

Gaëtan Rivet May 27, 2020, 3:07 p.m. UTC | #1
On 27/05/20 15:34 +0100, Andrew Rybchenko wrote:
> From: Ian Dolzhansky <ian.dolzhansky@oktetlabs.ru>
> 
> Failsafe driver has been indicating zero for RSS redirection table size
> after device info reporting had been reworked. Report proper value.
> 
> Fixes: 4586be3743d4 ("net/failsafe: fix reported device info")
> Cc: Stephen Hemminger <sthemmin@microsoft.com>
> Cc: stable@dpdk.org
> 
> Signed-off-by: Ian Dolzhansky <ian.dolzhansky@oktetlabs.ru>
> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
> ---
>  drivers/net/failsafe/failsafe_ops.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/net/failsafe/failsafe_ops.c b/drivers/net/failsafe/failsafe_ops.c
> index e046cfe6aa..45da9378c3 100644
> --- a/drivers/net/failsafe/failsafe_ops.c
> +++ b/drivers/net/failsafe/failsafe_ops.c
> @@ -1068,6 +1068,13 @@ fs_dev_merge_info(struct rte_eth_dev_info *info,
>  	info->rx_queue_offload_capa &= sinfo->rx_queue_offload_capa;
>  	info->tx_queue_offload_capa &= sinfo->tx_queue_offload_capa;
>  	info->flow_type_rss_offloads &= sinfo->flow_type_rss_offloads;
> +
> +	/*
> +	 * RETA size is a GCD of RETA sizes indicated by sub-devices.
> +	 * Each of these sizes is a power of 2, so use the lower one.
> +	 */
> +	info->reta_size = RTE_MIN(info->reta_size, sinfo->reta_size);
> +
>  	info->hash_key_size = RTE_MIN(info->hash_key_size,
>  				      sinfo->hash_key_size);
>  
> @@ -1128,6 +1135,7 @@ fs_dev_infos_get(struct rte_eth_dev *dev,
>  	infos->max_hash_mac_addrs = UINT32_MAX;
>  	infos->max_vfs = UINT16_MAX;
>  	infos->max_vmdq_pools = UINT16_MAX;
> +	infos->reta_size = UINT16_MAX;
>  	infos->hash_key_size = UINT8_MAX;
>  
>  	/*
> -- 
> 2.17.1
> 

Hello Andrew, Ian,

The reta_size info is linked to being able to update the RSS RETA.

I don't think it is a bug for the moment to report 0, as long as
failsafe does not support RETA update. Now, the reta_update ops could be
quickly implemented in failsafe, but that should be a new feature.

Did you hit an issue with it?

Regards,
Andrew Rybchenko May 27, 2020, 3:30 p.m. UTC | #2
On 5/27/20 6:07 PM, Gaëtan Rivet wrote:
> On 27/05/20 15:34 +0100, Andrew Rybchenko wrote:
>> From: Ian Dolzhansky <ian.dolzhansky@oktetlabs.ru>
>>
>> Failsafe driver has been indicating zero for RSS redirection table size
>> after device info reporting had been reworked. Report proper value.
>>
>> Fixes: 4586be3743d4 ("net/failsafe: fix reported device info")
>> Cc: Stephen Hemminger <sthemmin@microsoft.com>
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Ian Dolzhansky <ian.dolzhansky@oktetlabs.ru>
>> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
>> ---
>>  drivers/net/failsafe/failsafe_ops.c | 8 ++++++++
>>  1 file changed, 8 insertions(+)
>>
>> diff --git a/drivers/net/failsafe/failsafe_ops.c b/drivers/net/failsafe/failsafe_ops.c
>> index e046cfe6aa..45da9378c3 100644
>> --- a/drivers/net/failsafe/failsafe_ops.c
>> +++ b/drivers/net/failsafe/failsafe_ops.c
>> @@ -1068,6 +1068,13 @@ fs_dev_merge_info(struct rte_eth_dev_info *info,
>>  	info->rx_queue_offload_capa &= sinfo->rx_queue_offload_capa;
>>  	info->tx_queue_offload_capa &= sinfo->tx_queue_offload_capa;
>>  	info->flow_type_rss_offloads &= sinfo->flow_type_rss_offloads;
>> +
>> +	/*
>> +	 * RETA size is a GCD of RETA sizes indicated by sub-devices.
>> +	 * Each of these sizes is a power of 2, so use the lower one.
>> +	 */
>> +	info->reta_size = RTE_MIN(info->reta_size, sinfo->reta_size);
>> +
>>  	info->hash_key_size = RTE_MIN(info->hash_key_size,
>>  				      sinfo->hash_key_size);
>>  
>> @@ -1128,6 +1135,7 @@ fs_dev_infos_get(struct rte_eth_dev *dev,
>>  	infos->max_hash_mac_addrs = UINT32_MAX;
>>  	infos->max_vfs = UINT16_MAX;
>>  	infos->max_vmdq_pools = UINT16_MAX;
>> +	infos->reta_size = UINT16_MAX;
>>  	infos->hash_key_size = UINT8_MAX;
>>  
>>  	/*
>> -- 
>> 2.17.1
>>
> 
> Hello Andrew, Ian,
> 
> The reta_size info is linked to being able to update the RSS RETA.
> 
> I don't think it is a bug for the moment to report 0, as long as
> failsafe does not support RETA update. Now, the reta_update ops could be
> quickly implemented in failsafe, but that should be a new feature.
> 
> Did you hit an issue with it?

Hello Gaetan,

in our case reta_size is required for RSS testing even if there
is no reta update and query API to estimate where a packet
should be delivered. It assumes that default reta spreads over
all Rx queue evenly. I.e. we need reta size and number of Rx
queues to built default reta.

May be it is really very testing specific, but IMO logical
anyway.

Andrew.
Gaëtan Rivet May 27, 2020, 9:35 p.m. UTC | #3
On 27/05/20 18:30 +0300, Andrew Rybchenko wrote:
> On 5/27/20 6:07 PM, Gaëtan Rivet wrote:
> > On 27/05/20 15:34 +0100, Andrew Rybchenko wrote:
> >> From: Ian Dolzhansky <ian.dolzhansky@oktetlabs.ru>
> >>
> >> Failsafe driver has been indicating zero for RSS redirection table size
> >> after device info reporting had been reworked. Report proper value.
> >>
> >> Fixes: 4586be3743d4 ("net/failsafe: fix reported device info")
> >> Cc: Stephen Hemminger <sthemmin@microsoft.com>
> >> Cc: stable@dpdk.org
> >>
> >> Signed-off-by: Ian Dolzhansky <ian.dolzhansky@oktetlabs.ru>
> >> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
> >> ---
> >>  drivers/net/failsafe/failsafe_ops.c | 8 ++++++++
> >>  1 file changed, 8 insertions(+)
> >>
> >> diff --git a/drivers/net/failsafe/failsafe_ops.c b/drivers/net/failsafe/failsafe_ops.c
> >> index e046cfe6aa..45da9378c3 100644
> >> --- a/drivers/net/failsafe/failsafe_ops.c
> >> +++ b/drivers/net/failsafe/failsafe_ops.c
> >> @@ -1068,6 +1068,13 @@ fs_dev_merge_info(struct rte_eth_dev_info *info,
> >>  	info->rx_queue_offload_capa &= sinfo->rx_queue_offload_capa;
> >>  	info->tx_queue_offload_capa &= sinfo->tx_queue_offload_capa;
> >>  	info->flow_type_rss_offloads &= sinfo->flow_type_rss_offloads;
> >> +
> >> +	/*
> >> +	 * RETA size is a GCD of RETA sizes indicated by sub-devices.
> >> +	 * Each of these sizes is a power of 2, so use the lower one.
> >> +	 */
> >> +	info->reta_size = RTE_MIN(info->reta_size, sinfo->reta_size);
> >> +
> >>  	info->hash_key_size = RTE_MIN(info->hash_key_size,
> >>  				      sinfo->hash_key_size);
> >>  
> >> @@ -1128,6 +1135,7 @@ fs_dev_infos_get(struct rte_eth_dev *dev,
> >>  	infos->max_hash_mac_addrs = UINT32_MAX;
> >>  	infos->max_vfs = UINT16_MAX;
> >>  	infos->max_vmdq_pools = UINT16_MAX;
> >> +	infos->reta_size = UINT16_MAX;
> >>  	infos->hash_key_size = UINT8_MAX;
> >>  
> >>  	/*
> >> -- 
> >> 2.17.1
> >>
> > 
> > Hello Andrew, Ian,
> > 
> > The reta_size info is linked to being able to update the RSS RETA.
> > 
> > I don't think it is a bug for the moment to report 0, as long as
> > failsafe does not support RETA update. Now, the reta_update ops could be
> > quickly implemented in failsafe, but that should be a new feature.
> > 
> > Did you hit an issue with it?
> 
> Hello Gaetan,
> 
> in our case reta_size is required for RSS testing even if there
> is no reta update and query API to estimate where a packet
> should be delivered. It assumes that default reta spreads over
> all Rx queue evenly. I.e. we need reta size and number of Rx
> queues to built default reta.
> 
> May be it is really very testing specific, but IMO logical
> anyway.
> 
> Andrew.

Alright, given the size of the change there is no reason not to add it
if it can improve consistency.

I think the patch is fine otherwise, I'm just not sure it should be
marked as a bug fix, but backporting won't be hard.

Thanks,

Acked-by: Gaetan Rivet <grive@u256.net>
Stephen Hemminger May 27, 2020, 10:02 p.m. UTC | #4
On Wed, 27 May 2020 23:35:25 +0200
Gaëtan Rivet <grive@u256.net> wrote:

> On 27/05/20 18:30 +0300, Andrew Rybchenko wrote:
> > On 5/27/20 6:07 PM, Gaëtan Rivet wrote:  
> > > On 27/05/20 15:34 +0100, Andrew Rybchenko wrote:  
> > >> From: Ian Dolzhansky <ian.dolzhansky@oktetlabs.ru>
> > >>
> > >> Failsafe driver has been indicating zero for RSS redirection table size
> > >> after device info reporting had been reworked. Report proper value.
> > >>
> > >> Fixes: 4586be3743d4 ("net/failsafe: fix reported device info")
> > >> Cc: Stephen Hemminger <sthemmin@microsoft.com>
> > >> Cc: stable@dpdk.org
> > >>
> > >> Signed-off-by: Ian Dolzhansky <ian.dolzhansky@oktetlabs.ru>
> > >> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
> > >> ---
> > >>  drivers/net/failsafe/failsafe_ops.c | 8 ++++++++
> > >>  1 file changed, 8 insertions(+)
> > >>
> > >> diff --git a/drivers/net/failsafe/failsafe_ops.c b/drivers/net/failsafe/failsafe_ops.c
> > >> index e046cfe6aa..45da9378c3 100644
> > >> --- a/drivers/net/failsafe/failsafe_ops.c
> > >> +++ b/drivers/net/failsafe/failsafe_ops.c
> > >> @@ -1068,6 +1068,13 @@ fs_dev_merge_info(struct rte_eth_dev_info *info,
> > >>  	info->rx_queue_offload_capa &= sinfo->rx_queue_offload_capa;
> > >>  	info->tx_queue_offload_capa &= sinfo->tx_queue_offload_capa;
> > >>  	info->flow_type_rss_offloads &= sinfo->flow_type_rss_offloads;
> > >> +
> > >> +	/*
> > >> +	 * RETA size is a GCD of RETA sizes indicated by sub-devices.
> > >> +	 * Each of these sizes is a power of 2, so use the lower one.
> > >> +	 */
> > >> +	info->reta_size = RTE_MIN(info->reta_size, sinfo->reta_size);
> > >> +
> > >>  	info->hash_key_size = RTE_MIN(info->hash_key_size,
> > >>  				      sinfo->hash_key_size);
> > >>  
> > >> @@ -1128,6 +1135,7 @@ fs_dev_infos_get(struct rte_eth_dev *dev,
> > >>  	infos->max_hash_mac_addrs = UINT32_MAX;
> > >>  	infos->max_vfs = UINT16_MAX;
> > >>  	infos->max_vmdq_pools = UINT16_MAX;
> > >> +	infos->reta_size = UINT16_MAX;
> > >>  	infos->hash_key_size = UINT8_MAX;
> > >>  
> > >>  	/*
> > >> -- 
> > >> 2.17.1
> > >>  
> > > 
> > > Hello Andrew, Ian,
> > > 
> > > The reta_size info is linked to being able to update the RSS RETA.
> > > 
> > > I don't think it is a bug for the moment to report 0, as long as
> > > failsafe does not support RETA update. Now, the reta_update ops could be
> > > quickly implemented in failsafe, but that should be a new feature.
> > > 
> > > Did you hit an issue with it?  
> > 
> > Hello Gaetan,
> > 
> > in our case reta_size is required for RSS testing even if there
> > is no reta update and query API to estimate where a packet
> > should be delivered. It assumes that default reta spreads over
> > all Rx queue evenly. I.e. we need reta size and number of Rx
> > queues to built default reta.
> > 
> > May be it is really very testing specific, but IMO logical
> > anyway.
> > 
> > Andrew.  
> 
> Alright, given the size of the change there is no reason not to add it
> if it can improve consistency.
> 
> I think the patch is fine otherwise, I'm just not sure it should be
> marked as a bug fix, but backporting won't be hard.
> 
> Thanks,
> 
> Acked-by: Gaetan Rivet <grive@u256.net>
> 

Agreed, looks good to me, but probably doesn't matter much. Almost all hardware
only supports one reta_size anyway.

Acked-by: Stephen Hemminger <stephen@networkplumber.org>
Ferruh Yigit June 2, 2020, 6:10 p.m. UTC | #5
On 5/27/2020 11:02 PM, Stephen Hemminger wrote:
> On Wed, 27 May 2020 23:35:25 +0200
> Gaëtan Rivet <grive@u256.net> wrote:
> 
>> On 27/05/20 18:30 +0300, Andrew Rybchenko wrote:
>>> On 5/27/20 6:07 PM, Gaëtan Rivet wrote:  
>>>> On 27/05/20 15:34 +0100, Andrew Rybchenko wrote:  
>>>>> From: Ian Dolzhansky <ian.dolzhansky@oktetlabs.ru>
>>>>>
>>>>> Failsafe driver has been indicating zero for RSS redirection table size
>>>>> after device info reporting had been reworked. Report proper value.
>>>>>
>>>>> Fixes: 4586be3743d4 ("net/failsafe: fix reported device info")
>>>>> Cc: Stephen Hemminger <sthemmin@microsoft.com>
>>>>> Cc: stable@dpdk.org
>>>>>
>>>>> Signed-off-by: Ian Dolzhansky <ian.dolzhansky@oktetlabs.ru>
>>>>> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
>>>>> ---
>>>>>  drivers/net/failsafe/failsafe_ops.c | 8 ++++++++
>>>>>  1 file changed, 8 insertions(+)
>>>>>
>>>>> diff --git a/drivers/net/failsafe/failsafe_ops.c b/drivers/net/failsafe/failsafe_ops.c
>>>>> index e046cfe6aa..45da9378c3 100644
>>>>> --- a/drivers/net/failsafe/failsafe_ops.c
>>>>> +++ b/drivers/net/failsafe/failsafe_ops.c
>>>>> @@ -1068,6 +1068,13 @@ fs_dev_merge_info(struct rte_eth_dev_info *info,
>>>>>  	info->rx_queue_offload_capa &= sinfo->rx_queue_offload_capa;
>>>>>  	info->tx_queue_offload_capa &= sinfo->tx_queue_offload_capa;
>>>>>  	info->flow_type_rss_offloads &= sinfo->flow_type_rss_offloads;
>>>>> +
>>>>> +	/*
>>>>> +	 * RETA size is a GCD of RETA sizes indicated by sub-devices.
>>>>> +	 * Each of these sizes is a power of 2, so use the lower one.
>>>>> +	 */
>>>>> +	info->reta_size = RTE_MIN(info->reta_size, sinfo->reta_size);
>>>>> +
>>>>>  	info->hash_key_size = RTE_MIN(info->hash_key_size,
>>>>>  				      sinfo->hash_key_size);
>>>>>  
>>>>> @@ -1128,6 +1135,7 @@ fs_dev_infos_get(struct rte_eth_dev *dev,
>>>>>  	infos->max_hash_mac_addrs = UINT32_MAX;
>>>>>  	infos->max_vfs = UINT16_MAX;
>>>>>  	infos->max_vmdq_pools = UINT16_MAX;
>>>>> +	infos->reta_size = UINT16_MAX;
>>>>>  	infos->hash_key_size = UINT8_MAX;
>>>>>  
>>>>>  	/*
>>>>> -- 
>>>>> 2.17.1
>>>>>  
>>>>
>>>> Hello Andrew, Ian,
>>>>
>>>> The reta_size info is linked to being able to update the RSS RETA.
>>>>
>>>> I don't think it is a bug for the moment to report 0, as long as
>>>> failsafe does not support RETA update. Now, the reta_update ops could be
>>>> quickly implemented in failsafe, but that should be a new feature.
>>>>
>>>> Did you hit an issue with it?  
>>>
>>> Hello Gaetan,
>>>
>>> in our case reta_size is required for RSS testing even if there
>>> is no reta update and query API to estimate where a packet
>>> should be delivered. It assumes that default reta spreads over
>>> all Rx queue evenly. I.e. we need reta size and number of Rx
>>> queues to built default reta.
>>>
>>> May be it is really very testing specific, but IMO logical
>>> anyway.
>>>
>>> Andrew.  
>>
>> Alright, given the size of the change there is no reason not to add it
>> if it can improve consistency.
>>
>> I think the patch is fine otherwise, I'm just not sure it should be
>> marked as a bug fix, but backporting won't be hard.
>>
>> Thanks,
>>
>> Acked-by: Gaetan Rivet <grive@u256.net>
>>
> 
> Agreed, looks good to me, but probably doesn't matter much. Almost all hardware
> only supports one reta_size anyway.
> 
> Acked-by: Stephen Hemminger <stephen@networkplumber.org>
> 

Applied to dpdk-next-net/master, thanks.

Patch
diff mbox series

diff --git a/drivers/net/failsafe/failsafe_ops.c b/drivers/net/failsafe/failsafe_ops.c
index e046cfe6aa..45da9378c3 100644
--- a/drivers/net/failsafe/failsafe_ops.c
+++ b/drivers/net/failsafe/failsafe_ops.c
@@ -1068,6 +1068,13 @@  fs_dev_merge_info(struct rte_eth_dev_info *info,
 	info->rx_queue_offload_capa &= sinfo->rx_queue_offload_capa;
 	info->tx_queue_offload_capa &= sinfo->tx_queue_offload_capa;
 	info->flow_type_rss_offloads &= sinfo->flow_type_rss_offloads;
+
+	/*
+	 * RETA size is a GCD of RETA sizes indicated by sub-devices.
+	 * Each of these sizes is a power of 2, so use the lower one.
+	 */
+	info->reta_size = RTE_MIN(info->reta_size, sinfo->reta_size);
+
 	info->hash_key_size = RTE_MIN(info->hash_key_size,
 				      sinfo->hash_key_size);
 
@@ -1128,6 +1135,7 @@  fs_dev_infos_get(struct rte_eth_dev *dev,
 	infos->max_hash_mac_addrs = UINT32_MAX;
 	infos->max_vfs = UINT16_MAX;
 	infos->max_vmdq_pools = UINT16_MAX;
+	infos->reta_size = UINT16_MAX;
 	infos->hash_key_size = UINT8_MAX;
 
 	/*