Re: [dpdk-dev] [Linuxarm] Re: [PATCH 1/3] app/testpmd: fix forwarding configuration when DCB test

[Ferruh Yigit <ferruh.yigit@intel.com>]

On 4/10/2021 3:58 AM, oulijun wrote:
> 
> 
> 在 2021/4/10 8:56, Ferruh Yigit 写道:
>> On 3/25/2021 2:21 AM, Li, Xiaoyun wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: oulijun <oulijun@huawei.com>
>>>> Sent: Wednesday, March 24, 2021 21:40
>>>> To: linuxarm@openeuler.org; Li, Xiaoyun <xiaoyun.li@intel.com>; dev
>>>> <dev@dpdk.org>
>>>> Subject: Re: [Linuxarm] Re: [PATCH 1/3] app/testpmd: fix forwarding
>>>> configuration when DCB test
>>>>
>>>>
>>>>
>>>> 在 2021/3/24 10:03, Li, Xiaoyun 写道:
>>>>>
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: oulijun <oulijun@huawei.com>
>>>>>> Sent: Tuesday, March 23, 2021 22:19
>>>>>> To: Li, Xiaoyun <xiaoyun.li@intel.com>; Yigit, Ferruh
>>>>>> <ferruh.yigit@intel.com>
>>>>>> Cc: dev@dpdk.org; linuxarm@openeuler.org
>>>>>> Subject: Re: [PATCH 1/3] app/testpmd: fix forwarding configuration
>>>>>> when DCB test
>>>>>>
>>>>> <snip>
>>>>>>>> @@ -2707,14 +2707,16 @@ stop_port(portid_t pid)
>>>>>>>>         portid_t peer_pl[RTE_MAX_ETHPORTS];
>>>>>>>>         int peer_pi;
>>>>>>>>
>>>>>>>> -    if (dcb_test) {
>>>>>>>> -        dcb_test = 0;
>>>>>>>> -        dcb_config = 0;
>>>>>>>> -    }
>>>>>>>> -
>>>>>>>>         if (port_id_is_invalid(pid, ENABLED_WARN))
>>>>>>>>             return;
>>>>>>>>
>>>>>>>> +    /*
>>>>>>>> +     * In "start_port" function, dcb_test is set to 1 based on dcb_config.
>>>>>>>> +     * So it should be cleared when dcb_config is 0.
>>>>>>>> +     */
>>>>>>>> +    if (dcb_config == 0)
>>>>>>>> +        dcb_test = 0;
>>>>>>>> +
>>>>>>>
>>>>>>> I don't understand why are you changing this.
>>>>>>> dcb_test will only be set when dcb_config is 1 when starting ports.
>>>>>>> And both
>>>>>> dcb_test and dcb_config will be cleared when stopping ports.
>>>>>>> So dcb will only affect when you set port dcb and then start port
>>>>>>> and when
>>>>>> stop port dcb will be cleared.
>>>>>>>
>>>>>> Yes, I think. The forwarding streams should not be changed from
>>>>>> "dcb_fwd_config_setup" to "rss_fwd_config_setup" after dcb info is
>>>> configured.
>>>>>> But, now, the logical codes do it when stopping ports and then starting 
>>>>>> ports.
>>>>>>> So what's the problem of original codes?
>>>>>>>
>>>>>>> Your change will cause issues that there's no place to set
>>>>>>> dcb_config as 0. If
>>>>>> you config dcb, then it'll be always dcb mode unless restart the whole
>>>> testpmd.
>>>>>>>
>>>>>> As far as I know, the current testpmd only supports switching from
>>>>>> the normal mode to the dcb mode, but does not support the reverse
>>>> operation.
>>>>>> And " dcb_config" is set to 1, and then "dcb_test" is set to 1 after
>>>>>> config.
>>>>>
>>>>> You're not answering my questions. Why are you changing the behavior of
>>>> testpmd?
>>>>> Your change will make testpmd stay dcb mode once set dcb mode and can't go
>>>> back to normal mode. If users want to go back to normal mode, he/she has to
>>>> restart the whole testpmd.
>>>>>
>>>> Yes. Testpmd and PMD driver are both in dcb mode after dcb info is configured.
>>>> In my opinion, the 'dcb_test' flag can't be clear to go back to normal mode 
>>>> after
>>>> stopping port and then starting port. Because PMD driver is still dcb mode. If
>>>> users want to go back it, users should disable dcb mode and enable RSS or other
>>>> mode.
>>>>
>>>>> It worked as you can set dcb mode and start port. After stopping port, if you
>>>> still want dcb mode, you need to set dcb mode command again. But at least the
>>>> old way won't break anything.
>>>>> @Yigit, Ferruh Not sure which behavior is better, what do you think?
>>>>>
>>>>> And @oulijun can you just answer all comments in one thread?
>>>>>
>>>> After stopping port, the 'dcb_test' flag is clear. At this moment, the dcb
>>>> configuration in testpmd has not be changed.  users may not understand why
>>>> the DCB mode needs to be reconfigured.
>>>
>>> OK. You're right. There's no place writing port->dcb_flag back to 0.
>>>
>>
>> If there is no way to turn off the DCB, isn't this code logically dead? This 
>> code is run when "dcb_config == 0" but in that case 'dcb_test' is already 0.
>> If so what is the point of the code, can it be removed?
>>
> yes. Currently, it is a dead logic unless testpmd adds support for switching 
> from DCB mode to other modes.
> I think dcb_test is a flag at the packet forwarding layer in testpmd. The 
> original purpose is to indicate that the current forwarding test is being 
> performed in DCB mode.

can 'dcb_config' be sufficient on its own for this? so that we can remove 
'dcb_test' and simplify the logic.

>> btw, do you know what 'dcb_test' does at all? It seems it is set when 
>> 'dcb_config' set, and reset when 'dcb_config' reset.
>>
> In my opinion, it is only a global flag. During a DCB test, DCB must be 
> configured on all forwarding ports.
>>>>>>>>         printf("Stopping ports...\n");
>>>>>>>>
>>>>>>>>         RTE_ETH_FOREACH_DEV(pi) {
>>>>>>>> -- 
>>>>>>>> 2.7.4
>>>>>>>
>>>>>>> .
>>>>>>>
>>>>> _______________________________________________
>>>>> Linuxarm mailing list -- linuxarm@openeuler.org To unsubscribe send an
>>>>> email to linuxarm-leave@openeuler.org
>>>>>
>>
>> .
>>
>