On 11/9/2017 5:53 PM, Mody, Rasesh wrote:
> Hi Ferruh,
>
>> From: Ferruh Yigit [mailto:[email protected]]
>> Sent: Thursday, November 09, 2017 5:07 PM
>>
>> On 11/9/2017 3:16 PM, Patil, Harish wrote:
>>> -----Original Message-----
>>> From: Ferruh Yigit <[email protected]>
>>> Date: Thursday, November 9, 2017 at 4:07 PM
>>> To: Harish Patil <[email protected]>, "Mody, Rasesh"
>>> <[email protected]>, "[email protected]" <[email protected]>,
>>> "[email protected]" <[email protected]>
>>> Cc: Dept-Eng DPDK Dev <[email protected]>
>>> Subject: Re: [dpdk-dev] [PATCH 2/2] net/qede: fix default config
>>> option
>>>
>>>> On 11/9/2017 3:00 PM, Patil, Harish wrote:
>>>>> -----Original Message-----
>>>>> From: Harish Patil <[email protected]>
>>>>> Date: Thursday, November 9, 2017 at 3:57 PM
>>>>> To: Ferruh Yigit <[email protected]>, "Mody, Rasesh"
>>>>> <[email protected]>, "[email protected]" <[email protected]>,
>>>>> "[email protected]" <[email protected]>
>>>>> Cc: Dept-Eng DPDK Dev <[email protected]>
>>>>> Subject: Re: [dpdk-dev] [PATCH 2/2] net/qede: fix default config
>>>>> option
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Ferruh Yigit <[email protected]>
>>>>>> Date: Thursday, November 9, 2017 at 3:48 PM
>>>>>> To: "Mody, Rasesh" <[email protected]>, "[email protected]"
>>>>>> <[email protected]>, "[email protected]"
>>>>>> <[email protected]>
>>>>>> Cc: Harish Patil <[email protected]>, Dept-Eng DPDK Dev
>>>>>> <[email protected]>
>>>>>> Subject: Re: [dpdk-dev] [PATCH 2/2] net/qede: fix default config
>>>>>> option
>>>>>>
>>>>>>> On 11/8/2017 10:52 PM, Rasesh Mody wrote:
>>>>>>>> From: Harish Patil <[email protected]>
>>>>>>>>
>>>>>>>> Restore the default configuration as in previous releases and add
>>>>>>>> a debug msg.
>>>>>>>
>>>>>>> Is this reverting
>>>>>>> "f07aa795c92a ("net/qede: disable per-VF Tx switching feature")"
>>>>>>>
>>>>>>> This will be same as code before f07aa795c92a , right? If so why
>>>>>>> not just remove the config option for this release and add a
>>>>>>> dynamic runtime in next release?
>>>>>>>
>>>>>>> I am not clear for both f07aa795c92a and this one fixing, and why
>>>>>>> should they be included for rc3?
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Ferruh
>>>>>>
>>>>>> Hi Ferruh,
>>>>>> Some customers are interested in getting better performance with
>>>>>> 64B sized packets. For that, they would need to keep this config
>>>>>> disabled.
>>>>>> But in all other cases, by default, we would like to keep Tx
>>>>>> switching enabled (at a reduced performance) as in previous releases.
>>>>>> As stated in other email with Thomas, we shall remove this
>>>>>> compile-time config option in next release and use runtime option
>> instead.
>>>>>> But for 17.08 we need it to be enabled by default.
>>>>>> Thanks.
>>>>>
>>>>> Correction, I meant 17.11 release, not 17.08.
>>>>
>>>> Other patch just sent two days ago, to introduce the config option as
>>>> disabled by default, so it was changing the behavior and accepted as
>>>> a fix for rc3.
>>>>
>>>> Now two days later, this patch enables it as a fix again, only
>>>> difference for two days ago becomes adding a config option?
>>>>
>>>
>>> Hi Ferruh,
>>> The patch sent two days ago with config option disabled was a mistake.
>>> Yesterday we realized that it is not a desirable thing to keep the
>>> config disabled since many customers would typically need Tx switching
>>> to be enabled.
>>
>> I am trying to say, if you want to revert back and turn to original code, why
>> not completely revert, removing config option too?
>
> We have an issue where we see low numbers with 64 byte frames and is seen
> when "per-VF Tx switching" feature is enabled (which is the default behavior
> without the config option). To get better performance with small sized
> frames, the fix is to disable the Tx switching feature. We need to have the
> config option as a hotfix as it provides a means to disable "per-VF Tx
> switching" feature and achieve better performance with small sized frames. In
> other words, it provides end user with an option to enable a feature or get
> better small packet performance.
I got what you are doing, this is about how you did it, but ok to patch, I think
already late to discus more.
> We also need this config option to be picked in DPDK 17.08 stable, which
> exhibits same behavior.
Not sure if we backport new config options to stable trees? cc'ed Yuanhan.
> Thanks!
> -Rasesh
>
>>
>> Specially taking into account that you will remove it next release already.
>>
>>> So the current patch we sent is just to change back the config option
>>> set to enable from disable. Hence its a fix for the previous patch.
>>
>> The cumulative output of both patches are adding a config option that
>> doesn't change code execution by default. Overall output is not a fix.
>> What do you think about a full revert of previous patch?
>>
>>> Thanks.
>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>> Fixes: f07aa795c92a ("net/qede: disable per-VF Tx switching
>>>>>>>> feature")
>>>>>>>>
>>>>>>>> Signed-off-by: Harish Patil <[email protected]>
>>>>>>>> Signed-off-by: Rasesh Mody <[email protected]>
>>>>>>>> ---
>>>>>>>> config/common_base | 2 +-
>>>>>>>> drivers/net/qede/qede_ethdev.c | 5 +++--
>>>>>>>> 2 files changed, 4 insertions(+), 3 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/config/common_base b/config/common_base index
>>>>>>>> 34f04a9..e74febe 100644
>>>>>>>> --- a/config/common_base
>>>>>>>> +++ b/config/common_base
>>>>>>>> @@ -415,7 +415,7 @@ CONFIG_RTE_LIBRTE_QEDE_DEBUG_INFO=n
>>>>>>>> CONFIG_RTE_LIBRTE_QEDE_DEBUG_DRIVER=n
>>>>>>>> CONFIG_RTE_LIBRTE_QEDE_DEBUG_TX=n
>>>>>>>> CONFIG_RTE_LIBRTE_QEDE_DEBUG_RX=n
>>>>>>>> -CONFIG_RTE_LIBRTE_QEDE_VF_TX_SWITCH=n
>>>>>>>> +CONFIG_RTE_LIBRTE_QEDE_VF_TX_SWITCH=y
>>>>>>>> #Provides abs path/name of the firmware file.
>>>>>>>> #Empty string denotes driver will use default firmware
>>>>>>>> CONFIG_RTE_LIBRTE_QEDE_FW=""
>>>>>>>> diff --git a/drivers/net/qede/qede_ethdev.c
>>>>>>>> b/drivers/net/qede/qede_ethdev.c index 8832145..6f5ba2a 100644
>>>>>>>> --- a/drivers/net/qede/qede_ethdev.c
>>>>>>>> +++ b/drivers/net/qede/qede_ethdev.c
>>>>>>>> @@ -457,6 +457,7 @@ int qede_activate_vport(struct rte_eth_dev
>>>>>>>> *eth_dev, bool flg)
>>>>>>>> if (IS_VF(edev)) {
>>>>>>>> params.update_tx_switching_flg = 1;
>>>>>>>> params.tx_switching_flg = !flg;
>>>>>>>> + DP_INFO(edev, "VF tx-switching is disabled\n");
>>>>>>>> }
>>>>>>>> #endif
>>>>>>>> for_each_hwfn(edev, i) {
>>>>>>>> @@ -469,8 +470,8 @@ int qede_activate_vport(struct rte_eth_dev
>>>>>>>> *eth_dev, bool flg)
>>>>>>>> break;
>>>>>>>> }
>>>>>>>> }
>>>>>>>> - DP_INFO(edev, "vport %s VF tx-switch %s\n", flg ?
>> "activated" :
>>>>>>>> "deactivated",
>>>>>>>> - params.tx_switching_flg ? "enabled" :
>> "disabled");
>>>>>>>> + DP_INFO(edev, "vport is %s\n", flg ? "activated" :
>>>>>>>> +"deactivated");
>>>>>>>> +
>>>>>>>> return rc;
>>>>>>>> }
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>
