[dpdk-dev] [PATCH] net/fm10k: fix RSS hash config

[dpdk-dev] [PATCH] net/fm10k: fix RSS hash config

[Xiao Wang]

Sometimes app just wants to update the RSS hash function and no RSS key
update is needed, but fm10k pmd will return EINVAL for this case.

If the rss_key is NULL, we don't need to check the rss_key_len.

Fixes: 57033cdf8fdc ("fm10k: add PF RSS")

Reported-by: Xueqin Lin <xueqin.lin@intel.com>
Signed-off-by: Xiao Wang <xiao.w.wang@intel.com>
---
 drivers/net/fm10k/fm10k_ethdev.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/fm10k/fm10k_ethdev.c b/drivers/net/fm10k/fm10k_ethdev.c
index 144b2de..01f4a72 100644
--- a/drivers/net/fm10k/fm10k_ethdev.c
+++ b/drivers/net/fm10k/fm10k_ethdev.c
@@ -2159,8 +2159,8 @@ fm10k_rss_hash_update(struct rte_eth_dev *dev,
 
 	PMD_INIT_FUNC_TRACE();
 
-	if (rss_conf->rss_key_len < FM10K_RSSRK_SIZE *
-		FM10K_RSSRK_ENTRIES_PER_REG)
+	if (key && (rss_conf->rss_key_len < FM10K_RSSRK_SIZE *
+				FM10K_RSSRK_ENTRIES_PER_REG))
 		return -EINVAL;
 
 	if (hf == 0)
@@ -2202,8 +2202,8 @@ fm10k_rss_hash_conf_get(struct rte_eth_dev *dev,
 
 	PMD_INIT_FUNC_TRACE();
 
-	if (rss_conf->rss_key_len < FM10K_RSSRK_SIZE *
-				FM10K_RSSRK_ENTRIES_PER_REG)
+	if (key && (rss_conf->rss_key_len < FM10K_RSSRK_SIZE *
+				FM10K_RSSRK_ENTRIES_PER_REG))
 		return -EINVAL;
 
 	if (key != NULL)
-- 
1.9.3

Re: [dpdk-dev] [PATCH] net/fm10k: fix RSS hash config

[Chen, Jing D]

Hi,

> diff --git a/drivers/net/fm10k/fm10k_ethdev.c b/drivers/net/fm10k/fm10k_ethdev.c
> index 144b2de..01f4a72 100644
> --- a/drivers/net/fm10k/fm10k_ethdev.c
> +++ b/drivers/net/fm10k/fm10k_ethdev.c
> @@ -2159,8 +2159,8 @@ fm10k_rss_hash_update(struct rte_eth_dev *dev,
> 
>  	PMD_INIT_FUNC_TRACE();
> 
> -	if (rss_conf->rss_key_len < FM10K_RSSRK_SIZE *
> -		FM10K_RSSRK_ENTRIES_PER_REG)
> +	if (key && (rss_conf->rss_key_len < FM10K_RSSRK_SIZE *
> +				FM10K_RSSRK_ENTRIES_PER_REG))
>  		return -EINVAL;
> 
>  	if (hf == 0)

It's also possible that app wants to update rss key and not expect to update hash
function.
Is that indicate we shouldn't return error in case hf == 0?

Re: [dpdk-dev] [PATCH] net/fm10k: fix RSS hash config

[Wang, Xiao W]

Hi Mark,

> -----Original Message-----
> From: Chen, Jing D
> Sent: Thursday, July 21, 2016 4:48 PM
> To: Wang, Xiao W <xiao.w.wang@intel.com>; dev@dpdk.org
> Cc: Lin, Xueqin <xueqin.lin@intel.com>
> Subject: RE: [PATCH] net/fm10k: fix RSS hash config
> 
> Hi,
> 
> > diff --git a/drivers/net/fm10k/fm10k_ethdev.c
> > b/drivers/net/fm10k/fm10k_ethdev.c
> > index 144b2de..01f4a72 100644
> > --- a/drivers/net/fm10k/fm10k_ethdev.c
> > +++ b/drivers/net/fm10k/fm10k_ethdev.c
> > @@ -2159,8 +2159,8 @@ fm10k_rss_hash_update(struct rte_eth_dev *dev,
> >
> >  	PMD_INIT_FUNC_TRACE();
> >
> > -	if (rss_conf->rss_key_len < FM10K_RSSRK_SIZE *
> > -		FM10K_RSSRK_ENTRIES_PER_REG)
> > +	if (key && (rss_conf->rss_key_len < FM10K_RSSRK_SIZE *
> > +				FM10K_RSSRK_ENTRIES_PER_REG))
> >  		return -EINVAL;
> >
> >  	if (hf == 0)
> 
> It's also possible that app wants to update rss key and not expect to update hash
> function.
> Is that indicate we shouldn't return error in case hf == 0?
> 

If the app just wants to update RSS key, it needs to read out the RSS config first, then
change only the key field. This is what testpmd does for this operation.

hf == 0 will disable RSS feature, I think we should return error to protect multi-queue.

Best Regards,
Xiao

Re: [dpdk-dev] [PATCH] net/fm10k: fix RSS hash config

[Thomas Monjalon]

2016-07-21 09:35, Wang, Xiao W:
> From: Chen, Jing D
> > > --- a/drivers/net/fm10k/fm10k_ethdev.c
> > > +++ b/drivers/net/fm10k/fm10k_ethdev.c
> > > @@ -2159,8 +2159,8 @@ fm10k_rss_hash_update(struct rte_eth_dev *dev,
> > >
> > >  	PMD_INIT_FUNC_TRACE();
> > >
> > > -	if (rss_conf->rss_key_len < FM10K_RSSRK_SIZE *
> > > -		FM10K_RSSRK_ENTRIES_PER_REG)
> > > +	if (key && (rss_conf->rss_key_len < FM10K_RSSRK_SIZE *
> > > +				FM10K_RSSRK_ENTRIES_PER_REG))
> > >  		return -EINVAL;
> > >
> > >  	if (hf == 0)
> > 
> > It's also possible that app wants to update rss key and not expect to update hash
> > function.
> > Is that indicate we shouldn't return error in case hf == 0?
> > 
> 
> If the app just wants to update RSS key, it needs to read out the RSS config first, then
> change only the key field. This is what testpmd does for this operation.
> 
> hf == 0 will disable RSS feature, I think we should return error to protect multi-queue.

Jing, do you confirm we can apply this patch, please?

Re: [dpdk-dev] [PATCH] net/fm10k: fix RSS hash config

[Chen, Jing D]

Hi, Thomas,

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Friday, July 22, 2016 4:22 PM
> To: Chen, Jing D <jing.d.chen@intel.com>
> Cc: dev@dpdk.org; Wang, Xiao W <xiao.w.wang@intel.com>; Lin, Xueqin
> <xueqin.lin@intel.com>
> Subject: Re: [dpdk-dev] [PATCH] net/fm10k: fix RSS hash config
> 
> 2016-07-21 09:35, Wang, Xiao W:
> > From: Chen, Jing D
> > > > --- a/drivers/net/fm10k/fm10k_ethdev.c
> > > > +++ b/drivers/net/fm10k/fm10k_ethdev.c
> > > > @@ -2159,8 +2159,8 @@ fm10k_rss_hash_update(struct rte_eth_dev *dev,
> > > >
> > > >  	PMD_INIT_FUNC_TRACE();
> > > >
> > > > -	if (rss_conf->rss_key_len < FM10K_RSSRK_SIZE *
> > > > -		FM10K_RSSRK_ENTRIES_PER_REG)
> > > > +	if (key && (rss_conf->rss_key_len < FM10K_RSSRK_SIZE *
> > > > +				FM10K_RSSRK_ENTRIES_PER_REG))
> > > >  		return -EINVAL;
> > > >
> > > >  	if (hf == 0)
> > >
> > > It's also possible that app wants to update rss key and not expect to update hash
> > > function.
> > > Is that indicate we shouldn't return error in case hf == 0?
> > >
> >
> > If the app just wants to update RSS key, it needs to read out the RSS config first,
> then
> > change only the key field. This is what testpmd does for this operation.
> >
> > hf == 0 will disable RSS feature, I think we should return error to protect multi-
> queue.
> 
> Jing, do you confirm we can apply this patch, please?
I think we need some rework or more explanations here.

Re: [dpdk-dev] [PATCH] net/fm10k: fix RSS hash config

[Thomas Monjalon]

2016-07-22 08:23, Chen, Jing D:
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > 2016-07-21 09:35, Wang, Xiao W:
> > > From: Chen, Jing D
> > > > > --- a/drivers/net/fm10k/fm10k_ethdev.c
> > > > > +++ b/drivers/net/fm10k/fm10k_ethdev.c
> > > > > @@ -2159,8 +2159,8 @@ fm10k_rss_hash_update(struct rte_eth_dev *dev,
> > > > >
> > > > >  	PMD_INIT_FUNC_TRACE();
> > > > >
> > > > > -	if (rss_conf->rss_key_len < FM10K_RSSRK_SIZE *
> > > > > -		FM10K_RSSRK_ENTRIES_PER_REG)
> > > > > +	if (key && (rss_conf->rss_key_len < FM10K_RSSRK_SIZE *
> > > > > +				FM10K_RSSRK_ENTRIES_PER_REG))
> > > > >  		return -EINVAL;
> > > > >
> > > > >  	if (hf == 0)
> > > >
> > > > It's also possible that app wants to update rss key and not expect to update hash
> > > > function.
> > > > Is that indicate we shouldn't return error in case hf == 0?
> > > >
> > >
> > > If the app just wants to update RSS key, it needs to read out the RSS config first,
> > then
> > > change only the key field. This is what testpmd does for this operation.
> > >
> > > hf == 0 will disable RSS feature, I think we should return error to protect multi-
> > queue.
> > 
> > Jing, do you confirm we can apply this patch, please?
> I think we need some rework or more explanations here.

It is not reasonnable to wait RC5 for such a fix.
Either it is not important and postponed to 16.11 or you submit
a v2 very shortly for 16.07.
Please advise

Re: [dpdk-dev] [PATCH] net/fm10k: fix RSS hash config

[Chen, Jing D]

Hi, Thomas,


> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Friday, July 22, 2016 4:29 PM
> To: Chen, Jing D <jing.d.chen@intel.com>
> Cc: dev@dpdk.org; Wang, Xiao W <xiao.w.wang@intel.com>; Lin, Xueqin
> <xueqin.lin@intel.com>
> Subject: Re: [dpdk-dev] [PATCH] net/fm10k: fix RSS hash config
> 
> 2016-07-22 08:23, Chen, Jing D:
> > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > > 2016-07-21 09:35, Wang, Xiao W:
> > > > From: Chen, Jing D
> > > > > > --- a/drivers/net/fm10k/fm10k_ethdev.c
> > > > > > +++ b/drivers/net/fm10k/fm10k_ethdev.c
> > > > > > @@ -2159,8 +2159,8 @@ fm10k_rss_hash_update(struct rte_eth_dev
> *dev,
> > > > > >
> > > > > >  	PMD_INIT_FUNC_TRACE();
> > > > > >
> > > > > > -	if (rss_conf->rss_key_len < FM10K_RSSRK_SIZE *
> > > > > > -		FM10K_RSSRK_ENTRIES_PER_REG)
> > > > > > +	if (key && (rss_conf->rss_key_len < FM10K_RSSRK_SIZE *
> > > > > > +				FM10K_RSSRK_ENTRIES_PER_REG))
> > > > > >  		return -EINVAL;
> > > > > >
> > > > > >  	if (hf == 0)
> > > > >
> > > > > It's also possible that app wants to update rss key and not expect to update
> hash
> > > > > function.
> > > > > Is that indicate we shouldn't return error in case hf == 0?
> > > > >
> > > >
> > > > If the app just wants to update RSS key, it needs to read out the RSS config first,
> > > then
> > > > change only the key field. This is what testpmd does for this operation.
> > > >
> > > > hf == 0 will disable RSS feature, I think we should return error to protect
> multi-
> > > queue.
> > >
> > > Jing, do you confirm we can apply this patch, please?
> > I think we need some rework or more explanations here.
> 
> It is not reasonnable to wait RC5 for such a fix.
> Either it is not important and postponed to 16.11 or you submit
> a v2 very shortly for 16.07.
> Please advise

Please kindly merge.

Re: [dpdk-dev] [PATCH] net/fm10k: fix RSS hash config

[Thomas Monjalon]

2016-07-21 16:24, Xiao Wang:
> Sometimes app just wants to update the RSS hash function and no RSS key
> update is needed, but fm10k pmd will return EINVAL for this case.
> 
> If the rss_key is NULL, we don't need to check the rss_key_len.
> 
> Fixes: 57033cdf8fdc ("fm10k: add PF RSS")
> 
> Reported-by: Xueqin Lin <xueqin.lin@intel.com>
> Signed-off-by: Xiao Wang <xiao.w.wang@intel.com>

Applied, thanks