From: "Tan, Jianfeng" <jianfeng.tan@intel.com>
To: Ferruh Yigit <ferruh.yigit@intel.com>,
"Patil, Harish" <Harish.Patil@cavium.com>,
Thomas Monjalon <thomas@monjalon.net>
Cc: "dev@dpdk.org" <dev@dpdk.org>,
Jingjing Wu <jingjing.wu@intel.com>,
"Thotton, Shijith" <Shijith.Thotton@cavium.com>,
Gregory Etelson <gregory@weka.io>,
George Prekas <george.prekas@epfl.ch>,
"stable@dpdk.org" <stable@dpdk.org>,
Sergio Gonzalez Monroy <sergio.gonzalez.monroy@intel.com>,
"Glynn, Michael J" <michael.j.glynn@intel.com>
Subject: Re: [dpdk-stable] [PATCH] igb_uio: revert open and release operations
Date: Fri, 20 Oct 2017 23:26:15 +0800 [thread overview]
Message-ID: <1f37e107-eb01-3a23-04a5-ba4c6e60d63d@intel.com> (raw)
In-Reply-To: <a95f1017-8096-2d98-1261-14983053f949@intel.com>
Hi Ferruh & Harish,
On 10/20/2017 9:15 AM, Ferruh Yigit wrote:
> On 10/19/2017 3:43 PM, Patil, Harish wrote:
>> -----Original Message-----
>> From: Harish Patil <Harish.Patil@cavium.com>
>> Date: Tuesday, October 17, 2017 at 9:50 PM
>> To: Thomas Monjalon <thomas@monjalon.net>, Ferruh Yigit
>> <ferruh.yigit@intel.com>
>> Cc: "dev@dpdk.org" <dev@dpdk.org>, Jianfeng Tan <jianfeng.tan@intel.com>,
>> Jingjing Wu <jingjing.wu@intel.com>, "Thotton, Shijith"
>> <Shijith.Thotton@cavium.com>, Gregory Etelson <gregory@weka.io>, George
>> Prekas <george.prekas@epfl.ch>, "stable@dpdk.org" <stable@dpdk.org>
>> Subject: Re: [PATCH] igb_uio: revert open and release operations
>>>
>>> -----Original Message-----
>>> From: Thomas Monjalon <thomas@monjalon.net>
>>> Date: Tuesday, October 17, 2017 at 1:33 PM
>>> To: Ferruh Yigit <ferruh.yigit@intel.com>, Harish Patil
>>> <Harish.Patil@cavium.com>
>>> Cc: "dev@dpdk.org" <dev@dpdk.org>, Jianfeng Tan <jianfeng.tan@intel.com>,
>>> Jingjing Wu <jingjing.wu@intel.com>, "Thotton, Shijith"
>>> <Shijith.Thotton@cavium.com>, Gregory Etelson <gregory@weka.io>, George
>>> Prekas <george.prekas@epfl.ch>, "stable@dpdk.org" <stable@dpdk.org>
>>> Subject: Re: [PATCH] igb_uio: revert open and release operations
>>>
>>>> 17/10/2017 22:14, Ferruh Yigit:
>>>>> There were bug reports about terminated application may leave device in
>>>>> undesired state:
>>>>> http://dpdk.org/ml/archives/dev/2016-November/049745.html
>>>>> http://dpdk.org/ml/archives/dev/2016-November/050932.html
>>>>>
>>>>> And a proposal to fix:
>>>>> http://dpdk.org/ml/archives/dev/2016-December/051844.html
>>>>>
>>>>> Later another proposal triggered the discussion:
>>>>> http://dpdk.org/ml/archives/dev/2017-May/066317.html
>>>>>
>>>>> Finally a fix patch pushed into v17.08:
>>>>> Commit: b58eedfc7dd5 ("igb_uio: issue FLR during open and release of
>>>>> device file")
>>>>>
>>>>> Later a regression report sent related to the pushed patch:
>>>>> http://dpdk.org/ml/archives/dev/2017-September/075236.html
>>>>>
>>>>> And a fix for regression integrated into v17.11-rc1:
>>>>> http://dpdk.org/ml/archives/dev/2017-October/079166.html
>>>>> Commit: 5f6ff30dc507 ("igb_uio: fix interrupt enablement after FLR in
>>>>> VM")
>>>>> Commit: 6b9ed026a870 ("igb_uio: fix build with kernel <= 3.17")
>>>>>
>>>>> Even after the fix qede PMD reported to be broken:
>>>>> http://dpdk.org/ml/archives/dev/2017-October/079359.html
>>>>>
>>>>> So this patch reverts original fix and related commits. The related
>>>>> igb_uio code part turns back to v17.05 base.
>>>> [...]
>>>>> ---
>>>>> It would be nice to solve this issue in LTS release, but being close to
>>>>> the release and the error report without details makes it hard to work
>>>>> more on this issue.
>>>> With this revert, we are back to the original issue.
>>>> We must really try the proposed solution.
>>>> Harish, please describe your issue and think how it could be fixed.
>>>> Jingjing made it work for i40e.
>>>> I know it is less effort to request a simple revert.
>>>> Please let's try to fix it once for all.
>>> Hi Ferruh/Thomas,
>>> I’m discussing it internally, so please hold on committing this patch till
>>> I revert back to you.
>>>
>>> Thanks.
>>> [Harish]
>>> 1) With the introduction of:
>>> Commit: b58eedfc7dd5 ("igb_uio: issue FLR during open and release of
>>> device file”)
>> We saw failures with qede PF & SR-IOV VF initialization in PCI passthru
>> mode.
>>
>>> PF PCI passthru mode initialization failure was resolved by:
>>> “Commit: 5f6ff30dc507 ("igb_uio: fix interrupt enablement after FLR in
>>> VM")
> Thank you for the update.
>
>>> SR-IOV VF PCI passthru mode initialization issue is that PCI FLR and
>>> related device cleanup is not completed by the time VF driver starts
>>> loading. It results in the mbox command failure sent over the HW channel
>>> between VF and PF.
> This seems same reason why i40e added a check and wait loop.
>
>>> Even though pci_reset_function() waits for the stipulated amount of time
>>> per standards, VF FLR takes longer than that and pci_reset_function() &
>>> igb_uio_open() call returns before FLR completes and VF PMD driver tries
>>> to load before FLR completes leading to VF PMD initialization failure.
>>>
>>>
>>> We can work around this problem by adding driver delay/retry logic since
>>> there is no deterministic way of detecting FLR completion. But this is
>>> going to increase the driver load time.
>>>
>>> 2) With the above patch ("igb_uio: issue FLR during open and release of
>>> device file), FLR is going to be issued unconditionally on all devices
>>> during igb_uio_open. We think it’s an over kill. FLR is required only for
>>> previous abnormal app termination. We already handle the abnormal app
>>> termination by doing necessary cleanup in the driver during load. This
>>> cleanup is more efficient as it is done only when required. So we feel
>>> that the drivers/devices needing such cleanup (the two cases listed
>>> below) should do it conditionally when required rather than
>>> igb_uio_open() unconditionally performing FLR all the time.
>>>
>>> e.g.,
>> - cdb166963cae ("net/liquidio: add API for VF FLR”)
> Both 1) and 2) related to the pci_reset during open().
> But the main functionality we are looking for is the pci_reset in release().
> So we can remove reset during open() [1].
>
> Will disabling pci_reset in open() [2] solve your problems?
>
> Jingjing, Jianfeng,
> What do you think?
If [2] can work for all drivers, I agree with this option.
Thanks,
Jianfeng
>
>
> [1]
> Perhaps will need to revert or partially revert liquidio patch after this.
>
> [2]
> disable line "pci_reset_function(dev);" in the igbuio_pci_open()
> http://dpdk.org/browse/dpdk/tree/lib/librte_eal/linuxapp/igb_uio/igb_uio.c#n339
>
next prev parent reply other threads:[~2017-10-20 15:26 UTC|newest]
Thread overview: 32+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-10-17 20:14 Ferruh Yigit
2017-10-17 20:33 ` Thomas Monjalon
2017-10-18 4:50 ` Patil, Harish
2017-10-19 22:43 ` Patil, Harish
2017-10-20 1:15 ` Ferruh Yigit
2017-10-20 15:26 ` Tan, Jianfeng [this message]
2017-10-20 16:32 ` Ferruh Yigit
2017-10-20 16:55 ` [dpdk-stable] [PATCH] igb_uio: remove device reset in open Ferruh Yigit
2017-10-20 16:57 ` Ferruh Yigit
2017-10-20 19:01 ` Gregory Etelson
2017-10-20 22:18 ` Patil, Harish
2017-10-23 12:28 ` Shijith Thotton
2017-10-23 16:36 ` Ferruh Yigit
2017-10-23 19:03 ` Shijith Thotton
2017-10-24 2:45 ` Wu, Jingjing
2017-10-25 23:43 ` [dpdk-stable] [dpdk-dev] " Mody, Rasesh
2017-10-26 9:28 ` Tan, Jianfeng
2017-10-27 0:49 ` Ferruh Yigit
2017-11-01 6:58 ` Mody, Rasesh
2017-11-01 14:12 ` Stephen Hemminger
2017-11-02 8:03 ` Mody, Rasesh
2017-11-02 8:55 ` Ferruh Yigit
2017-11-02 17:34 ` Mody, Rasesh
2017-11-02 18:09 ` Ferruh Yigit
2017-11-02 18:45 ` Mody, Rasesh
2017-11-03 0:31 ` Ferruh Yigit
2017-11-03 19:18 ` Mody, Rasesh
2017-10-24 21:38 ` [dpdk-stable] " Ferruh Yigit
2017-10-18 0:14 ` [dpdk-stable] [PATCH] igb_uio: revert open and release operations Wu, Jingjing
2017-10-18 6:27 ` Shijith Thotton
2017-10-18 20:47 ` Ferruh Yigit
2017-10-24 21:32 ` Ferruh Yigit
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1f37e107-eb01-3a23-04a5-ba4c6e60d63d@intel.com \
--to=jianfeng.tan@intel.com \
--cc=Harish.Patil@cavium.com \
--cc=Shijith.Thotton@cavium.com \
--cc=dev@dpdk.org \
--cc=ferruh.yigit@intel.com \
--cc=george.prekas@epfl.ch \
--cc=gregory@weka.io \
--cc=jingjing.wu@intel.com \
--cc=michael.j.glynn@intel.com \
--cc=sergio.gonzalez.monroy@intel.com \
--cc=stable@dpdk.org \
--cc=thomas@monjalon.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).