Re: [dpdk-dev] [PATCH v4 6/6] testpmd: Set Rx VMDq RSS mode

["Ouyang, Changchun" <changchun.ouyang@intel.com>]

> -----Original Message-----
> From: Vlad Zolotarov [mailto:vladz@cloudius-systems.com]
> Sent: Tuesday, January 6, 2015 8:53 PM
> To: Ouyang, Changchun; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v4 6/6] testpmd: Set Rx VMDq RSS mode
> 
> 
> On 01/06/15 04:01, Ouyang, Changchun wrote:
> >
> >> -----Original Message-----
> >> From: Vlad Zolotarov [mailto:vladz@cloudius-systems.com]
> >> Sent: Monday, January 5, 2015 6:12 PM
> >> To: Ouyang, Changchun; dev@dpdk.org
> >> Subject: Re: [dpdk-dev] [PATCH v4 6/6] testpmd: Set Rx VMDq RSS mode
> >>
> >>
> >> On 01/05/15 04:38, Ouyang, Changchun wrote:
> >>>> -----Original Message-----
> >>>> From: Vlad Zolotarov [mailto:vladz@cloudius-systems.com]
> >>>> Sent: Sunday, January 4, 2015 5:47 PM
> >>>> To: Ouyang, Changchun; dev@dpdk.org
> >>>> Subject: Re: [dpdk-dev] [PATCH v4 6/6] testpmd: Set Rx VMDq RSS
> >>>> mode
> >>>>
> >>>>
> >>>> On 01/04/15 11:01, Ouyang, Changchun wrote:
> >>>>>> -----Original Message-----
> >>>>>> From: Vlad Zolotarov [mailto:vladz@cloudius-systems.com]
> >>>>>> Sent: Sunday, January 4, 2015 4:50 PM
> >>>>>> To: Ouyang, Changchun; dev@dpdk.org
> >>>>>> Subject: Re: [dpdk-dev] [PATCH v4 6/6] testpmd: Set Rx VMDq RSS
> >>>>>> mode
> >>>>>>
> >>>>>>
> >>>>>> On 01/04/15 09:18, Ouyang Changchun wrote:
> >>>>>>> Set VMDq RSS mode if it has VF(VF number is more than 1) and has
> >>>>>>> RSS
> >>>>>> information.
> >>>>>>> Signed-off-by: Changchun Ouyang <changchun.ouyang@intel.com>
> >>>>>>> ---
> >>>>>>>      app/test-pmd/testpmd.c | 10 ++++++++++
> >>>>>>>      1 file changed, 10 insertions(+)
> >>>>>>>
> >>>>>>> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
> >>>>>>> index 8c69756..6230f8b 100644
> >>>>>>> --- a/app/test-pmd/testpmd.c
> >>>>>>> +++ b/app/test-pmd/testpmd.c
> >>>>>>> @@ -1708,6 +1708,16 @@ init_port_config(void)
> >>>>>>>      				port->dev_conf.rxmode.mq_mode =
> >>>>>> ETH_MQ_RX_NONE;
> >>>>>>>      		}
> >>>>>>>
> >>>>>>> +		if (port->dev_info.max_vfs != 0) {
> >>>>>>> +			if (port-
> >>> dev_conf.rx_adv_conf.rss_conf.rss_hf != 0)
> >>>>>>> +				port->dev_conf.rxmode.mq_mode =
> >>>>>>> +					ETH_MQ_RX_VMDQ_RSS;
> >>>>>>> +			else {
> >>>>>>> +				port->dev_conf.rxmode.mq_mode =
> >>>>>> ETH_MQ_RX_NONE;
> >>>>>>> +				port->dev_conf.txmode.mq_mode =
> >>>>>> ETH_MQ_TX_NONE;
> >>>>>>
> >>>>>> And what about the txmode.mq_mode when RSS is available
> (the :if"
> >>>> clause)?
> >>>>> I think we can keep its original value for txmode.mq_mode, so
> >>>>> don't
> >>>> change its value. How do you think of it?
> >>>>
> >>>> I agree that not changing a Tx mq_mode in both cases would be better.
> >>> In the else clause, set txmode.mq_mode as ETH_MQ_TX_NONE
> explicitly
> >> to
> >>> make sure it is neither ETH_MQ_TX_DCB, ETH_MQ_TX_VMDQ_DCB, nor
> >> ETH_MQ_TX_VMDQ_ONLY.
> >>
> >> It's not obvious to me why u should do that since AFAIK any of these
> >> modes requires RX_RSS. Do I miss anything?
> > No, I don't think so, in the else clause, it doesn't need rx_rss, and
> > no way to do it, because the case is there is no rss configuration
> information(note: in the else clause, dev_conf.rx_adv_conf.rss_conf.rss_hf
> == 0).
> >
> > So ETH_MQ_RX_NONE for rx_mode, and ETH_MQ_TX_NONE for tx_mode.
> 
> Of course, however, in general, one may ask, why u configure TX MQ mode
> in "else" clause an don't do it in the "if" one. Possibly the "if" case in TX MQ
> context has been handled elsewhere but this is what makes this code
> confusing: to make it the most readable u'd rather configure the same
> feature set in both "if" and "else".
> For instance:
> 
> if (bla-bla) {
>    tx_mode = X1;
>    rx_mode = X2;
> } else {
>   tx_mode = Y1;
>   rx_mode = Y2;
> }
> 
> Look at the non-SR-IOV clause right above the "if-else" block u've added.
> Why don't they configure tx_mode there? Is it a bug in their code?

It also makes sense,  I will add  tx_mode = ETH_MQ_TX_NONE as no rss for tx mode,
Rss only for rx mode.

> By the way, u forgot to fix the remark below
> 
> /* In SR-IOV mode, RSS mode is not available */
> 
> which is located a few lines above the code u've added. ;)

Sorry, I missed these few lines before, I will remove them in v5. 

Thanks
Changchun