diff mbox series

net/failsafe: report valid RSS RETA size

Message ID 1590590073-8964-1-git-send-email-arybchenko@solarflare.com (mailing list archive)
State Accepted, archived
Delegated to: Ferruh Yigit
Headers
Series net/failsafe: report valid RSS RETA size |

Checks

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

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.
diff mbox series

Patch

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;
 
 	/*