[dpdk-dev] i40e igb_uio: reset pci on process exit
Checks
Commit Message
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
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??
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??
>
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
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
>
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.
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.
>
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;
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;
>
>
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
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
>
@@ -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())