Re: [dpdk-dev] [PATCH] igb_uio: remove device reset in open

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

On 11/2/2017 10:34 AM, Mody, Rasesh wrote:
>> From: Ferruh Yigit [mailto:ferruh.yigit@intel.com]
>> Sent: Thursday, November 02, 2017 1:55 AM
>>
>> On 11/2/2017 1:03 AM, Mody, Rasesh wrote:
>>>> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
>>>> Sent: Wednesday, November 01, 2017 7:12 AM
>>>>
>>>> On Wed, 1 Nov 2017 06:58:53 +0000
>>>> "Mody, Rasesh" <Rasesh.Mody@cavium.com> wrote:
>>>>
>>>>> Hi Jianfeng and Ferruh,
>>>>>
>>>>>> From: Ferruh Yigit [mailto:ferruh.yigit@intel.com]
>>>>>> Sent: Thursday, October 26, 2017 5:50 PM
>>>>>>
>>>>>> On 10/26/2017 2:28 AM, Tan, Jianfeng wrote:
>>>>>>> Hi Rasesh,
>>>>>>>
>>>>>>>
>>>>>>> On 10/26/2017 7:43 AM, Mody, Rasesh wrote:
>>>>>>>> Hi Ferruh,
>>>>>>>>
>>>>>>>>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ferruh
>>>>>>>>> Yigit
>>>>>>>>> Sent: Friday, October 20, 2017 9:58 AM
>>>>>>>>>
>>>>>>>>> On 10/20/2017 9:55 AM, Ferruh Yigit wrote:
>>>>>>>>>> Remove device reset during application start, the reset for
>>>>>>>>>> application exit still there.
>>>>>>>>>>
>>>>>>>>>> Reset in open removed because of following comments:
>>>>>>>>>> 1- Device reset not completed when VF driver loaded, which
>>>>>>>>>> cause VF
>>>>>> PMD
>>>>>>>>>>     initialization error.
>>>>>>>>>>     Adding delay can solve the issue but will increase driver load
>> time.
>>>>>>>>>>
>>>>>>>>>> 2- Reset will be issues all devices unconditionally, not very
>> efficient
>>>>>>>>>>     way.
>>>>>>>>>>
>>>>>>>>>> Fixes: b58eedfc7dd5 ("igb_uio: issue FLR during open and
>>>>>>>>>> release of device file")
>>>>>>>>>> Cc: stable@dpdk.org
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
>>>>>>>>> Hi Jingjing, Shijith, Gregory, Harish,
>>>>>>>>>
>>>>>>>>> Can you please test this on top of current master (which has
>>>>>>>>> already Jingjin's
>>>>>>>>> fix) ?
>>>>>>>> The original FLR change during igb_uio open()/release() in
>>>>>>>> DPDK17.08 also
>>>>>> impacts BNX2X PMD and it exhibits the issues with bare metal testing.
>>>>>>>>
>>>>>>>> Now, we tested this change for BNX2X PMD using latest dpdk, which
>>>>>>>> has
>>>>>> this fix where FLR is invoked only in the release().
>>>>>>
>>>>>> Good to hear this fixed the problem.
>>>>>
>>>>> Yes, it fixed the issue caused by pci reset during application start.
>>>>>
>>>>>>
>>>>>>>> However, we ran into an issue when trying to reload the testpmd
>>>>>> application in quick succession. The pci reset, called during the
>>>>>> igb_uio
>>>>>> release() operation, is taking longer time and adapter is still
>>>>>> doing the FLR when we relaunch the application. We see this
>>>>>> behavior with bare metal testing.
>>>>>>>
>>>>>>> If we don't reset that device, it will continue working which is a
>>>>>>> more serious issue IMO.
>>>>>>
>>>>>> +1
>>>>>
>>>>> I think, it would better for the individual PMDs to take care of the
>>>>> reset
>>>> during the application exit.
>>>>
>>>> That will never be possible. Poll Mode Drivers are userspace entities
>>>> and part of the application. If application crashes, there is no way
>>>> for PMD to do cleanup, it must be handled by kernel.
>>>
>>> The pci reset in release is breaking the BNX2X PMD. Could we revert this
>> reset and get it included with a solution that works for all in the next release?
>>
>> Hi Rasesh,
>>
>> I am not sure if there is more to do for solution for next releases, and related
>> to your case, indeed I wasn't expecting a device reset will take more than
>> five minutes...
>>
>> Would you be OK to control the reset via a compile time config option, which
>> is enabled by default. So you will need to disable it to prevent the reset?
> 
> Hi Ferruh,
> 
> As I understand, we will have a compile time config option, enabled by default, to guard the pci_reset_function() in the igbuio_pci_release(). We will disable this config option to prevent the reset when using BNX2X.

Yep, this is the idea.

> The controlled reset should work for us.

If there is no objection, I can send a patch for this.

> 
> Thanks!
> -Rasesh
>