* Re: [RFC PATCH 0/2] virtio nvme
From: Nicholas A. Bellinger @ 2015-09-17 21:43 UTC (permalink / raw)
To: Ming Lin
Cc: Minturn, Dave B, linux-nvme, Linux Virtualization, target-devel,
Christoph Hellwig
In-Reply-To: <CAF1ivSaOZwjg3OEdj+miXsWwbpf-Y-tmN7wmjZooxTPDQ5r9tg@mail.gmail.com>
On Thu, 2015-09-17 at 11:18 -0700, Ming Lin wrote:
> On Wed, Sep 16, 2015 at 11:10 PM, Nicholas A. Bellinger
> <nab@linux-iscsi.org> wrote:
> > Hi Ming & Co,
> >> >
> >> > At first glance it seems like the virtio_nvme guest driver is just
> >> > another block driver like virtio_blk, so I'm not clear why a
> >> > virtio-nvme device makes sense.
> >>
> >> I think the future "LIO NVMe target" only speaks NVMe protocol.
> >>
> >> Nick(CCed), could you correct me if I'm wrong?
> >>
> >> For SCSI stack, we have:
> >> virtio-scsi(guest)
> >> tcm_vhost(or vhost_scsi, host)
> >> LIO-scsi-target
> >>
> >> For NVMe stack, we'll have similar components:
> >> virtio-nvme(guest)
> >> vhost_nvme(host)
> >> LIO-NVMe-target
> >>
> >
> > I think it's more interesting to consider a 'vhost style' driver that
> > can be used with unmodified nvme host OS drivers.
> >
> > Dr. Hannes (CC'ed) had done something like this for megasas a few years
> > back using specialized QEMU emulation + eventfd based LIO fabric driver,
> > and got it working with Linux + MSFT guests.
>
> Are the patches already in qemu upstream and LIO upstream?
>
> I found you played it in 2010. Is it?
> [QEMU-KVM]: Megasas + TCM_Loop + SG_IO into Windows XP guests
> https://groups.google.com/forum/#!topic/linux-iscsi-target-dev/3hdaI6H3X0A
>
AFAIK it was never submitted for kernel/QEMU upstream inclusion.
Dr Hannes, would you be so kind to put this code in a public repo to
grok as an example for nvme host-aware emulation..?
--nab
^ permalink raw reply
* Re: [RFC PATCH 0/2] virtio nvme
From: Ming Lin @ 2015-09-17 18:18 UTC (permalink / raw)
To: Nicholas A. Bellinger
Cc: Minturn, Dave B, linux-nvme, Linux Virtualization, target-devel,
Christoph Hellwig
In-Reply-To: <1442470241.23132.77.camel@haakon3.risingtidesystems.com>
On Wed, Sep 16, 2015 at 11:10 PM, Nicholas A. Bellinger
<nab@linux-iscsi.org> wrote:
> Hi Ming & Co,
Hi Nic,
>
> On Thu, 2015-09-10 at 10:28 -0700, Ming Lin wrote:
>> On Thu, 2015-09-10 at 15:38 +0100, Stefan Hajnoczi wrote:
>> > On Thu, Sep 10, 2015 at 6:48 AM, Ming Lin <mlin@kernel.org> wrote:
>> > > These 2 patches added virtio-nvme to kernel and qemu,
>> > > basically modified from virtio-blk and nvme code.
>> > >
>> > > As title said, request for your comments.
>
> <SNIP>
>
>> >
>> > At first glance it seems like the virtio_nvme guest driver is just
>> > another block driver like virtio_blk, so I'm not clear why a
>> > virtio-nvme device makes sense.
>>
>> I think the future "LIO NVMe target" only speaks NVMe protocol.
>>
>> Nick(CCed), could you correct me if I'm wrong?
>>
>> For SCSI stack, we have:
>> virtio-scsi(guest)
>> tcm_vhost(or vhost_scsi, host)
>> LIO-scsi-target
>>
>> For NVMe stack, we'll have similar components:
>> virtio-nvme(guest)
>> vhost_nvme(host)
>> LIO-NVMe-target
>>
>
> I think it's more interesting to consider a 'vhost style' driver that
> can be used with unmodified nvme host OS drivers.
>
> Dr. Hannes (CC'ed) had done something like this for megasas a few years
> back using specialized QEMU emulation + eventfd based LIO fabric driver,
> and got it working with Linux + MSFT guests.
Are the patches already in qemu upstream and LIO upstream?
I found you played it in 2010. Is it?
[QEMU-KVM]: Megasas + TCM_Loop + SG_IO into Windows XP guests
https://groups.google.com/forum/#!topic/linux-iscsi-target-dev/3hdaI6H3X0A
>
> Doing something similar for nvme would (potentially) be on par with
> current virtio-scsi+vhost-scsi small-block performance for scsi-mq
> guests, without the extra burden of a new command set specific virtio
> driver.
^ permalink raw reply
* Re: DEFINE_IDA causing memory leaks? (was Re: [PATCH 1/2] virtio: fix memory leak of virtio ida cache layers)
From: Tejun Heo @ 2015-09-17 18:00 UTC (permalink / raw)
To: James Bottomley
Cc: open-iscsi, linux-scsi, Michael S. Tsirkin, linux-kernel,
virtualization, Suman Anna
In-Reply-To: <1442512709.4073.24.camel@HansenPartnership.com>
Hello, James.
On Thu, Sep 17, 2015 at 10:58:29AM -0700, James Bottomley wrote:
> The argument is that we shouldn't have to explicitly destroy a
> statically initialized object, so
>
> DEFINE_IDA(someida);
>
> Should just work without having to explicitly do
>
> ida_destory(someida);
>
> somewhere in the exit code. It's about usage patterns. Michael's
> argument is that if we can't follow the no destructor pattern for
> DEFINE_IDA() then we shouldn't have it at all, because it's confusing
> kernel design patterns. The pattern we would have would be
>
> struct ida someida:
>
> ida_init(&someida);
>
> ...
>
> ida_destroy(&someida);
>
> so the object explicitly has a constructor matched to a destructor.
Yeah, I get that. I'm just not convinced that this matters enough
especially if we can get debugobj/ksan/whatever trip on it.
Thanks.
--
tejun
^ permalink raw reply
* Re: DEFINE_IDA causing memory leaks? (was Re: [PATCH 1/2] virtio: fix memory leak of virtio ida cache layers)
From: Tejun Heo @ 2015-09-17 17:15 UTC (permalink / raw)
To: James Bottomley
Cc: open-iscsi, linux-scsi, Michael S. Tsirkin, linux-kernel,
virtualization, Suman Anna
In-Reply-To: <1442508517.4073.13.camel@HansenPartnership.com>
Hello,
On Thu, Sep 17, 2015 at 09:48:37AM -0700, James Bottomley wrote:
> Well, there's an easy fix for that. We could have ida_remove() actually
> free the bitmap and not cache it if it's the last layer. That way ida
> would naturally empty and we wouldn't need a destructor. Tejun, would
> that work?
Yeah, that definitely is one way to go about it. It kinda muddles the
purpose of ida_destroy() tho. I suppose we can rename it to
idr_remove_all() and then do the same to idr. I'm not particularly
objecting to all that but what's wrong with just calling idr_destroy()
on exit paths? If missing the call in modules is an issue, maybe we
can just annotate idr/ida with debugobj?
Thanks.
--
tejun
^ permalink raw reply
* Re: DEFINE_IDA causing memory leaks? (was Re: [PATCH 1/2] virtio: fix memory leak of virtio ida cache layers)
From: Michael S. Tsirkin @ 2015-09-17 16:06 UTC (permalink / raw)
To: James Bottomley
Cc: Suman Anna, linux-scsi, linux-kernel, virtualization, Tejun Heo,
open-iscsi
In-Reply-To: <1442499344.4073.0.camel@HansenPartnership.com>
On Thu, Sep 17, 2015 at 07:15:44AM -0700, James Bottomley wrote:
> On Thu, 2015-09-17 at 08:33 +0300, Michael S. Tsirkin wrote:
> > On Wed, Sep 16, 2015 at 07:29:17PM -0500, Suman Anna wrote:
> > > The virtio core uses a static ida named virtio_index_ida for
> > > assigning index numbers to virtio devices during registration.
> > > The ida core may allocate some internal idr cache layers and
> > > an ida bitmap upon any ida allocation, and all these layers are
> > > truely freed only upon the ida destruction. The virtio_index_ida
> > > is not destroyed at present, leading to a memory leak when using
> > > the virtio core as a module and atleast one virtio device is
> > > registered and unregistered.
> > >
> > > Fix this by invoking ida_destroy() in the virtio core module
> > > exit.
> > >
> > > Cc: "Michael S. Tsirkin" <mst@redhat.com>
> > > Signed-off-by: Suman Anna <s-anna@ti.com>
> >
> > Interesting.
> > Will the same apply to e.g. sd_index_ida in drivers/scsi/sd.c
> > or iscsi_sess_ida in drivers/scsi/scsi_transport_iscsi.c?
> >
> > If no, why not?
> >
> > One doesn't generally expect to have to free global variables.
> > Maybe we should forbid DEFINE_IDA in modules?
> >
> > James, could you comment on this please?
>
> ida is Tejun's baby (cc'd). However, it does look like without
> ida_destroy() you will leave a cached ida->bitmap dangling because we're
> trying to be a bit clever in ida_remove() so we cache the bitmap to
> relieve ida_pre_get() of the burden if we would otherwise free it.
>
> I don't understand why you'd want to forbid DEFINE_IDA ... all it does
> is pre-initialise a usually static ida structure. The initialised
> structure will have a NULL bitmap cache that's allocated in the first
> ida_pre_get() ... that all seems to work as expected and no different
> from a dynamically allocated struct ida. Or are you thinking because
> ida_destory() doesn't set bitmap to NULL, it damages the reuse? In
> which case I'm not sure there's much benefit to making it reusable, but
> I suppose we could by adding a memset into ida_destroy().
>
> James
It's just unusual to have a descructor without a constructor.
I bet more drivers misuse this AI because of this.
> > > ---
> > > drivers/virtio/virtio.c | 1 +
> > > 1 file changed, 1 insertion(+)
> > >
> > > diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> > > index b1877d73fa56..7062bb0975a5 100644
> > > --- a/drivers/virtio/virtio.c
> > > +++ b/drivers/virtio/virtio.c
> > > @@ -412,6 +412,7 @@ static int virtio_init(void)
> > > static void __exit virtio_exit(void)
> > > {
> > > bus_unregister(&virtio_bus);
> > > + ida_destroy(&virtio_index_ida);
> > > }
> > > core_initcall(virtio_init);
> > > module_exit(virtio_exit);
> > > --
> > > 2.5.0
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> >
>
>
>
^ permalink raw reply
* Re: [PATCH v7] pci: quirk to skip msi disable on shutdown
From: Michael S. Tsirkin @ 2015-09-17 16:03 UTC (permalink / raw)
To: Eric W. Biederman
Cc: Fam Zheng, linux-pci, linux-kernel, Ulrich Obergfell, Yinghai Lu,
Bjorn Helgaas, virtualization
In-Reply-To: <874mitxczx.fsf@x220.int.ebiederm.org>
On Thu, Sep 17, 2015 at 10:49:06AM -0500, Eric W. Biederman wrote:
> Bjorn Helgaas <bhelgaas@google.com> writes:
>
> > On Sun, Sep 06, 2015 at 06:32:35PM +0300, Michael S. Tsirkin wrote:
> >> On some hypervisors, virtio devices tend to generate spurious interrupts
> >> when switching between MSI and non-MSI mode. Normally, either MSI or
> >> non-MSI is used and all is well, but during shutdown, linux disables MSI
> >> which then causes an "irq %d: nobody cared" message, with irq being
> >> subsequently disabled.
> >>
> >> Since bus mastering is already disabled at this point, disabling MSI
> >> isn't actually useful for spec compliant devices: MSI interrupts are
> >> in-bus memory writes so disabling Bus Master (which is already done)
> >> disables these as well: after some research, it appears to be there for
> >> the benefit of devices that ignore the bus master bit.
> >>
> >> As it's not clear how common this kind of bug is, this patch simply adds
> >> a quirk, to be set by devices that wish to skip disabling msi on
> >> shutdown, relying on bus master instead.
> >>
> >> We set this quirk in virtio core.
> >>
> >> Reported-by: Fam Zheng <famz@redhat.com>
> >> Cc: Bjorn Helgaas <bhelgaas@google.com>
> >> Cc: Yinghai Lu <yhlu.kernel.send@gmail.com>
> >> Cc: Ulrich Obergfell <uobergfe@redhat.com>
> >> Cc: Rusty Russell <rusty@rustcorp.com.au>
> >> Cc: "Eric W. Biederman" <ebiederm@xmission.com>
> >> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> >
> > Eric, what do you think of this? You had many comments on previous
> > versions.
>
> I still think it is a hack to avoid actually fixing a driver.
The bug's in the hypervisor though.
> I think the hack is better as a quirk.
You mean put it in drivers/pci/quirks.c?
> I think the quirk would tend to
> be better if it was limited to whatever set of hypervisors where this is
> actually a problem.
I guess we might add a new flag "msi disable at shutdown is safe" in future
hypervisors. Until such hypervisors ship I don't see a way to detect
this.
> And of course given that on any sane configuration we have the irq
> watchdog I don't see what this is fixing in practice.
>
> Other than suggesting this hack become find a way to limit itself to the
> driver that is actually having problems I don't see a way to improve it.
It's already limited: virtio is the only one that sets
PCI_DEV_FLAGS_NO_MSI_SHUTDOWN. Did you notice this?
> > Minor comment on a comment below.
> >
> >> ---
> >>
> >>
> >> changes from v6:
> >> limit changes to virtio only
> >> changes from v5:
> >> rebased on top of pci/msi
> >> fixed commit log, including comments by Bjorn
> >> and adding explanation to address comments/questions by Eric
> >> dropped stable Cc, this patch does not seem to qualify for stable
> >> changes from v4:
> >> Yijing Wang <wangyijing@huawei.com> noted that
> >> early fixups rely on pci_msi_off.
> >> Split out the functionality and move off the
> >> required part to run early during pci_device_setup.
> >> Changes from v3:
> >> fix a copy-and-paste error in
> >> pci: drop some duplicate code
> >> other patches are unchanged
> >> drop Cc stable for now
> >> Changes from v2:
> >> move code from probe to device enumeration
> >> add patches to unexport pci_msi_off
> >>
> >>
> >> include/linux/pci.h | 2 ++
> >> drivers/pci/pci-driver.c | 6 ++++--
> >> drivers/virtio/virtio_pci_common.c | 4 ++++
> >> 3 files changed, 10 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/include/linux/pci.h b/include/linux/pci.h
> >> index 860c751..80f3494 100644
> >> --- a/include/linux/pci.h
> >> +++ b/include/linux/pci.h
> >> @@ -180,6 +180,8 @@ enum pci_dev_flags {
> >> PCI_DEV_FLAGS_NO_BUS_RESET = (__force pci_dev_flags_t) (1 << 6),
> >> /* Do not use PM reset even if device advertises NoSoftRst- */
> >> PCI_DEV_FLAGS_NO_PM_RESET = (__force pci_dev_flags_t) (1 << 7),
> >> + /* Do not disable MSI on shutdown, disable bus master instead */
> >
> > This comment doesn't really match what the patch does. The patch merely
> > does "Do not disable MSI on shutdown." It doesn't "disable bus master
> > instead."
> >
> > Bus master may be disabled elsewhere, but that is independent of the
> > PCI_DEV_FLAGS_NO_MSI_SHUTDOWN flag.
> >
> >> + PCI_DEV_FLAGS_NO_MSI_SHUTDOWN = (__force pci_dev_flags_t) (1 << 8),
> >> };
> >>
> >> enum pci_irq_reroute_variant {
> >> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> >> index 3cb2210..59d9e40 100644
> >> --- a/drivers/pci/pci-driver.c
> >> +++ b/drivers/pci/pci-driver.c
> >> @@ -450,8 +450,10 @@ static void pci_device_shutdown(struct device *dev)
> >>
> >> if (drv && drv->shutdown)
> >> drv->shutdown(pci_dev);
> >> - pci_msi_shutdown(pci_dev);
> >> - pci_msix_shutdown(pci_dev);
> >> + if (!(pci_dev->dev_flags & PCI_DEV_FLAGS_NO_MSI_SHUTDOWN)) {
> >> + pci_msi_shutdown(pci_dev);
> >> + pci_msix_shutdown(pci_dev);
> >> + }
> >>
> >> #ifdef CONFIG_KEXEC
> >> /*
> >> diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
> >> index 78f804a..26f46c3 100644
> >> --- a/drivers/virtio/virtio_pci_common.c
> >> +++ b/drivers/virtio/virtio_pci_common.c
> >> @@ -528,6 +528,8 @@ static int virtio_pci_probe(struct pci_dev *pci_dev,
> >> if (rc)
> >> goto err_register;
> >>
> >> + pci_dev->dev_flags |= PCI_DEV_FLAGS_NO_MSI_SHUTDOWN;
> >> +
> >> return 0;
> >>
> >> err_register:
> >> @@ -546,6 +548,8 @@ static void virtio_pci_remove(struct pci_dev *pci_dev)
> >> {
> >> struct virtio_pci_device *vp_dev = pci_get_drvdata(pci_dev);
> >>
> >> + pci_dev->dev_flags &= ~PCI_DEV_FLAGS_NO_MSI_SHUTDOWN;
> >> +
> >> unregister_virtio_device(&vp_dev->vdev);
> >>
> >> if (vp_dev->ioaddr)
> >> --
> >> MST
^ permalink raw reply
* Re: [PATCH v7] pci: quirk to skip msi disable on shutdown
From: Michael S. Tsirkin @ 2015-09-17 15:56 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Fam Zheng, linux-pci, linux-kernel, Ulrich Obergfell, Yinghai Lu,
Eric W. Biederman, virtualization
In-Reply-To: <20150917154424.GI25767@google.com>
On Thu, Sep 17, 2015 at 10:44:24AM -0500, Bjorn Helgaas wrote:
> On Sun, Sep 06, 2015 at 06:32:35PM +0300, Michael S. Tsirkin wrote:
> > On some hypervisors, virtio devices tend to generate spurious interrupts
> > when switching between MSI and non-MSI mode. Normally, either MSI or
> > non-MSI is used and all is well, but during shutdown, linux disables MSI
> > which then causes an "irq %d: nobody cared" message, with irq being
> > subsequently disabled.
> >
> > Since bus mastering is already disabled at this point, disabling MSI
> > isn't actually useful for spec compliant devices: MSI interrupts are
> > in-bus memory writes so disabling Bus Master (which is already done)
> > disables these as well: after some research, it appears to be there for
> > the benefit of devices that ignore the bus master bit.
> >
> > As it's not clear how common this kind of bug is, this patch simply adds
> > a quirk, to be set by devices that wish to skip disabling msi on
> > shutdown, relying on bus master instead.
> >
> > We set this quirk in virtio core.
> >
> > Reported-by: Fam Zheng <famz@redhat.com>
> > Cc: Bjorn Helgaas <bhelgaas@google.com>
> > Cc: Yinghai Lu <yhlu.kernel.send@gmail.com>
> > Cc: Ulrich Obergfell <uobergfe@redhat.com>
> > Cc: Rusty Russell <rusty@rustcorp.com.au>
> > Cc: "Eric W. Biederman" <ebiederm@xmission.com>
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>
> Eric, what do you think of this? You had many comments on previous
> versions.
>
> Minor comment on a comment below.
>
> > ---
> >
> >
> > changes from v6:
> > limit changes to virtio only
> > changes from v5:
> > rebased on top of pci/msi
> > fixed commit log, including comments by Bjorn
> > and adding explanation to address comments/questions by Eric
> > dropped stable Cc, this patch does not seem to qualify for stable
> > changes from v4:
> > Yijing Wang <wangyijing@huawei.com> noted that
> > early fixups rely on pci_msi_off.
> > Split out the functionality and move off the
> > required part to run early during pci_device_setup.
> > Changes from v3:
> > fix a copy-and-paste error in
> > pci: drop some duplicate code
> > other patches are unchanged
> > drop Cc stable for now
> > Changes from v2:
> > move code from probe to device enumeration
> > add patches to unexport pci_msi_off
> >
> >
> > include/linux/pci.h | 2 ++
> > drivers/pci/pci-driver.c | 6 ++++--
> > drivers/virtio/virtio_pci_common.c | 4 ++++
> > 3 files changed, 10 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/linux/pci.h b/include/linux/pci.h
> > index 860c751..80f3494 100644
> > --- a/include/linux/pci.h
> > +++ b/include/linux/pci.h
> > @@ -180,6 +180,8 @@ enum pci_dev_flags {
> > PCI_DEV_FLAGS_NO_BUS_RESET = (__force pci_dev_flags_t) (1 << 6),
> > /* Do not use PM reset even if device advertises NoSoftRst- */
> > PCI_DEV_FLAGS_NO_PM_RESET = (__force pci_dev_flags_t) (1 << 7),
> > + /* Do not disable MSI on shutdown, disable bus master instead */
>
> This comment doesn't really match what the patch does. The patch merely
> does "Do not disable MSI on shutdown." It doesn't "disable bus master
> instead."
>
> Bus master may be disabled elsewhere, but that is independent of the
> PCI_DEV_FLAGS_NO_MSI_SHUTDOWN flag.
Yes but I wanted to point out that bus master needs to work.
One of the reasons we don't do this patch for all devices is because
some builtin devices might have a broken bus master.
How about
"Some other means (e.g. disabling bus master) suppress interrupts."
> > + PCI_DEV_FLAGS_NO_MSI_SHUTDOWN = (__force pci_dev_flags_t) (1 << 8),
> > };
> >
> > enum pci_irq_reroute_variant {
> > diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> > index 3cb2210..59d9e40 100644
> > --- a/drivers/pci/pci-driver.c
> > +++ b/drivers/pci/pci-driver.c
> > @@ -450,8 +450,10 @@ static void pci_device_shutdown(struct device *dev)
> >
> > if (drv && drv->shutdown)
> > drv->shutdown(pci_dev);
> > - pci_msi_shutdown(pci_dev);
> > - pci_msix_shutdown(pci_dev);
> > + if (!(pci_dev->dev_flags & PCI_DEV_FLAGS_NO_MSI_SHUTDOWN)) {
> > + pci_msi_shutdown(pci_dev);
> > + pci_msix_shutdown(pci_dev);
> > + }
> >
> > #ifdef CONFIG_KEXEC
> > /*
> > diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
> > index 78f804a..26f46c3 100644
> > --- a/drivers/virtio/virtio_pci_common.c
> > +++ b/drivers/virtio/virtio_pci_common.c
> > @@ -528,6 +528,8 @@ static int virtio_pci_probe(struct pci_dev *pci_dev,
> > if (rc)
> > goto err_register;
> >
> > + pci_dev->dev_flags |= PCI_DEV_FLAGS_NO_MSI_SHUTDOWN;
> > +
> > return 0;
> >
> > err_register:
> > @@ -546,6 +548,8 @@ static void virtio_pci_remove(struct pci_dev *pci_dev)
> > {
> > struct virtio_pci_device *vp_dev = pci_get_drvdata(pci_dev);
> >
> > + pci_dev->dev_flags &= ~PCI_DEV_FLAGS_NO_MSI_SHUTDOWN;
> > +
> > unregister_virtio_device(&vp_dev->vdev);
> >
> > if (vp_dev->ioaddr)
> > --
> > MST
^ permalink raw reply
* Re: [PATCH v7] pci: quirk to skip msi disable on shutdown
From: Eric W. Biederman @ 2015-09-17 15:49 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Fam Zheng, Michael S. Tsirkin, linux-pci, Ulrich Obergfell,
linux-kernel, Yinghai Lu, virtualization
In-Reply-To: <20150917154424.GI25767@google.com>
Bjorn Helgaas <bhelgaas@google.com> writes:
> On Sun, Sep 06, 2015 at 06:32:35PM +0300, Michael S. Tsirkin wrote:
>> On some hypervisors, virtio devices tend to generate spurious interrupts
>> when switching between MSI and non-MSI mode. Normally, either MSI or
>> non-MSI is used and all is well, but during shutdown, linux disables MSI
>> which then causes an "irq %d: nobody cared" message, with irq being
>> subsequently disabled.
>>
>> Since bus mastering is already disabled at this point, disabling MSI
>> isn't actually useful for spec compliant devices: MSI interrupts are
>> in-bus memory writes so disabling Bus Master (which is already done)
>> disables these as well: after some research, it appears to be there for
>> the benefit of devices that ignore the bus master bit.
>>
>> As it's not clear how common this kind of bug is, this patch simply adds
>> a quirk, to be set by devices that wish to skip disabling msi on
>> shutdown, relying on bus master instead.
>>
>> We set this quirk in virtio core.
>>
>> Reported-by: Fam Zheng <famz@redhat.com>
>> Cc: Bjorn Helgaas <bhelgaas@google.com>
>> Cc: Yinghai Lu <yhlu.kernel.send@gmail.com>
>> Cc: Ulrich Obergfell <uobergfe@redhat.com>
>> Cc: Rusty Russell <rusty@rustcorp.com.au>
>> Cc: "Eric W. Biederman" <ebiederm@xmission.com>
>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>
> Eric, what do you think of this? You had many comments on previous
> versions.
I still think it is a hack to avoid actually fixing a driver.
I think the hack is better as a quirk. I think the quirk would tend to
be better if it was limited to whatever set of hypervisors where this is
actually a problem.
And of course given that on any sane configuration we have the irq
watchdog I don't see what this is fixing in practice.
Other than suggesting this hack become find a way to limit itself to the
driver that is actually having problems I don't see a way to improve it.
> Minor comment on a comment below.
>
>> ---
>>
>>
>> changes from v6:
>> limit changes to virtio only
>> changes from v5:
>> rebased on top of pci/msi
>> fixed commit log, including comments by Bjorn
>> and adding explanation to address comments/questions by Eric
>> dropped stable Cc, this patch does not seem to qualify for stable
>> changes from v4:
>> Yijing Wang <wangyijing@huawei.com> noted that
>> early fixups rely on pci_msi_off.
>> Split out the functionality and move off the
>> required part to run early during pci_device_setup.
>> Changes from v3:
>> fix a copy-and-paste error in
>> pci: drop some duplicate code
>> other patches are unchanged
>> drop Cc stable for now
>> Changes from v2:
>> move code from probe to device enumeration
>> add patches to unexport pci_msi_off
>>
>>
>> include/linux/pci.h | 2 ++
>> drivers/pci/pci-driver.c | 6 ++++--
>> drivers/virtio/virtio_pci_common.c | 4 ++++
>> 3 files changed, 10 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/linux/pci.h b/include/linux/pci.h
>> index 860c751..80f3494 100644
>> --- a/include/linux/pci.h
>> +++ b/include/linux/pci.h
>> @@ -180,6 +180,8 @@ enum pci_dev_flags {
>> PCI_DEV_FLAGS_NO_BUS_RESET = (__force pci_dev_flags_t) (1 << 6),
>> /* Do not use PM reset even if device advertises NoSoftRst- */
>> PCI_DEV_FLAGS_NO_PM_RESET = (__force pci_dev_flags_t) (1 << 7),
>> + /* Do not disable MSI on shutdown, disable bus master instead */
>
> This comment doesn't really match what the patch does. The patch merely
> does "Do not disable MSI on shutdown." It doesn't "disable bus master
> instead."
>
> Bus master may be disabled elsewhere, but that is independent of the
> PCI_DEV_FLAGS_NO_MSI_SHUTDOWN flag.
>
>> + PCI_DEV_FLAGS_NO_MSI_SHUTDOWN = (__force pci_dev_flags_t) (1 << 8),
>> };
>>
>> enum pci_irq_reroute_variant {
>> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
>> index 3cb2210..59d9e40 100644
>> --- a/drivers/pci/pci-driver.c
>> +++ b/drivers/pci/pci-driver.c
>> @@ -450,8 +450,10 @@ static void pci_device_shutdown(struct device *dev)
>>
>> if (drv && drv->shutdown)
>> drv->shutdown(pci_dev);
>> - pci_msi_shutdown(pci_dev);
>> - pci_msix_shutdown(pci_dev);
>> + if (!(pci_dev->dev_flags & PCI_DEV_FLAGS_NO_MSI_SHUTDOWN)) {
>> + pci_msi_shutdown(pci_dev);
>> + pci_msix_shutdown(pci_dev);
>> + }
>>
>> #ifdef CONFIG_KEXEC
>> /*
>> diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
>> index 78f804a..26f46c3 100644
>> --- a/drivers/virtio/virtio_pci_common.c
>> +++ b/drivers/virtio/virtio_pci_common.c
>> @@ -528,6 +528,8 @@ static int virtio_pci_probe(struct pci_dev *pci_dev,
>> if (rc)
>> goto err_register;
>>
>> + pci_dev->dev_flags |= PCI_DEV_FLAGS_NO_MSI_SHUTDOWN;
>> +
>> return 0;
>>
>> err_register:
>> @@ -546,6 +548,8 @@ static void virtio_pci_remove(struct pci_dev *pci_dev)
>> {
>> struct virtio_pci_device *vp_dev = pci_get_drvdata(pci_dev);
>>
>> + pci_dev->dev_flags &= ~PCI_DEV_FLAGS_NO_MSI_SHUTDOWN;
>> +
>> unregister_virtio_device(&vp_dev->vdev);
>>
>> if (vp_dev->ioaddr)
>> --
>> MST
^ permalink raw reply
* Re: [PATCH v7] pci: quirk to skip msi disable on shutdown
From: Bjorn Helgaas via Virtualization @ 2015-09-17 15:44 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Fam Zheng, linux-pci, linux-kernel, Ulrich Obergfell, Yinghai Lu,
Eric W. Biederman, virtualization
In-Reply-To: <1441553385-3810-1-git-send-email-mst@redhat.com>
On Sun, Sep 06, 2015 at 06:32:35PM +0300, Michael S. Tsirkin wrote:
> On some hypervisors, virtio devices tend to generate spurious interrupts
> when switching between MSI and non-MSI mode. Normally, either MSI or
> non-MSI is used and all is well, but during shutdown, linux disables MSI
> which then causes an "irq %d: nobody cared" message, with irq being
> subsequently disabled.
>
> Since bus mastering is already disabled at this point, disabling MSI
> isn't actually useful for spec compliant devices: MSI interrupts are
> in-bus memory writes so disabling Bus Master (which is already done)
> disables these as well: after some research, it appears to be there for
> the benefit of devices that ignore the bus master bit.
>
> As it's not clear how common this kind of bug is, this patch simply adds
> a quirk, to be set by devices that wish to skip disabling msi on
> shutdown, relying on bus master instead.
>
> We set this quirk in virtio core.
>
> Reported-by: Fam Zheng <famz@redhat.com>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: Yinghai Lu <yhlu.kernel.send@gmail.com>
> Cc: Ulrich Obergfell <uobergfe@redhat.com>
> Cc: Rusty Russell <rusty@rustcorp.com.au>
> Cc: "Eric W. Biederman" <ebiederm@xmission.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Eric, what do you think of this? You had many comments on previous
versions.
Minor comment on a comment below.
> ---
>
>
> changes from v6:
> limit changes to virtio only
> changes from v5:
> rebased on top of pci/msi
> fixed commit log, including comments by Bjorn
> and adding explanation to address comments/questions by Eric
> dropped stable Cc, this patch does not seem to qualify for stable
> changes from v4:
> Yijing Wang <wangyijing@huawei.com> noted that
> early fixups rely on pci_msi_off.
> Split out the functionality and move off the
> required part to run early during pci_device_setup.
> Changes from v3:
> fix a copy-and-paste error in
> pci: drop some duplicate code
> other patches are unchanged
> drop Cc stable for now
> Changes from v2:
> move code from probe to device enumeration
> add patches to unexport pci_msi_off
>
>
> include/linux/pci.h | 2 ++
> drivers/pci/pci-driver.c | 6 ++++--
> drivers/virtio/virtio_pci_common.c | 4 ++++
> 3 files changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 860c751..80f3494 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -180,6 +180,8 @@ enum pci_dev_flags {
> PCI_DEV_FLAGS_NO_BUS_RESET = (__force pci_dev_flags_t) (1 << 6),
> /* Do not use PM reset even if device advertises NoSoftRst- */
> PCI_DEV_FLAGS_NO_PM_RESET = (__force pci_dev_flags_t) (1 << 7),
> + /* Do not disable MSI on shutdown, disable bus master instead */
This comment doesn't really match what the patch does. The patch merely
does "Do not disable MSI on shutdown." It doesn't "disable bus master
instead."
Bus master may be disabled elsewhere, but that is independent of the
PCI_DEV_FLAGS_NO_MSI_SHUTDOWN flag.
> + PCI_DEV_FLAGS_NO_MSI_SHUTDOWN = (__force pci_dev_flags_t) (1 << 8),
> };
>
> enum pci_irq_reroute_variant {
> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> index 3cb2210..59d9e40 100644
> --- a/drivers/pci/pci-driver.c
> +++ b/drivers/pci/pci-driver.c
> @@ -450,8 +450,10 @@ static void pci_device_shutdown(struct device *dev)
>
> if (drv && drv->shutdown)
> drv->shutdown(pci_dev);
> - pci_msi_shutdown(pci_dev);
> - pci_msix_shutdown(pci_dev);
> + if (!(pci_dev->dev_flags & PCI_DEV_FLAGS_NO_MSI_SHUTDOWN)) {
> + pci_msi_shutdown(pci_dev);
> + pci_msix_shutdown(pci_dev);
> + }
>
> #ifdef CONFIG_KEXEC
> /*
> diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
> index 78f804a..26f46c3 100644
> --- a/drivers/virtio/virtio_pci_common.c
> +++ b/drivers/virtio/virtio_pci_common.c
> @@ -528,6 +528,8 @@ static int virtio_pci_probe(struct pci_dev *pci_dev,
> if (rc)
> goto err_register;
>
> + pci_dev->dev_flags |= PCI_DEV_FLAGS_NO_MSI_SHUTDOWN;
> +
> return 0;
>
> err_register:
> @@ -546,6 +548,8 @@ static void virtio_pci_remove(struct pci_dev *pci_dev)
> {
> struct virtio_pci_device *vp_dev = pci_get_drvdata(pci_dev);
>
> + pci_dev->dev_flags &= ~PCI_DEV_FLAGS_NO_MSI_SHUTDOWN;
> +
> unregister_virtio_device(&vp_dev->vdev);
>
> if (vp_dev->ioaddr)
> --
> MST
^ permalink raw reply
* Re: [dpdk-dev] virtio optimization idea
From: Xie, Huawei @ 2015-09-17 15:41 UTC (permalink / raw)
To: Stephen Hemminger
Cc: dev@dpdk.org, virtualization@lists.linux-foundation.org,
ms >> Michael S. Tsirkin
In-Reply-To: <C37D651A908B024F974696C65296B57B2BDC0872@SHSMSX101.ccr.corp.intel.com>
On 9/8/2015 11:54 PM, Xie, Huawei wrote:
> On 9/8/2015 11:39 PM, Stephen Hemminger wrote:
>> On Fri, 4 Sep 2015 08:25:05 +0000
>> "Xie, Huawei" <huawei.xie@intel.com> wrote:
>>
>>> Hi:
>>>
>>> Recently I have done one virtio optimization proof of concept. The
>>> optimization includes two parts:
>>> 1) avail ring set with fixed descriptors
>>> 2) RX vectorization
>>> With the optimizations, we could have several times of performance boost
>>> for purely vhost-virtio throughput.
>>>
>>> Here i will only cover the first part, which is the prerequisite for the
>>> second part.
>>> Let us first take RX for example. Currently when we fill the avail ring
>>> with guest mbuf, we need
>>> a) allocate one descriptor(for non sg mbuf) from free descriptors
>>> b) set the idx of the desc into the entry of avail ring
>>> c) set the addr/len field of the descriptor to point to guest blank mbuf
>>> data area
>>>
>>> Those operation takes time, and especially step b results in modifed (M)
>>> state of the cache line for the avail ring in the virtio processing
>>> core. When vhost processes the avail ring, the cache line transfer from
>>> virtio processing core to vhost processing core takes pretty much CPU
>>> cycles.
>>> To solve this problem, this is the arrangement of RX ring for DPDK
>>> pmd(for non-mergable case).
>>>
>>> avail
>>> idx
>>> +
>>> |
>>> +----+----+---+-------------+------+
>>> | 0 | 1 | 2 | ... | 254 | 255 | avail ring
>>> +-+--+-+--+-+-+---------+---+--+---+
>>> | | | | | |
>>> | | | | | |
>>> v v v | v v
>>> +-+--+-+--+-+-+---------+---+--+---+
>>> | 0 | 1 | 2 | ... | 254 | 255 | desc ring
>>> +----+----+---+-------------+------+
>>> |
>>> |
>>> +----+----+---+-------------+------+
>>> | 0 | 1 | 2 | | 254 | 255 | used ring
>>> +----+----+---+-------------+------+
>>> |
>>> +
>>> Avail ring is initialized with fixed descriptor and is never changed,
>>> i.e, the index value of the nth avail ring entry is always n, which
>>> means virtio PMD is actually refilling desc ring only, without having to
>>> change avail ring.
>>> When vhost fetches avail ring, if not evicted, it is always in its first
>>> level cache.
>>>
>>> When RX receives packets from used ring, we use the used->idx as the
>>> desc idx. This requires that vhost processes and returns descs from
>>> avail ring to used ring in order, which is true for both current dpdk
>>> vhost and kernel vhost implementation. In my understanding, there is no
>>> necessity for vhost net to process descriptors OOO. One case could be
>>> zero copy, for example, if one descriptor doesn't meet zero copy
>>> requirment, we could directly return it to used ring, earlier than the
>>> descriptors in front of it.
>>> To enforce this, i want to use a reserved bit to indicate in order
>>> processing of descriptors.
>>>
>>> For tx ring, the arrangement is like below. Each transmitted mbuf needs
>>> a desc for virtio_net_hdr, so actually we have only 128 free slots.
>>>
>>>
>>>
++
||
||
+-----+-----+-----+--------------+------+------+------+
| 0 | 1 | ... | 127 || 128 | 129 | ... | 255 | avail ring
+--+--+--+--+-----+---+------+---+--+---+------+--+---+
| | | || | | |
v v v || v v v
+--+--+--+--+-----+---+------+---+--+---+------+--+---+
| 127 | 128 | ... | 255 || 127 | 128 | ... | 255 | desc ring for virtio_net_hdr
+--+--+--+--+-----+---+------+---+--+---+------+--+---+
| | | || | | |
v v v || v v v
+--+--+--+--+-----+---+------+---+--+---+------+--+---+
| 0 | 1 | ... | 127 || 0 | 1 | ... | 127 | desc ring for tx dat
>>>
>>>
>> Does this still work with Linux (or BSD) guest/host.
>> If you are assuming both virtio/vhost are DPDK this is never going
>> to be usable.
> It works with both dpdk vhost and kernel vhost implementations.
> But to enforce this, we had better add a new feature bit.
Hi Stephen, some update about compatibility:
This optimization in theory is compliant with current kernel vhost,
qemu, and dpdk vhost implementations.
Today i run dpdk virtio PMD with qemu and kernel vhost, and it works fine.
>> On a related note, have you looked at getting virtio to support the
>> new standard (not legacy) mode?
> Yes, we add it to our plan to support virtio 1.0.
>>
>
^ permalink raw reply
* Re: DEFINE_IDA causing memory leaks? (was Re: [PATCH 1/2] virtio: fix memory leak of virtio ida cache layers)
From: Tejun Heo @ 2015-09-17 15:10 UTC (permalink / raw)
To: James Bottomley
Cc: open-iscsi, linux-scsi, Michael S. Tsirkin, linux-kernel,
virtualization, Suman Anna
In-Reply-To: <1442499344.4073.0.camel@HansenPartnership.com>
Hello,
On Thu, Sep 17, 2015 at 07:15:44AM -0700, James Bottomley wrote:
> I don't understand why you'd want to forbid DEFINE_IDA ... all it does
I guess to require the use of explicit init / creation so that it's
clear the data structure needs to be destroyed?
> is pre-initialise a usually static ida structure. The initialised
> structure will have a NULL bitmap cache that's allocated in the first
> ida_pre_get() ... that all seems to work as expected and no different
> from a dynamically allocated struct ida. Or are you thinking because
> ida_destory() doesn't set bitmap to NULL, it damages the reuse? In
> which case I'm not sure there's much benefit to making it reusable, but
> I suppose we could by adding a memset into ida_destroy().
I don't know. Data structures which do lazy anything would likely
need explicit destruction and I'm not sure we'd wanna ban static
initialization for all such cases. Seems like an unnecessary
restriction.
Thanks.
--
tejun
^ permalink raw reply
* Re: DEFINE_IDA causing memory leaks? (was Re: [PATCH 1/2] virtio: fix memory leak of virtio ida cache layers)
From: Hannes Reinecke @ 2015-09-17 6:51 UTC (permalink / raw)
To: Michael S. Tsirkin, Suman Anna
Cc: open-iscsi, JBottomley, linux-kernel, linux-scsi, virtualization
In-Reply-To: <20150917082425-mutt-send-email-mst@redhat.com>
On 09/17/2015 07:33 AM, Michael S. Tsirkin wrote:
> On Wed, Sep 16, 2015 at 07:29:17PM -0500, Suman Anna wrote:
>> The virtio core uses a static ida named virtio_index_ida for
>> assigning index numbers to virtio devices during registration.
>> The ida core may allocate some internal idr cache layers and
>> an ida bitmap upon any ida allocation, and all these layers are
>> truely freed only upon the ida destruction. The virtio_index_ida
>> is not destroyed at present, leading to a memory leak when using
>> the virtio core as a module and atleast one virtio device is
>> registered and unregistered.
>>
>> Fix this by invoking ida_destroy() in the virtio core module
>> exit.
>>
>> Cc: "Michael S. Tsirkin" <mst@redhat.com>
>> Signed-off-by: Suman Anna <s-anna@ti.com>
>
> Interesting.
> Will the same apply to e.g. sd_index_ida in drivers/scsi/sd.c
> or iscsi_sess_ida in drivers/scsi/scsi_transport_iscsi.c?
>
> If no, why not?
>
> One doesn't generally expect to have to free global variables.
> Maybe we should forbid DEFINE_IDA in modules?
>
> James, could you comment on this please?
>
Well, looking at the code 'ida_destroy' only need to be called
if you want/need to do a general cleanup.
It shouldn't be required if you do correct reference counting
on your objects, and call idr_remove() on each of them.
Unless I'm misreading something.
Seems like a topic for KS; Johannes had a larger patchset recently to
clean up idr, which run into very much the same issues.
Cheers,
Hannes
--
Dr. Hannes Reinecke zSeries & Storage
hare@suse.de +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
^ permalink raw reply
* Re: [RFC PATCH 0/2] virtio nvme
From: Nicholas A. Bellinger @ 2015-09-17 6:10 UTC (permalink / raw)
To: Ming Lin
Cc: Minturn, Dave B, linux-nvme, Linux Virtualization, target-devel,
Christoph Hellwig
In-Reply-To: <1441906098.18716.21.camel@ssi>
Hi Ming & Co,
On Thu, 2015-09-10 at 10:28 -0700, Ming Lin wrote:
> On Thu, 2015-09-10 at 15:38 +0100, Stefan Hajnoczi wrote:
> > On Thu, Sep 10, 2015 at 6:48 AM, Ming Lin <mlin@kernel.org> wrote:
> > > These 2 patches added virtio-nvme to kernel and qemu,
> > > basically modified from virtio-blk and nvme code.
> > >
> > > As title said, request for your comments.
<SNIP>
> >
> > At first glance it seems like the virtio_nvme guest driver is just
> > another block driver like virtio_blk, so I'm not clear why a
> > virtio-nvme device makes sense.
>
> I think the future "LIO NVMe target" only speaks NVMe protocol.
>
> Nick(CCed), could you correct me if I'm wrong?
>
> For SCSI stack, we have:
> virtio-scsi(guest)
> tcm_vhost(or vhost_scsi, host)
> LIO-scsi-target
>
> For NVMe stack, we'll have similar components:
> virtio-nvme(guest)
> vhost_nvme(host)
> LIO-NVMe-target
>
I think it's more interesting to consider a 'vhost style' driver that
can be used with unmodified nvme host OS drivers.
Dr. Hannes (CC'ed) had done something like this for megasas a few years
back using specialized QEMU emulation + eventfd based LIO fabric driver,
and got it working with Linux + MSFT guests.
Doing something similar for nvme would (potentially) be on par with
current virtio-scsi+vhost-scsi small-block performance for scsi-mq
guests, without the extra burden of a new command set specific virtio
driver.
> >
> > > Now there are lots of duplicated code with linux/nvme-core.c and qemu/nvme.c.
> > > The ideal result is to have a multi level NVMe stack(similar as SCSI).
> > > So we can re-use the nvme code, for example
> > >
> > > .-------------------------.
> > > | NVMe device register |
> > > Upper level | NVMe protocol process |
> > > | |
> > > '-------------------------'
> > >
> > >
> > >
> > > .-----------. .-----------. .------------------.
> > > Lower level | PCIe | | VIRTIO | |NVMe over Fabrics |
> > > | | | | |initiator |
> > > '-----------' '-----------' '------------------'
> >
> > You mentioned LIO and SCSI. How will NVMe over Fabrics be integrated
> > into LIO? If it is mapped to SCSI then using virtio_scsi in the guest
> > and tcm_vhost should work.
>
> I think it's not mapped to SCSI.
>
> Nick, would you share more here?
>
(Adding Dave M. CC')
So NVMe target code needs to function in at least two different modes:
- Direct mapping of nvme backend driver provided hw queues to nvme
fabric driver provided hw queues.
- Decoding of NVMe command set for basic Read/Write/Flush I/O for
submission to existing backend drivers (eg: iblock, fileio, rd_mcp)
With the former case, it's safe to assumes there to be anywhere from a
very small amount of code involved, to no code involved for fast-path
operation.
For more involved logic like PR, ALUA, and EXTENDED_COPY, I think both
modes will still mostly likely handle some aspects of this in software,
and not entirely behind a backend nvme host hw interface.
--nab
^ permalink raw reply
* DEFINE_IDA causing memory leaks? (was Re: [PATCH 1/2] virtio: fix memory leak of virtio ida cache layers)
From: Michael S. Tsirkin @ 2015-09-17 5:33 UTC (permalink / raw)
To: Suman Anna
Cc: linux-scsi, JBottomley, linux-kernel, virtualization, open-iscsi
In-Reply-To: <1442449758-14594-2-git-send-email-s-anna@ti.com>
On Wed, Sep 16, 2015 at 07:29:17PM -0500, Suman Anna wrote:
> The virtio core uses a static ida named virtio_index_ida for
> assigning index numbers to virtio devices during registration.
> The ida core may allocate some internal idr cache layers and
> an ida bitmap upon any ida allocation, and all these layers are
> truely freed only upon the ida destruction. The virtio_index_ida
> is not destroyed at present, leading to a memory leak when using
> the virtio core as a module and atleast one virtio device is
> registered and unregistered.
>
> Fix this by invoking ida_destroy() in the virtio core module
> exit.
>
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Signed-off-by: Suman Anna <s-anna@ti.com>
Interesting.
Will the same apply to e.g. sd_index_ida in drivers/scsi/sd.c
or iscsi_sess_ida in drivers/scsi/scsi_transport_iscsi.c?
If no, why not?
One doesn't generally expect to have to free global variables.
Maybe we should forbid DEFINE_IDA in modules?
James, could you comment on this please?
> ---
> drivers/virtio/virtio.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> index b1877d73fa56..7062bb0975a5 100644
> --- a/drivers/virtio/virtio.c
> +++ b/drivers/virtio/virtio.c
> @@ -412,6 +412,7 @@ static int virtio_init(void)
> static void __exit virtio_exit(void)
> {
> bus_unregister(&virtio_bus);
> + ida_destroy(&virtio_index_ida);
> }
> core_initcall(virtio_init);
> module_exit(virtio_exit);
> --
> 2.5.0
^ permalink raw reply
* [PATCH 2/2] remoteproc: fix memory leak of remoteproc ida cache layers
From: Suman Anna @ 2015-09-17 0:29 UTC (permalink / raw)
To: Ohad Ben-Cohen, Michael S. Tsirkin
Cc: Suman Anna, linux-kernel, virtualization
In-Reply-To: <1442449758-14594-1-git-send-email-s-anna@ti.com>
The remoteproc core uses a static ida named rproc_dev_index for
assigning an automatic index number to a registered remoteproc.
The ida core may allocate some internal idr cache layers and ida
bitmap upon any ida allocation, and all these layers are truely
freed only upon the ida destruction. The rproc_dev_index ida is
not destroyed at present, leading to a memory leak when using the
remoteproc core as a module and atleast one rproc device is
registered and unregistered.
Fix this by invoking ida_destroy() in the remoteproc core module
exit.
Cc: Ohad Ben-Cohen <ohad@wizery.com>
Signed-off-by: Suman Anna <s-anna@ti.com>
---
drivers/remoteproc/remoteproc_core.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 8b3130f22b42..9e03d158f411 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -1478,6 +1478,8 @@ module_init(remoteproc_init);
static void __exit remoteproc_exit(void)
{
+ ida_destroy(&rproc_dev_index);
+
rproc_exit_debugfs();
}
module_exit(remoteproc_exit);
--
2.5.0
^ permalink raw reply related
* [PATCH 1/2] virtio: fix memory leak of virtio ida cache layers
From: Suman Anna @ 2015-09-17 0:29 UTC (permalink / raw)
To: Ohad Ben-Cohen, Michael S. Tsirkin
Cc: Suman Anna, linux-kernel, virtualization
In-Reply-To: <1442449758-14594-1-git-send-email-s-anna@ti.com>
The virtio core uses a static ida named virtio_index_ida for
assigning index numbers to virtio devices during registration.
The ida core may allocate some internal idr cache layers and
an ida bitmap upon any ida allocation, and all these layers are
truely freed only upon the ida destruction. The virtio_index_ida
is not destroyed at present, leading to a memory leak when using
the virtio core as a module and atleast one virtio device is
registered and unregistered.
Fix this by invoking ida_destroy() in the virtio core module
exit.
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Signed-off-by: Suman Anna <s-anna@ti.com>
---
drivers/virtio/virtio.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
index b1877d73fa56..7062bb0975a5 100644
--- a/drivers/virtio/virtio.c
+++ b/drivers/virtio/virtio.c
@@ -412,6 +412,7 @@ static int virtio_init(void)
static void __exit virtio_exit(void)
{
bus_unregister(&virtio_bus);
+ ida_destroy(&virtio_index_ida);
}
core_initcall(virtio_init);
module_exit(virtio_exit);
--
2.5.0
^ permalink raw reply related
* [PATCH 0/2] Fix memory leaks in virtio & remoteproc cores
From: Suman Anna @ 2015-09-17 0:29 UTC (permalink / raw)
To: Ohad Ben-Cohen, Michael S. Tsirkin
Cc: Suman Anna, linux-kernel, virtualization
Hi,
The following patches fix couple of memory leaks in the virtio and
remoteproc cores when using these as modules, and going through a
cycle of insmod and rmmod with at least a device registered with
the corresponding cores in between. I ran into this on our downstream
product kernels on both 3.14 and 4.1 based kernels, and should apply
to the latest kernel as well.
Patches can be picked up independently through the respective trees.
regards
Suman
Suman Anna (2):
virtio: fix memory leak of virtio ida cache layers
remoteproc: fix memory leak of remoteproc ida cache layers
drivers/remoteproc/remoteproc_core.c | 2 ++
drivers/virtio/virtio.c | 1 +
2 files changed, 3 insertions(+)
--
2.5.0
^ permalink raw reply
* Re: vhost: build failure
From: Sudip Mukherjee @ 2015-09-16 9:50 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: netdev, linux-kernel, kvm, virtualization
In-Reply-To: <20150916112119-mutt-send-email-mst@redhat.com>
On Wed, Sep 16, 2015 at 11:36:45AM +0300, Michael S. Tsirkin wrote:
> On Wed, Sep 16, 2015 at 01:50:08PM +0530, Sudip Mukherjee wrote:
> > Hi,
> > While crosscompiling the kernel for openrisc with allmodconfig the build
> > failed with the error:
> > drivers/vhost/vhost.c: In function 'vhost_vring_ioctl':
> > drivers/vhost/vhost.c:818:3: error: call to '__compiletime_assert_818' declared with attribute error: BUILD_BUG_ON failed: __alignof__
> > *vq->avail > VRING_AVAIL_ALIGN_SIZE
> >
> > Can you please give me any idea about what the problem might be and how
> > it can be solved.
> >
> > You can see the build log at:
> > https://travis-ci.org/sudipm-mukherjee/parport/jobs/80365425
> >
> > regards
> > sudip
>
> Yes - I think I saw this already.
> I think the openrisc cross-compiler is broken.
I thought so. Thanks for the quick reply. I will open a bug in gcc and
lets see what they say.
regards
sudip
^ permalink raw reply
* Re: vhost: build failure
From: Michael S. Tsirkin @ 2015-09-16 8:36 UTC (permalink / raw)
To: Sudip Mukherjee; +Cc: netdev, linux-kernel, kvm, virtualization
In-Reply-To: <20150916082008.GA28512@sudip-pc>
On Wed, Sep 16, 2015 at 01:50:08PM +0530, Sudip Mukherjee wrote:
> Hi,
> While crosscompiling the kernel for openrisc with allmodconfig the build
> failed with the error:
> drivers/vhost/vhost.c: In function 'vhost_vring_ioctl':
> drivers/vhost/vhost.c:818:3: error: call to '__compiletime_assert_818' declared with attribute error: BUILD_BUG_ON failed: __alignof__
> *vq->avail > VRING_AVAIL_ALIGN_SIZE
>
> Can you please give me any idea about what the problem might be and how
> it can be solved.
>
> You can see the build log at:
> https://travis-ci.org/sudipm-mukherjee/parport/jobs/80365425
>
> regards
> sudip
Yes - I think I saw this already.
I think the openrisc cross-compiler is broken.
VRING_AVAIL_ALIGN_SIZE is 2
*vq->avail is:
struct vring_avail {
__virtio16 flags;
__virtio16 idx;
__virtio16 ring[];
};
And __virtio16 is just a u16 with some sparse annotations.
Looking at openrisc architecture document:
Operand: Length addr[3:0] if aligned
Halfword (or half) 2 bytes Xxx0
Type C-TYPE Sizeof Alignment Openrisc Equivalent
Short Signed short 2 2 Signed halfword
and
16.1.2
Aggregates and Unions
Aggregates (structures and arrays) and unions assume the alignment of their most
strictly aligned element.
So to me, it looks like your gcc violates the ABI
by adding alignment requirements > 2.
--
MST
^ permalink raw reply
* vhost: build failure
From: Sudip Mukherjee @ 2015-09-16 8:20 UTC (permalink / raw)
To: mst; +Cc: netdev, linux-kernel, kvm, virtualization
Hi,
While crosscompiling the kernel for openrisc with allmodconfig the build
failed with the error:
drivers/vhost/vhost.c: In function 'vhost_vring_ioctl':
drivers/vhost/vhost.c:818:3: error: call to '__compiletime_assert_818' declared with attribute error: BUILD_BUG_ON failed: __alignof__
*vq->avail > VRING_AVAIL_ALIGN_SIZE
Can you please give me any idea about what the problem might be and how
it can be solved.
You can see the build log at:
https://travis-ci.org/sudipm-mukherjee/parport/jobs/80365425
regards
sudip
^ permalink raw reply
* Re: [virtio-dev] rfc: vhost user enhancements for vm2vm communication
From: Stefan Hajnoczi @ 2015-09-14 16:00 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: virtio-dev, Jan Kiszka, Claudio.Fontana, qemu-devel,
virtualization, opnfv-tech-discuss
In-Reply-To: <20150831160655-mutt-send-email-mst@redhat.com>
On Mon, Aug 31, 2015 at 05:11:02PM +0300, Michael S. Tsirkin wrote:
> The resulting channel might look something like the following:
>
> +-- VM1 --------------+ +---VM2-----------+
> | virtio-pci -- iommu +--+ vhost-pci -- VF | -- VFIO -- IOMMU -- NIC
> +---------------------+ +-----------------+
>
> comparing the two diagrams, a vhost-user thread on the host is
> no longer required, reducing the host CPU utilization when
> polling is active. At the same time, VM2 can not access all of VM1's
> memory - it is limited by the iommu configuration setup by VM1.
Can this use virtio's vring? If standard virtio devices (net, blk, etc)
cannot be used because this scheme requires new descriptor rings or
memory layout, then this is more an "ivshmem 2.0" than "virtio".
I'm not clear on how vhost-pci works - is this a host kernel component
that updates VM2's memory mappings when VM1 changes iommu entries?
In VM2 there is a userspace network router. It can mmap the VF's BARs
to access the physical network. What about the virtual NIC to VM1, how
does the userspace network router access it?
^ permalink raw reply
* [Patch RESEND] x86/paravirt: remove unused operation
From: Juergen Gross @ 2015-09-14 10:42 UTC (permalink / raw)
To: linux-kernel, x86, hpa, tglx, mingo, jeremy, chrisw, akataria,
rusty, virtualization
Cc: Juergen Gross
Remove the paravirt operation "get_tsc_khz" as it is used nowhere.
Signed-off-by: Juergen Gross <jgross@suse.com>
Acked-by: Rusty Russell <rusty@rustcorp.com.au>
---
arch/x86/include/asm/paravirt_types.h | 1 -
1 file changed, 1 deletion(-)
diff --git a/arch/x86/include/asm/paravirt_types.h b/arch/x86/include/asm/paravirt_types.h
index ce029e4..31247b5 100644
--- a/arch/x86/include/asm/paravirt_types.h
+++ b/arch/x86/include/asm/paravirt_types.h
@@ -97,7 +97,6 @@ struct pv_lazy_ops {
struct pv_time_ops {
unsigned long long (*sched_clock)(void);
unsigned long long (*steal_clock)(int cpu);
- unsigned long (*get_tsc_khz)(void);
};
struct pv_cpu_ops {
--
2.1.4
^ permalink raw reply related
* Re: [PATCH] x86/paravirt: remove unused operation
From: Ingo Molnar @ 2015-09-14 10:14 UTC (permalink / raw)
To: Juergen Gross; +Cc: jeremy, x86, linux-kernel, virtualization, chrisw, akataria
In-Reply-To: <55F15F7E.3000502@suse.com>
* Juergen Gross <jgross@suse.com> wrote:
> On 08/31/2015 02:05 AM, Rusty Russell wrote:
> >Juergen Gross <jgross@suse.com> writes:
> >>Ping?
> >
> >Acked-by: Rusty Russell <rusty@rustcorp.com.au>
>
> Anyone willing to take this patch?
I wasn't Cc:-ed to the submission, please resend it with the Acked-by added.
Thanks,
Ingo
^ permalink raw reply
* Re: virtio optimization idea
From: Xie, Huawei @ 2015-09-14 3:08 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Venkatesan, Venky, Linhaifeng, dev@dpdk.org,
virtualization@lists.linux-foundation.org, Tetsuya Mukawa,
Ouyang, Changchun, Thomas Monjalon, Rogers, Gerald
In-Reply-To: <20150910101319-mutt-send-email-mst@redhat.com>
On 9/10/2015 3:20 PM, Michael S. Tsirkin wrote:
> On Thu, Sep 10, 2015 at 06:32:35AM +0000, Xie, Huawei wrote:
>> On 9/9/2015 3:34 PM, Michael S. Tsirkin wrote:
>>> On Fri, Sep 04, 2015 at 08:25:05AM +0000, Xie, Huawei wrote:
>>>> Hi:
>>>>
>>>> Recently I have done one virtio optimization proof of concept. The
>>>> optimization includes two parts:
>>>> 1) avail ring set with fixed descriptors
>>>> 2) RX vectorization
>>>> With the optimizations, we could have several times of performance boost
>>>> for purely vhost-virtio throughput.
>>> Thanks!
>>> I'm very happy to see people work on the virtio ring format
>>> optimizations.
>>>
>>> I think it's best to analyze each optimization separately,
>>> unless you see a reason why they would only give benefit when applied
>>> together.
>>
>> Agree. Will split the patch to see each change's benefit. Of course it
>> is also very common two give much more gain than the sum of individual.
>>
>>> Also ideally, we'd need a unit test to show the performance impact.
>>> We've been using the tests in tools/virtio/ under linux,
>>> feel free to enhance these to simulate more workloads, or
>>> to suggest something else entirely.
>> If possible, we would provide perf test case under tools/virtio.
>> I am interested if the optimization could help kernel virtio-net driver
>> performance, if not the other is the bottleneck.
>>> Avail ring is initialized with fixed descriptor and is never changed,
>>> i.e, the index value of the nth avail ring entry is always n, which
>>> means virtio PMD is actually refilling desc ring only, without having to
>>> change avail ring.
>>> When vhost fetches avail ring, if not evicted, it is always in its first
>>> level cache.
>>>
>>> When RX receives packets from used ring, we use the used->idx as the
>>> desc idx. This requires that vhost processes and returns descs from
>>> avail ring to used ring in order, which is true for both current dpdk
>>> vhost and kernel vhost implementation. In my understanding, there is no
>>> necessity for vhost net to process descriptors OOO. One case could be
>>> zero copy, for example, if one descriptor doesn't meet zero copy
>>> requirment, we could directly return it to used ring, earlier than the
>>> descriptors in front of it.
>>> To enforce this, i want to use a reserved bit to indicate in order
>>> processing of descriptors.
>>> So what's the point in changing the idx for the used ring?
>>> You need to communicate the length to the guest anyway, don't you?
>> For guest virtio driver, we only use the length field in the entry of
>> the used ring and don't use the index in the entry. Instead, use
>> used->idx & 255 as the desc idx for RX, and used->idx & 127 as the desc
>> idx for TX.
> OK but length and index are in the same cache line.
> As long as we read the length, does it make sense
> to skip reading the index?
I don't understand "skipping reading the index". Currently virtio RX
needs the length field, and CPU will automatically fetch the adjacent
index field in the unit of cache line, though the optimized driver
doesn't use the index field.
/huawei
>
>> For vhost driver, as it couldn't assume fixed ring layout(to support
>> legacy virtio), it needs to update the idx in the used ring entry.
>>>> For tx ring, the arrangement is like below. Each transmitted mbuf needs
>>>> a desc for virtio_net_hdr, so actually we have only 128 free slots.
>>> Just fix this one. Support ANY_LAYOUT and then you can put data
>>> linearly. And/or support INDIRECT_DESC and then you can
>>> use an indirect descriptor.
>> Would check those two features.
>>>>
>>>>
>>> This one came out corrupted.
>> Actually i ever replied to the original mail and fixed it. Copy it here
>> again.
>>
>> ++
>> ||
>> ||
>> +-----+-----+-----+--------------+------+------+------+
>> | 0 | 1 | ... | 127 || 128 | 129 | ... | 255 | avail ring
>> +--+--+--+--+-----+---+------+---+--+---+------+--+---+
>> | | | || | | |
>> v v v || v v v
>> +--+--+--+--+-----+---+------+---+--+---+------+--+---+
>> | 127 | 128 | ... | 255 || 127 | 128 | ... | 255 | desc ring for virtio_net_hdr
>> +--+--+--+--+-----+---+------+---+--+---+------+--+---+
>> | | | || | | |
>> v v v || v v v
>> +--+--+--+--+-----+---+------+---+--+---+------+--+---+
>> | 0 | 1 | ... | 127 || 0 | 1 | ... | 127 | desc ring for tx dat
>>
>>
>>>>
>>>> /huawei
>>> Please Cc virtio related discussion more widely.
>>> I added the virtualization mailing list.
>>>
>>>
>>> So what you want to do is avoid changing the avail
>>> ring, isn't it enough to pre-format it and cache
>>> the values in the guest?
>>>
>>> Host can then keep using avail ring without changes, it will stay in cache.
>>> Something like the below for guest should do the trick (untested):
>> Good optimization to tackle the cache line transfer issue.
>> Your change could avoid changing avail ring if it points to same head
>> index. It could improve kernel virtio-net driver's performance if the
>> avail ring doesn't change in running.
>> We would also investigating applying this idea to DPDK virtio PMD slow path.
>> For the optimization i did, which i call fast path, the driver "knows"
>> in theory the avail ring willn't never get changed, so it doesn't a)
>> allocate and free descriptors b) care the avail ring.
>> Based on the fact the DPDK pmd is actually using the simple desc ring
>> below, we could directly map avail->idx and used->idx to desc idx, and
>> use vector instruction to do parallel processing.
>>
>> +-+--+-+--+-+-+---------+---+--+---+
>> | 0 | 1 | 2 | ... | 254 | 255 | rx desc ring
>> +----+----+---+-------------+------+
> Yes, using these instructions in kernel is generally problematic,
> but can work in userspace. Waiting for a description of that.
About vectorization? Would do that.
/huawei
>
>
>>
>>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>>>
>>> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
>>> index 096b857..9363b50 100644
>>> --- a/drivers/virtio/virtio_ring.c
>>> +++ b/drivers/virtio/virtio_ring.c
>>> @@ -91,6 +91,7 @@ struct vring_virtqueue {
>>> bool last_add_time_valid;
>>> ktime_t last_add_time;
>>> #endif
>>> + u16 *avail;
>>>
>>> /* Tokens for callbacks. */
>>> void *data[];
>>> @@ -236,7 +237,10 @@ static inline int virtqueue_add(struct virtqueue *_vq,
>>> /* Put entry in available array (but don't update avail->idx until they
>>> * do sync). */
>>> avail = virtio16_to_cpu(_vq->vdev, vq->vring.avail->idx) & (vq->vring.num - 1);
>>> - vq->vring.avail->ring[avail] = cpu_to_virtio16(_vq->vdev, head);
>>> + if (vq->avail[avail] != head) {
>>> + vq->avail[avail] = head;
>>> + vq->vring.avail->ring[avail] = cpu_to_virtio16(_vq->vdev, head);
>>> + }
>>>
>>> /* Descriptors and available array need to be set before we expose the
>>> * new available array entries. */
>>> @@ -724,6 +728,11 @@ struct virtqueue *vring_new_virtqueue(unsigned int index,
>>> vq = kmalloc(sizeof(*vq) + sizeof(void *)*num, GFP_KERNEL);
>>> if (!vq)
>>> return NULL;
>>> + vq->avail = kzalloc(sizeof (*vq->avail) * num, GFP_KERNEL);
>>> + if (!va->avail) {
>>> + kfree(vq);
>>> + return NULL;
>>> + }
>>>
>>> vring_init(&vq->vring, num, pages, vring_align);
>>> vq->vq.callback = callback;
>>>
^ permalink raw reply
* RE: [opnfv-tech-discuss] rfc: vhost user enhancements for vm2vm communication
From: Zhang, Yang Z @ 2015-09-14 0:43 UTC (permalink / raw)
To: Michael S. Tsirkin, Claudio Fontana
Cc: qemu-devel@nongnu.org, virtio-dev@lists.oasis-open.org,
virtualization@lists.linux-foundation.org,
opnfv-tech-discuss@lists.opnfv.org, Jan Kiszka
In-Reply-To: <20150913120821-mutt-send-email-mst@redhat.com>
Michael S. Tsirkin wrote on 2015-09-13:
> On Fri, Sep 11, 2015 at 05:39:07PM +0200, Claudio Fontana wrote:
>> On 09.09.2015 09:06, Michael S. Tsirkin wrote:
>>
>> There are many consequences to this, offset within BAR alone is not
>> enough, there are multiple things at the virtio level that need sorting
>> out. Also we need to consider virtio-mmio etc.
>>
>>> This would allow VM2VM communication if there are only 2 VMs, but
>>> if data needs to be sent to multiple VMs, you must copy it.
>>
>> Not necessarily, however getting it to work (sharing the backend window
>> and arbitrating the multicast) is really hard.
>>
>>>
>>> Additionally, it's a single-purpose feature: you can use it from a
>>> userspace PMD but linux will never use it.
>>>
>>>
>>> My proposal is a superset: don't require that BAR memory is used,
>>> use IOMMU translation tables.
>>> This way, data can be sent to multiple VMs by sharing the same
>>> memory with them all.
>>
>> Can you describe in detail how your proposal deals with the
>> arbitration
> necessary for multicast handling?
>
> Basically it falls out naturally. Consider linux guest as an example,
> and assume dynamic mappings for simplicity.
>
> Multicast is done by a bridge on the guest side. That code clones the
> skb (reference-counting page fragments) and passes it to multiple ports.
> Each of these will program the IOMMU to allow read access to the
> fragments to the relevant device.
How to work with vswitch in host side like OVS? Since the flow table is inside host, but guest cannot see it.
Best regards,
Yang
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox