diff mbox

[dpdk-dev] i40e igb_uio: reset pci on process exit

Message ID 106841857.Z7q1jSDIte@polaris (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers

Checks

Context Check Description
ci/checkpatch warning coding style issues
ci/Intel-compilation fail apply patch file failure

Commit Message

Gregory Etelson May 24, 2017, 11:22 a.m. UTC 
  Hello,

My tests show 
if DPDK process attached to i40e VF through igb_uio 
exists without calling rte_eth_dev_close() 
then new instance attached to that VF could experience IO failures.
I came up with the following patch to ensure igb_uio will always reset PCI on process exit.
However, I would like to hear experts opinion to be sure __pci_reset_function
resets i40e VF properly.

Thank you.
Regards,
Gregory

Comments

Stephen Hemminger May 25, 2017, 6:42 p.m. UTC | #1 
On Wed, 24 May 2017 14:22:11 +0300
Gregory Etelson <gregory@weka.io> wrote:

>  
> +static int
> +igbuio_pci_release(struct uio_info *info, struct inode *inode)
> +{
> +       int ret;
> +       struct rte_uio_pci_dev *udev = info->priv;
> +       struct pci_dev *dev = udev->pdev;
> +       ret = __pci_reset_function(dev);
> +       dev_info(&dev->dev, "pci_reset_function %s \n",
> +                 ret == 0 ? "succeded" : "failed");
> +       return 0;
> +}
> +
> +

Patch in general looks ok, but:
 * no Signed-off
 * whitespace issues
 * doesn't pass kernel coding style
 * don't log on success, why log at all??
Gregory Etelson May 26, 2017, 4:30 a.m. UTC | #2 
Hello,

This patch introduces idea I would like to implement in igb_uio driver
However, I would like to get hardware experts confirmation.
If the procedure correct I'll submit proper patch 

Regards,
Gregory

On Thursday, 25 May 2017 21:42:42 IDT Stephen Hemminger wrote:
> On Wed, 24 May 2017 14:22:11 +0300
> Gregory Etelson <gregory@weka.io> wrote:
> 
> >  
> > +static int
> > +igbuio_pci_release(struct uio_info *info, struct inode *inode)
> > +{
> > +       int ret;
> > +       struct rte_uio_pci_dev *udev = info->priv;
> > +       struct pci_dev *dev = udev->pdev;
> > +       ret = __pci_reset_function(dev);
> > +       dev_info(&dev->dev, "pci_reset_function %s \n",
> > +                 ret == 0 ? "succeded" : "failed");
> > +       return 0;
> > +}
> > +
> > +
> 
> Patch in general looks ok, but:
>  * no Signed-off
>  * whitespace issues
>  * doesn't pass kernel coding style
>  * don't log on success, why log at all??
>
Shijith Thotton May 26, 2017, 6:05 a.m. UTC | #3 
On Fri, May 26, 2017 at 07:30:58AM +0300, Gregory Etelson wrote:

Hi Gregory,

The patch is useful for LiquidIO PMD as we can avoid VF FLR request to
PF. One comment inline..

[..]
> > >  
> > > +static int
> > > +igbuio_pci_release(struct uio_info *info, struct inode *inode)
> > > +{
> > > +       int ret;
> > > +       struct rte_uio_pci_dev *udev = info->priv;
> > > +       struct pci_dev *dev = udev->pdev;
> > > +       ret = __pci_reset_function(dev);

s/__pci_reset_function/pci_reset_function

> > > +       dev_info(&dev->dev, "pci_reset_function %s \n",
> > > +                 ret == 0 ? "succeded" : "failed");
> > > +       return 0;
> > > +}
[..]

Thanks,
Shijith
Gregory Etelson May 26, 2017, 6:17 a.m. UTC | #4 
Thank you.

Regards,
Gregory

On Friday, 26 May 2017 09:05:11 IDT Shijith Thotton wrote:
> On Fri, May 26, 2017 at 07:30:58AM +0300, Gregory Etelson wrote:
> 
> Hi Gregory,
> 
> The patch is useful for LiquidIO PMD as we can avoid VF FLR request to
> PF. One comment inline..
> 
> [..]
> > > >  
> > > > +static int
> > > > +igbuio_pci_release(struct uio_info *info, struct inode *inode)
> > > > +{
> > > > +       int ret;
> > > > +       struct rte_uio_pci_dev *udev = info->priv;
> > > > +       struct pci_dev *dev = udev->pdev;
> > > > +       ret = __pci_reset_function(dev);
> 
> s/__pci_reset_function/pci_reset_function
> 
> > > > +       dev_info(&dev->dev, "pci_reset_function %s \n",
> > > > +                 ret == 0 ? "succeded" : "failed");
> > > > +       return 0;
> > > > +}
> [..]
> 
> Thanks,
> Shijith
>
Stephen Hemminger May 26, 2017, 3:53 p.m. UTC | #5 
On Fri, 26 May 2017 09:17:33 +0300
Gregory Etelson <gregory@weka.io> wrote:

> Thank you.
> 
> Regards,
> Gregory
> 
> On Friday, 26 May 2017 09:05:11 IDT Shijith Thotton wrote:
> > On Fri, May 26, 2017 at 07:30:58AM +0300, Gregory Etelson wrote:
> > 
> > Hi Gregory,
> > 
> > The patch is useful for LiquidIO PMD as we can avoid VF FLR request to
> > PF. One comment inline..
> > 
> > [..]
> > > > >  
> > > > > +static int
> > > > > +igbuio_pci_release(struct uio_info *info, struct inode *inode)
> > > > > +{
> > > > > +       int ret;
> > > > > +       struct rte_uio_pci_dev *udev = info->priv;
> > > > > +       struct pci_dev *dev = udev->pdev;
> > > > > +       ret = __pci_reset_function(dev);
> > 
> > s/__pci_reset_function/pci_reset_function
> > 
> > > > > +       dev_info(&dev->dev, "pci_reset_function %s \n",
> > > > > +                 ret == 0 ? "succeded" : "failed");
> > > > > +       return 0;
> > > > > +}
> > [..]
> > 
> > Thanks,
> > Shijith
> > 
> 

What does VFIO do?

It looks like in vfio case pci_enable is held off until open and pci_disable is done
on close. There are other things that may need to be done to make close work
correctly. Like turning of msix. Also reset may not always be possible.
Gregory Etelson May 26, 2017, 4:14 p.m. UTC | #6 
I did not look into VFIO driver yet

Regards,
Gregory

On Friday, 26 May 2017 18:53:21 IDT Stephen Hemminger wrote:
> On Fri, 26 May 2017 09:17:33 +0300
> Gregory Etelson <gregory@weka.io> wrote:
> 
> > Thank you.
> > 
> > Regards,
> > Gregory
> > 
> > On Friday, 26 May 2017 09:05:11 IDT Shijith Thotton wrote:
> > > On Fri, May 26, 2017 at 07:30:58AM +0300, Gregory Etelson wrote:
> > > 
> > > Hi Gregory,
> > > 
> > > The patch is useful for LiquidIO PMD as we can avoid VF FLR request to
> > > PF. One comment inline..
> > > 
> > > [..]
> > > > > >  
> > > > > > +static int
> > > > > > +igbuio_pci_release(struct uio_info *info, struct inode *inode)
> > > > > > +{
> > > > > > +       int ret;
> > > > > > +       struct rte_uio_pci_dev *udev = info->priv;
> > > > > > +       struct pci_dev *dev = udev->pdev;
> > > > > > +       ret = __pci_reset_function(dev);
> > > 
> > > s/__pci_reset_function/pci_reset_function
> > > 
> > > > > > +       dev_info(&dev->dev, "pci_reset_function %s \n",
> > > > > > +                 ret == 0 ? "succeded" : "failed");
> > > > > > +       return 0;
> > > > > > +}
> > > [..]
> > > 
> > > Thanks,
> > > Shijith
> > > 
> > 
> 
> What does VFIO do?
> 
> It looks like in vfio case pci_enable is held off until open and pci_disable is done
> on close. There are other things that may need to be done to make close work
> correctly. Like turning of msix. Also reset may not always be possible.
>
Shijith Thotton May 29, 2017, 9:48 a.m. UTC | #7 
On Fri, May 26, 2017 at 07:14:55PM +0300, Gregory Etelson wrote:
>    I did not look into VFIO driver yet
> 
>     
> 
>    Regards,
> 
>    Gregory
> 
>     
> 
>    On Friday, 26 May 2017 18:53:21 IDT Stephen Hemminger wrote:
> 
>    > On Fri, 26 May 2017 09:17:33 +0300
> 
>    > Gregory Etelson <gregory@weka.io> wrote:
> 
>    >
> 
>    > > Thank you.
> 
>    > >
> 
>    > > Regards,
> 
>    > > Gregory
> 
>    > >
> 
>    > > On Friday, 26 May 2017 09:05:11 IDT Shijith Thotton wrote:
> 
>    > > > On Fri, May 26, 2017 at 07:30:58AM +0300, Gregory Etelson wrote:
> 
>    > > >
> 
>    > > > Hi Gregory,
> 
>    > > >
> 
>    > > > The patch is useful for LiquidIO PMD as we can avoid VF FLR request
>    to
> 
>    > > > PF. One comment inline..
> 
>    > > >
> 
>    > > > [..]
> 
>    > > > > > >
> 
>    > > > > > > +static int
> 
>    > > > > > > +igbuio_pci_release(struct uio_info *info, struct inode
>    *inode)
> 
>    > > > > > > +{
> 
>    > > > > > > + int ret;
> 
>    > > > > > > + struct rte_uio_pci_dev *udev = info->priv;
> 
>    > > > > > > + struct pci_dev *dev = udev->pdev;
> 
>    > > > > > > + ret = __pci_reset_function(dev);
> 
>    > > >
> 
>    > > > s/__pci_reset_function/pci_reset_function
> 
>    > > >
> 
>    > > > > > > + dev_info(&dev->dev, "pci_reset_function %s \n",
> 
>    > > > > > > + ret == 0 ? "succeded" : "failed");
> 
>    > > > > > > + return 0;
> 
>    > > > > > > +}
> 
>    > > > [..]
> 
>    > > >
> 
>    > > > Thanks,
> 
>    > > > Shijith
> 
>    > > >
> 
>    > >
> 
>    >
> 
>    > What does VFIO do?
> 
>    >
> 
>    > It looks like in vfio case pci_enable is held off until open and
>    pci_disable is done
> 
>    > on close. There are other things that may need to be done to make close
>    work
> 
>    > correctly. Like turning of msix. Also reset may not always be possible.
>

Better follow VFIO as Stephen advised. VFIO does pci reset inside open[1] and
tries to reset device during release[2]. 

1. elixir.free-electrons.com/linux/latest/source/drivers/vfio/pci/vfio_pci.c#L229
2. elixir.free-electrons.com/linux/latest/source/drivers/vfio/pci/vfio_pci.c#L361

static int
igbuio_pci_open(struct uio_info *info, struct inode *inode)
{
       struct rte_uio_pci_dev *udev = info->priv;
       struct pci_dev *dev = udev->pdev;

       return pci_reset_function(dev);
}

and.. 
	udev->info.open = igbuio_pci_open;
Gregory Etelson May 29, 2017, 10:01 a.m. UTC | #8 
I still have to support Red Hat 6.x. These system do not have VFIO 
IGB_UIO is the only option there.

Also, there was a discussion that claimed IGB_UIO has better performance than VFIO.
http://dpdk.org/ml/archives/dev/2014-August/004609.html

Regards,
Gregory

 
On Monday, 29 May 2017 12:48:59 IDT Shijith Thotton wrote:
> On Fri, May 26, 2017 at 07:14:55PM +0300, Gregory Etelson wrote:
> >    I did not look into VFIO driver yet
> > 
> >     
> > 
> >    Regards,
> > 
> >    Gregory
> > 
> >     
> > 
> >    On Friday, 26 May 2017 18:53:21 IDT Stephen Hemminger wrote:
> > 
> >    > On Fri, 26 May 2017 09:17:33 +0300
> > 
> >    > Gregory Etelson <gregory@weka.io> wrote:
> > 
> >    >
> > 
> >    > > Thank you.
> > 
> >    > >
> > 
> >    > > Regards,
> > 
> >    > > Gregory
> > 
> >    > >
> > 
> >    > > On Friday, 26 May 2017 09:05:11 IDT Shijith Thotton wrote:
> > 
> >    > > > On Fri, May 26, 2017 at 07:30:58AM +0300, Gregory Etelson wrote:
> > 
> >    > > >
> > 
> >    > > > Hi Gregory,
> > 
> >    > > >
> > 
> >    > > > The patch is useful for LiquidIO PMD as we can avoid VF FLR request
> >    to
> > 
> >    > > > PF. One comment inline..
> > 
> >    > > >
> > 
> >    > > > [..]
> > 
> >    > > > > > >
> > 
> >    > > > > > > +static int
> > 
> >    > > > > > > +igbuio_pci_release(struct uio_info *info, struct inode
> >    *inode)
> > 
> >    > > > > > > +{
> > 
> >    > > > > > > + int ret;
> > 
> >    > > > > > > + struct rte_uio_pci_dev *udev = info->priv;
> > 
> >    > > > > > > + struct pci_dev *dev = udev->pdev;
> > 
> >    > > > > > > + ret = __pci_reset_function(dev);
> > 
> >    > > >
> > 
> >    > > > s/__pci_reset_function/pci_reset_function
> > 
> >    > > >
> > 
> >    > > > > > > + dev_info(&dev->dev, "pci_reset_function %s \n",
> > 
> >    > > > > > > + ret == 0 ? "succeded" : "failed");
> > 
> >    > > > > > > + return 0;
> > 
> >    > > > > > > +}
> > 
> >    > > > [..]
> > 
> >    > > >
> > 
> >    > > > Thanks,
> > 
> >    > > > Shijith
> > 
> >    > > >
> > 
> >    > >
> > 
> >    >
> > 
> >    > What does VFIO do?
> > 
> >    >
> > 
> >    > It looks like in vfio case pci_enable is held off until open and
> >    pci_disable is done
> > 
> >    > on close. There are other things that may need to be done to make close
> >    work
> > 
> >    > correctly. Like turning of msix. Also reset may not always be possible.
> >
> 
> Better follow VFIO as Stephen advised. VFIO does pci reset inside open[1] and
> tries to reset device during release[2]. 
> 
> 1. elixir.free-electrons.com/linux/latest/source/drivers/vfio/pci/vfio_pci.c#L229
> 2. elixir.free-electrons.com/linux/latest/source/drivers/vfio/pci/vfio_pci.c#L361
> 
> static int
> igbuio_pci_open(struct uio_info *info, struct inode *inode)
> {
>        struct rte_uio_pci_dev *udev = info->priv;
>        struct pci_dev *dev = udev->pdev;
> 
>        return pci_reset_function(dev);
> }
> 
> and.. 
> 	udev->info.open = igbuio_pci_open;
> 
>
Shijith Thotton May 29, 2017, 11:01 a.m. UTC | #9 
On Mon, May 29, 2017 at 01:01:06PM +0300, Gregory Etelson wrote:
>    I still have to support Red Hat 6.x. These system do not have VFIO
> 
>    IGB_UIO is the only option there.
> 
>    Also, there was a discussion that claimed IGB_UIO has better performance
>    than VFIO.
> 
>    http://dpdk.org/ml/archives/dev/2014-August/004609.html
> 
>    Regards,
>    Gregory
> 

[..]
>> static int
>> igbuio_pci_open(struct uio_info *info, struct inode *inode)
>> {
>>	struct rte_uio_pci_dev *udev = info->priv;
>>	struct pci_dev *dev = udev->pdev;
>>
>>	return pci_reset_function(dev);
>> }
>>
>> and..
>> udev->info.open = igbuio_pci_open;
>>

I was suggesting to make reset part of open. It should work on your setup.

-	udev->info.release = igbuio_pci_release;
+	udev->info.open = igbuio_pci_open;

Shijith
Gregory Etelson May 29, 2017, 11:21 a.m. UTC | #10 
PMD already resets PCI during initialization.
In my patch, exiting process forced to release it's resources

On Monday, 29 May 2017 14:01:48 IDT Shijith Thotton wrote:
> On Mon, May 29, 2017 at 01:01:06PM +0300, Gregory Etelson wrote:
> >    I still have to support Red Hat 6.x. These system do not have VFIO
> > 
> >    IGB_UIO is the only option there.
> > 
> >    Also, there was a discussion that claimed IGB_UIO has better performance
> >    than VFIO.
> > 
> >    http://dpdk.org/ml/archives/dev/2014-August/004609.html
> > 
> >    Regards,
> >    Gregory
> > 
> 
> [..]
> >> static int
> >> igbuio_pci_open(struct uio_info *info, struct inode *inode)
> >> {
> >>	struct rte_uio_pci_dev *udev = info->priv;
> >>	struct pci_dev *dev = udev->pdev;
> >>
> >>	return pci_reset_function(dev);
> >> }
> >>
> >> and..
> >> udev->info.open = igbuio_pci_open;
> >>
> 
> I was suggesting to make reset part of open. It should work on your setup.
> 
> -	udev->info.release = igbuio_pci_release;
> +	udev->info.open = igbuio_pci_open;
> 
> Shijith
>
diff mbox

Patch

diff --git a/lib/librte_eal/linuxapp/igb_uio/igb_uio.c b/lib/librte_eal/linuxapp/igb_uio/igb_uio.c
index b9d427c..7d7ff9d 100644
--- a/lib/librte_eal/linuxapp/igb_uio/igb_uio.c
+++ b/lib/librte_eal/linuxapp/igb_uio/igb_uio.c
@@ -170,6 +170,19 @@  struct rte_uio_pci_dev {
        return IRQ_HANDLED;
 }
 
+static int
+igbuio_pci_release(struct uio_info *info, struct inode *inode)
+{
+       int ret;
+       struct rte_uio_pci_dev *udev = info->priv;
+       struct pci_dev *dev = udev->pdev;
+       ret = __pci_reset_function(dev);
+       dev_info(&dev->dev, "pci_reset_function %s \n",
+                 ret == 0 ? "succeded" : "failed");
+       return 0;
+}
+
+
 #ifdef CONFIG_XEN_DOM0
 static int
 igbuio_dom0_mmap_phys(struct uio_info *info, struct vm_area_struct *vma)
@@ -372,6 +385,7 @@  struct rte_uio_pci_dev {
        udev->info.version = "0.1";
        udev->info.handler = igbuio_pci_irqhandler;
        udev->info.irqcontrol = igbuio_pci_irqcontrol;
+       udev->info.release = igbuio_pci_release;
 #ifdef CONFIG_XEN_DOM0
        /* check if the driver run on Xen Dom0 */
        if (xen_initial_domain())