| Message ID | 20150415095934.09966367@urahara (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Apr 15, 2015 at 09:59:34AM -0700, Stephen Hemminger wrote: > The driver already supported INTX interrupts but had no in kernel > function to enable and disable them. > > It is possible for userspace to do this by accessing PCI config > directly, but this racy How is it racy? We have userspace using this interface, if there's a race I want to fix it. > and better handled by same mechanism > that already exists in kernel. > > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org> > > > --- a/drivers/uio/uio_pci_generic.c 2015-04-15 08:50:15.543900681 -0700 > +++ b/drivers/uio/uio_pci_generic.c 2015-04-15 09:00:01.658609786 -0700 > @@ -53,6 +53,18 @@ static irqreturn_t irqhandler(int irq, s > return IRQ_HANDLED; > } > > +static int irqcontrol(struct uio_info *info, s32 irq_on) > +{ > + struct uio_pci_generic_dev *gdev = to_uio_pci_generic_dev(info); > + struct pci_dev *pdev = gdev->pdev; > + > + pci_cfg_access_lock(pdev); > + pci_intx(pdev, irq_on); > + pci_cfg_access_unlock(pdev); > + > + return 0; > +} > + > static int probe(struct pci_dev *pdev, > const struct pci_device_id *id) > { > @@ -89,6 +101,7 @@ static int probe(struct pci_dev *pdev, > gdev->info.irq = pdev->irq; > gdev->info.irq_flags = IRQF_SHARED; > gdev->info.handler = irqhandler; > + gdev->info.irqcontrol = irqcontrol; > gdev->pdev = pdev; > > err = uio_register_device(&pdev->dev, &gdev->info); -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 16 Apr 2015 09:43:24 +0200 "Michael S. Tsirkin" <mst@redhat.com> wrote: > On Wed, Apr 15, 2015 at 09:59:34AM -0700, Stephen Hemminger wrote: > > The driver already supported INTX interrupts but had no in kernel > > function to enable and disable them. > > > > It is possible for userspace to do this by accessing PCI config > > directly, but this racy > > How is it racy? We have userspace using this interface, > if there's a race I want to fix it. There is nothing to prevent two threads in user space doing read/modify write at the same time. The bigger issue is that DPDK needs to support multiple UIO interface types. And with current model there is no abstraction. The way to enable/disable IRQ is different depending on the UIO drivers. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Apr 16, 2015 at 02:21:10PM -0700, Stephen Hemminger wrote: > On Thu, 16 Apr 2015 09:43:24 +0200 > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > On Wed, Apr 15, 2015 at 09:59:34AM -0700, Stephen Hemminger wrote: > > > The driver already supported INTX interrupts but had no in kernel > > > function to enable and disable them. > > > > > > It is possible for userspace to do this by accessing PCI config > > > directly, but this racy > > > > How is it racy? We have userspace using this interface, > > if there's a race I want to fix it. > > There is nothing to prevent two threads in user space doing > read/modify write at the same time. Well that's a userspace bug then - so let's drop that from commit log lest people think this fixes some kernel bugs. read/modify/write to the same register is at least an easy to grasp problem, creating an extra interface for the same function opens up the possibility that some userspace will do read/modify/write from one thread with irqcontrol from another thread, creating more races. > The bigger issue is that DPDK needs to support multiple UIO > interface types. And with current model there is no abstraction. > The way to enable/disable IRQ is different depending on the UIO > drivers. OK compatibility with other devices might be useful, but what are the other UIO drivers DPDK supports? I only found support for igb_uio so far, and that doesn't seem to be upstream.
On Mon, 20 Apr 2015 15:59:06 +0200 "Michael S. Tsirkin" <mst@redhat.com> wrote: > On Thu, Apr 16, 2015 at 02:21:10PM -0700, Stephen Hemminger wrote: > > On Thu, 16 Apr 2015 09:43:24 +0200 > > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > > > On Wed, Apr 15, 2015 at 09:59:34AM -0700, Stephen Hemminger wrote: > > > > The driver already supported INTX interrupts but had no in kernel > > > > function to enable and disable them. > > > > > > > > It is possible for userspace to do this by accessing PCI config > > > > directly, but this racy > > > > > > How is it racy? We have userspace using this interface, > > > if there's a race I want to fix it. > > > > There is nothing to prevent two threads in user space doing > > read/modify write at the same time. > > Well that's a userspace bug then - so let's drop that > from commit log lest people think this fixes some > kernel bugs. read/modify/write to the same register > is at least an easy to grasp problem, creating > an extra interface for the same function opens up > the possibility that some userspace will do > read/modify/write from one thread with irqcontrol > from another thread, creating more races. > > > The bigger issue is that DPDK needs to support multiple UIO > > interface types. And with current model there is no abstraction. > > The way to enable/disable IRQ is different depending on the UIO > > drivers. > > OK compatibility with other devices might be useful, but what are the > other UIO drivers DPDK supports? I only found support for igb_uio so > far, and that doesn't seem to be upstream. > Currently, supports: igb_uio, uio_pci_generic (as well as vfio) There are additional drivers which been submitted but not accepted for Xen and HyperV both of which require special uio drivers. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Apr 20, 2015 at 08:33:18AM -0700, Stephen Hemminger wrote: > On Mon, 20 Apr 2015 15:59:06 +0200 > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > On Thu, Apr 16, 2015 at 02:21:10PM -0700, Stephen Hemminger wrote: > > > On Thu, 16 Apr 2015 09:43:24 +0200 > > > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > > > > > On Wed, Apr 15, 2015 at 09:59:34AM -0700, Stephen Hemminger wrote: > > > > > The driver already supported INTX interrupts but had no in kernel > > > > > function to enable and disable them. > > > > > > > > > > It is possible for userspace to do this by accessing PCI config > > > > > directly, but this racy > > > > > > > > How is it racy? We have userspace using this interface, > > > > if there's a race I want to fix it. > > > > > > There is nothing to prevent two threads in user space doing > > > read/modify write at the same time. > > > > Well that's a userspace bug then - so let's drop that > > from commit log lest people think this fixes some > > kernel bugs. read/modify/write to the same register > > is at least an easy to grasp problem, creating > > an extra interface for the same function opens up > > the possibility that some userspace will do > > read/modify/write from one thread with irqcontrol > > from another thread, creating more races. > > > > > The bigger issue is that DPDK needs to support multiple UIO > > > interface types. And with current model there is no abstraction. > > > The way to enable/disable IRQ is different depending on the UIO > > > drivers. > > > > OK compatibility with other devices might be useful, but what are the > > other UIO drivers DPDK supports? I only found support for igb_uio so > > far, and that doesn't seem to be upstream. > > > > Currently, supports: > igb_uio, uio_pci_generic (as well as vfio) > > There are additional drivers which been submitted but not accepted for Xen and HyperV > both of which require special uio drivers. Well vfio doesn't have irq_control, does it? So I'd say it's best to wait and see before we commit to a new ABI then. You probably need to support existing kernels anyway, if igb_uio makes it upstream, then adding an interface that's consistent with it will make sense.
--- a/drivers/uio/uio_pci_generic.c 2015-04-15 08:50:15.543900681 -0700 +++ b/drivers/uio/uio_pci_generic.c 2015-04-15 09:00:01.658609786 -0700 @@ -53,6 +53,18 @@ static irqreturn_t irqhandler(int irq, s return IRQ_HANDLED; } +static int irqcontrol(struct uio_info *info, s32 irq_on) +{ + struct uio_pci_generic_dev *gdev = to_uio_pci_generic_dev(info); + struct pci_dev *pdev = gdev->pdev; + + pci_cfg_access_lock(pdev); + pci_intx(pdev, irq_on); + pci_cfg_access_unlock(pdev); + + return 0; +} + static int probe(struct pci_dev *pdev, const struct pci_device_id *id) { @@ -89,6 +101,7 @@ static int probe(struct pci_dev *pdev, gdev->info.irq = pdev->irq; gdev->info.irq_flags = IRQF_SHARED; gdev->info.handler = irqhandler; + gdev->info.irqcontrol = irqcontrol; gdev->pdev = pdev; err = uio_register_device(&pdev->dev, &gdev->info);
The driver already supported INTX interrupts but had no in kernel function to enable and disable them. It is possible for userspace to do this by accessing PCI config directly, but this racy and better handled by same mechanism that already exists in kernel. Signed-off-by: Stephen Hemminger <stephen@networkplumber.org> -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html