[dpdk-dev] [PATCH] ixgbe: Fix an unnecessary check in vf rss

[dpdk-dev] [PATCH] ixgbe: Fix an unnecessary check in vf rss

[Ouyang Changchun]

To follow up the comments from Wodkowski, PawelX, remove this unnecessary check,
as check_mq_mode has already check the queue number in device configure stage,
if the queue number of vf is not correct, it will return error code and exit,
so it doesn't need check again here in device start stage(note: pf_host_configure
is called in device start stage).

This fixes commit 42d2f78abcb77ecb769be4149df550308169ef0f

Signed-off-by: Changchun Ouyang <changchun.ouyang@intel.com>
---
 lib/librte_pmd_ixgbe/ixgbe_pf.c | 15 ---------------
 1 file changed, 15 deletions(-)

diff --git a/lib/librte_pmd_ixgbe/ixgbe_pf.c b/lib/librte_pmd_ixgbe/ixgbe_pf.c
index 93f6e43..dbda9b5 100644
--- a/lib/librte_pmd_ixgbe/ixgbe_pf.c
+++ b/lib/librte_pmd_ixgbe/ixgbe_pf.c
@@ -187,21 +187,6 @@ int ixgbe_pf_host_configure(struct rte_eth_dev *eth_dev)
 	IXGBE_WRITE_REG(hw, IXGBE_MPSAR_LO(hw->mac.num_rar_entries), 0);
 	IXGBE_WRITE_REG(hw, IXGBE_MPSAR_HI(hw->mac.num_rar_entries), 0);
 
-	/*
-	 * VF RSS can support at most 4 queues for each VF, even if
-	 * 8 queues are available for each VF, it need refine to 4
-	 * queues here due to this limitation, otherwise no queue
-	 * will receive any packet even RSS is enabled.
-	 */
-	if (eth_dev->data->dev_conf.rxmode.mq_mode == ETH_MQ_RX_VMDQ_RSS) {
-		if (RTE_ETH_DEV_SRIOV(eth_dev).nb_q_per_pool == 8) {
-			RTE_ETH_DEV_SRIOV(eth_dev).active = ETH_32_POOLS;
-			RTE_ETH_DEV_SRIOV(eth_dev).nb_q_per_pool = 4;
-			RTE_ETH_DEV_SRIOV(eth_dev).def_pool_q_idx =
-				dev_num_vf(eth_dev) * 4;
-		}
-	}
-
 	/* set VMDq map to default PF pool */
 	hw->mac.ops.set_vmdq(hw, 0, RTE_ETH_DEV_SRIOV(eth_dev).def_vmdq_idx);
 
-- 
1.8.4.2

Re: [dpdk-dev] [PATCH] ixgbe: Fix an unnecessary check in vf rss

[Thomas Monjalon]

> To follow up the comments from Wodkowski, PawelX, remove this unnecessary check,
> as check_mq_mode has already check the queue number in device configure stage,
> if the queue number of vf is not correct, it will return error code and exit,
> so it doesn't need check again here in device start stage(note: pf_host_configure
> is called in device start stage).
> 
> This fixes commit 42d2f78abcb77ecb769be4149df550308169ef0f
> 
> Signed-off-by: Changchun Ouyang <changchun.ouyang@intel.com>

Suggested-by: Pawel Wodkowski <pawelx.wodkowski@intel.com>
Fixes: 42d2f78abcb77 ("configure VF RSS")

Applied

Changchun, as you are working on ixgbe, maybe you would like to review
some ixgbe patches from others?

Thanks
-- 
Thomas

Re: [dpdk-dev] [PATCH] ixgbe: Fix an unnecessary check in vf rss

[Ouyang, Changchun]

Hi Thomas,

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Tuesday, January 27, 2015 8:13 PM
> To: Ouyang, Changchun
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] ixgbe: Fix an unnecessary check in vf rss
> 
> > To follow up the comments from Wodkowski, PawelX, remove this
> > unnecessary check, as check_mq_mode has already check the queue
> number
> > in device configure stage, if the queue number of vf is not correct,
> > it will return error code and exit, so it doesn't need check again
> > here in device start stage(note: pf_host_configure is called in device start
> stage).
> >
> > This fixes commit 42d2f78abcb77ecb769be4149df550308169ef0f
> >
> > Signed-off-by: Changchun Ouyang <changchun.ouyang@intel.com>
> 
> Suggested-by: Pawel Wodkowski <pawelx.wodkowski@intel.com>
> Fixes: 42d2f78abcb77 ("configure VF RSS")
> 
> Applied
> 

Thanks very much for the applying!

> Changchun, as you are working on ixgbe, maybe you would like to review
> some ixgbe patches from others?
> 

No problem, I will try to do it when my bandwidth allows me to do it, :-)
Thanks
Changchun

Re: [dpdk-dev] [PATCH] ixgbe: Fix an unnecessary check in vf rss

[Wodkowski, PawelX]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ouyang, Changchun
> Sent: Wednesday, January 28, 2015 2:35 AM
> To: Thomas Monjalon
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] ixgbe: Fix an unnecessary check in vf rss
> 
> Hi Thomas,
> 
> > -----Original Message-----
> > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > Sent: Tuesday, January 27, 2015 8:13 PM
> > To: Ouyang, Changchun
> > Cc: dev@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH] ixgbe: Fix an unnecessary check in vf rss
> >
> > > To follow up the comments from Wodkowski, PawelX, remove this
> > > unnecessary check, as check_mq_mode has already check the queue
> > number
> > > in device configure stage, if the queue number of vf is not correct,
> > > it will return error code and exit, so it doesn't need check again
> > > here in device start stage(note: pf_host_configure is called in device start
> > stage).
> > >
> > > This fixes commit 42d2f78abcb77ecb769be4149df550308169ef0f
> > >
> > > Signed-off-by: Changchun Ouyang <changchun.ouyang@intel.com>
> >
> > Suggested-by: Pawel Wodkowski <pawelx.wodkowski@intel.com>
> > Fixes: 42d2f78abcb77 ("configure VF RSS")
> >
> > Applied
> >
> 
> Thanks very much for the applying!
> 
> > Changchun, as you are working on ixgbe, maybe you would like to review
> > some ixgbe patches from others?
> >
> 
> No problem, I will try to do it when my bandwidth allows me to do it, :-)
> Thanks
> Changchun

Actually I was suggesting exactly opposite direction. Main issue is that the
sriov field in rte_eth_dev_data is only used by igb and ixgbe drivers. In addition
rte_eth_dev_check_mq_mode() is specialized for ixgbe driver.

I am thinking about moving sriov from rte_eth_dev_data to driver's private
structure or at least move rte_eth_dev_check_mq_mode() to struct eth_dev_ops
as optional driver configuration step.

What do you think about both steps?

Pawel

Re: [dpdk-dev] [PATCH] ixgbe: Fix an unnecessary check in vf rss

[Ouyang, Changchun]

Hi PawelX

> -----Original Message-----
> From: Wodkowski, PawelX
> Sent: Friday, January 30, 2015 12:14 AM
> To: Ouyang, Changchun; Thomas Monjalon; Richardson, Bruce
> Cc: dev@dpdk.org
> Subject: RE: [dpdk-dev] [PATCH] ixgbe: Fix an unnecessary check in vf rss
> 
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ouyang,
> Changchun
> > Sent: Wednesday, January 28, 2015 2:35 AM
> > To: Thomas Monjalon
> > Cc: dev@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH] ixgbe: Fix an unnecessary check in vf
> > rss
> >
> > Hi Thomas,
> >
> > > -----Original Message-----
> > > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > > Sent: Tuesday, January 27, 2015 8:13 PM
> > > To: Ouyang, Changchun
> > > Cc: dev@dpdk.org
> > > Subject: Re: [dpdk-dev] [PATCH] ixgbe: Fix an unnecessary check in
> > > vf rss
> > >
> > > > To follow up the comments from Wodkowski, PawelX, remove this
> > > > unnecessary check, as check_mq_mode has already check the queue
> > > number
> > > > in device configure stage, if the queue number of vf is not
> > > > correct, it will return error code and exit, so it doesn't need
> > > > check again here in device start stage(note: pf_host_configure is
> > > > called in device start
> > > stage).
> > > >
> > > > This fixes commit 42d2f78abcb77ecb769be4149df550308169ef0f
> > > >
> > > > Signed-off-by: Changchun Ouyang <changchun.ouyang@intel.com>
> > >
> > > Suggested-by: Pawel Wodkowski <pawelx.wodkowski@intel.com>
> > > Fixes: 42d2f78abcb77 ("configure VF RSS")
> > >
> > > Applied
> > >
> >
> > Thanks very much for the applying!
> >
> > > Changchun, as you are working on ixgbe, maybe you would like to
> > > review some ixgbe patches from others?
> > >
> >
> > No problem, I will try to do it when my bandwidth allows me to do it,
> > :-) Thanks Changchun
> 
> Actually I was suggesting exactly opposite direction. Main issue is that the
> sriov field in rte_eth_dev_data is only used by igb and ixgbe drivers. In
> addition
> rte_eth_dev_check_mq_mode() is specialized for ixgbe driver.
> 
> I am thinking about moving sriov from rte_eth_dev_data to driver's private
> structure or at least move rte_eth_dev_check_mq_mode() to struct
> eth_dev_ops as optional driver configuration step.
> 
> What do you think about both steps?

Good opinion!
I prefer to move rte_eth_dev_check_mq_mode to eth_dev_ops as optional driver configure,
The reason is that in future other eth type may also need such kind of check or even refine some queue number values by their own way,
I can help review your patch after you send out. 
Thanks for your enhancing that.

Changchun

 

Re: [dpdk-dev] [PATCH] ixgbe: Fix an unnecessary check in vf rss

[Wodkowski, PawelX]

> -----Original Message-----
> From: Ouyang, Changchun
> Sent: Friday, January 30, 2015 2:20 AM
> To: Wodkowski, PawelX; Thomas Monjalon; Richardson, Bruce
> Cc: dev@dpdk.org; Ouyang, Changchun
> Subject: RE: [dpdk-dev] [PATCH] ixgbe: Fix an unnecessary check in vf rss
> 
> Hi PawelX
> 
> > -----Original Message-----
> > From: Wodkowski, PawelX
> > Sent: Friday, January 30, 2015 12:14 AM
> > To: Ouyang, Changchun; Thomas Monjalon; Richardson, Bruce
> > Cc: dev@dpdk.org
> > Subject: RE: [dpdk-dev] [PATCH] ixgbe: Fix an unnecessary check in vf rss
> >
> > > -----Original Message-----
> > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ouyang,
> > Changchun
> > > Sent: Wednesday, January 28, 2015 2:35 AM
> > > To: Thomas Monjalon
> > > Cc: dev@dpdk.org
> > > Subject: Re: [dpdk-dev] [PATCH] ixgbe: Fix an unnecessary check in vf
> > > rss
> > >
> > > Hi Thomas,
> > >
> > > > -----Original Message-----
> > > > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > > > Sent: Tuesday, January 27, 2015 8:13 PM
> > > > To: Ouyang, Changchun
> > > > Cc: dev@dpdk.org
> > > > Subject: Re: [dpdk-dev] [PATCH] ixgbe: Fix an unnecessary check in
> > > > vf rss
> > > >
> > > > > To follow up the comments from Wodkowski, PawelX, remove this
> > > > > unnecessary check, as check_mq_mode has already check the queue
> > > > number
> > > > > in device configure stage, if the queue number of vf is not
> > > > > correct, it will return error code and exit, so it doesn't need
> > > > > check again here in device start stage(note: pf_host_configure is
> > > > > called in device start
> > > > stage).
> > > > >
> > > > > This fixes commit 42d2f78abcb77ecb769be4149df550308169ef0f
> > > > >
> > > > > Signed-off-by: Changchun Ouyang <changchun.ouyang@intel.com>
> > > >
> > > > Suggested-by: Pawel Wodkowski <pawelx.wodkowski@intel.com>
> > > > Fixes: 42d2f78abcb77 ("configure VF RSS")
> > > >
> > > > Applied
> > > >
> > >
> > > Thanks very much for the applying!
> > >
> > > > Changchun, as you are working on ixgbe, maybe you would like to
> > > > review some ixgbe patches from others?
> > > >
> > >
> > > No problem, I will try to do it when my bandwidth allows me to do it,
> > > :-) Thanks Changchun
> >
> > Actually I was suggesting exactly opposite direction. Main issue is that the
> > sriov field in rte_eth_dev_data is only used by igb and ixgbe drivers. In
> > addition
> > rte_eth_dev_check_mq_mode() is specialized for ixgbe driver.
> >
> > I am thinking about moving sriov from rte_eth_dev_data to driver's private
> > structure or at least move rte_eth_dev_check_mq_mode() to struct
> > eth_dev_ops as optional driver configuration step.
> >
> > What do you think about both steps?
> 
> Good opinion!
> I prefer to move rte_eth_dev_check_mq_mode to eth_dev_ops as optional
> driver configure,
> The reason is that in future other eth type may also need such kind of check or
> even refine some queue number values by their own way,
> I can help review your patch after you send out.
> Thanks for your enhancing that.
> 

What about about moving sriov from rte_eth_dev_data to driver's private
structure?

Pawel