Re: [dpdk-dev] [PATCH] net/i40e: force promiscuous state after VF reset

["Eelco Chaudron" <echaudro@redhat.com>]

On 1 Nov 2019, at 3:38, Xing, Beilei wrote:

>> -----Original Message-----
>> From: Eelco Chaudron [mailto:echaudro@redhat.com]
>> Sent: Tuesday, October 29, 2019 3:47 PM
>> To: Zhang, Xiao <xiao.zhang@intel.com>; Xing, Beilei 
>> <beilei.xing@intel.com>
>> Cc: Zhang, Qi Z <qi.z.zhang@intel.com>; dev@dpdk.org
>> Subject: Re: [dpdk-dev] [PATCH] net/i40e: force promiscuous state 
>> after VF
>> reset
>>
>>
>>
>> On 29 Oct 2019, at 8:39, Zhang, Xiao wrote:
>>
>>>> -----Original Message-----
>>>> From: Eelco Chaudron <echaudro@redhat.com>
>>>> Sent: Friday, October 25, 2019 5:24 PM
>>>> To: Xing, Beilei <beilei.xing@intel.com>; Zhang, Xiao
>>>> <xiao.zhang@intel.com>
>>>> Cc: Zhang, Qi Z <qi.z.zhang@intel.com>; dev@dpdk.org
>>>> Subject: Re: [dpdk-dev] [PATCH] net/i40e: force promiscuous state
>>>> after VF reset
>>>>
>>>>
>>>>
>>>> On 17 Oct 2019, at 8:34, Xing, Beilei wrote:
>>>>
>>>>>> On 17 Sep 2019, at 9:40, Eelco Chaudron wrote:
>>>>>>
>>>>>>> Even though the device reset is successful, disabling 
>>>>>>> promiscuous
>>>>>>> mode might not always succeed, causing enabling it after reset 
>>>>>>> to
>>>>>>> fail.
>>>>>>> This would happen when the kernel driver requires a reset of the
>>>>>>> VF.
>>>>>>>
>>>>>>> This patch resets the internal state, so next time promiscuous
>>>>>>> mode
>>>>>>> is configured it will be enabled.
>>>>>>>
>>>>>>> Signed-off-by: Eelco Chaudron <echaudro@redhat.com>
>>>>>>> ---
>>>>>>>  drivers/net/i40e/i40e_ethdev_vf.c | 10 ++++++++++
>>>>>>>  1 file changed, 10 insertions(+)
>>>>>>>
>>>>>>> diff --git a/drivers/net/i40e/i40e_ethdev_vf.c
>>>>>>> b/drivers/net/i40e/i40e_ethdev_vf.c
>>>>>>> index 551f6fa..e0f99a4 100644
>>>>>>> --- a/drivers/net/i40e/i40e_ethdev_vf.c
>>>>>>> +++ b/drivers/net/i40e/i40e_ethdev_vf.c
>>>>>>> @@ -2276,11 +2276,21 @@ static int eth_i40evf_pci_remove(struct
>>>>>>> rte_pci_device *pci_dev)  i40evf_dev_reset(struct rte_eth_dev
>>>>>>> *dev)
>>>>>>> {
>>>>>>>  	int ret;
>>>>>>> +	struct i40e_vf *vf =
>>>>>>> I40EVF_DEV_PRIVATE_TO_VF(dev->data->dev_private);
>>>>>>>
>>>>>>>  	ret = i40evf_dev_uninit(dev);
>>>>>>>  	if (ret)
>>>>>>>  		return ret;
>>>>>>>
>>>>>>> +	/*
>>>>>>> +	 * Even though the device reset is successful disabling
>>>>>>> promiscuous
>>>>>>> +	 * mode might not always succeed, causing enabling it after
>>>>>>> reset
>>>>>>> to
>>>>>
>>>>> I think we need to root cause why fail to disable promiscuous mode
>>>>> and
>>>>> try to fix it first.
>>>>
>>>> I’ve copied in Xiao who helped me identify the issue in your
>>>> driver.
>>>>
>>>> The issue is because the change from kernel pf was not synced to 
>>>> DPDK
>>>> vf during
>>>> the closing period of reset, so we get this failure. Xiao can you 
>>>> add
>>>> more details?
>>>>
>>>
>>> The root cause is when kernel pf generate DPDK vf reset event, the
>>> flag
>>> vf->promisc_unicast_enabled will not be cleared but promiscuous mode
>>> is
>>> disabled. When trying to enable promiscuous mode next time by 
>>> calling
>>> i40evf_dev_promiscuous_enable won't work because it will check the
>>> vf->promisc_unicast_enabled flag first.
>>>
>>> static int
>>> i40evf_dev_promiscuous_enable(struct rte_eth_dev *dev)
>>> {
>>> ...
>>>     /* If enabled, just return */
>>>     if (vf->promisc_unicast_enabled)
>>>         return 0;
>>> ...
>>> }
>>>
>>> Hi Eelco,
>>>
>>> I think you may need add more detailed message in the commit log or
>>> comments.
>>
>> My interpretation of the request was that Beilei wanted to know why
>> disabling promiscuous mode in HW was failing. Beilei can you comment, 
>> is
>> the additional description from Xiao enough?
>
> Yes, promisc_unicast_enabled flag is not cleared during vf reset 
> because fail to disable promiscuous mode,
> So I think we need to root cause why fail to  disable promiscuous mode 
> first.
> This patch looks like a workaround but not a fix.
>

This was debugged together with Xiao and from what I understand is that 
DPDK fails to reset promiscuous mode in hardware as PF and VF operations 
are not synced between kernel and DPDK.

Xiao told me this could not be fixed in another way, Xiao can you 
comment?

>>
>>>>
>>>>>>> +	 * fail. This would happen when the kernel driver requires a
>>>>>>> reset
>>>>>>> +	 * of the VF.
>>>>>>> +	 */
>>>>>>> +	if (rte_eal_process_type() == RTE_PROC_PRIMARY)
>>>>>>> +		vf->promisc_unicast_enabled = FALSE;
>>>>>>> +
>>>>>>>  	ret = i40evf_dev_init(dev);
>>>>>>>
>>>>>>>  	return ret;
>>>>>>> --
>>>>>>> 1.8.3.1