* Re: [PATCH 7/7] [v4] drivers/virt: introduce Freescale hypervisor management driver
From: Timur Tabi @ 2011-06-09 16:48 UTC (permalink / raw)
To: Randy Dunlap
Cc: Arnd Bergmann, konrad.wilk, greg, kumar.gala, linux-kernel,
cmetcalf, Linux Virtualization, dsaxena, linux-console,
linuxppc-dev, alan
In-Reply-To: <4DF0F7F5.5050107@oracle.com>
Randy Dunlap wrote:
>> > I'm okay with that idea, except there's a consensus that drivers should be in
>> > drivers/.
>> >
> Like sound/ ?
My understanding is that this is something that's considered broken and should
be fixed, but I don't know what the holdup is.
> but what makes it a "driver"?
That's a good point.
Ok, so maybe I don't have any really good answers here. :-)
--
Timur Tabi
Linux kernel developer at Freescale
^ permalink raw reply
* Re: [PATCH 7/7] [v4] drivers/virt: introduce Freescale hypervisor management driver
From: Randy Dunlap @ 2011-06-09 16:42 UTC (permalink / raw)
To: Timur Tabi
Cc: Arnd Bergmann, konrad.wilk, greg, kumar.gala, linux-kernel,
cmetcalf, Linux Virtualization, dsaxena, linux-console,
linuxppc-dev, alan
In-Reply-To: <4DF0F672.8040206@freescale.com>
On 06/09/11 09:36, Timur Tabi wrote:
> Randy Dunlap wrote:
>> But it sounds like virt/ needs virt/host/ and virt/guest/ to me.
>
> I'm okay with that idea, except there's a consensus that drivers should be in
> drivers/.
>
Like sound/ ?
but what makes it a "driver"?
--
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***
^ permalink raw reply
* Re: [PATCH 7/7] [v4] drivers/virt: introduce Freescale hypervisor management driver
From: Timur Tabi @ 2011-06-09 16:36 UTC (permalink / raw)
To: Randy Dunlap
Cc: Arnd Bergmann, konrad.wilk, greg, kumar.gala, linux-kernel,
cmetcalf, Linux Virtualization, dsaxena, linux-console,
linuxppc-dev, alan
In-Reply-To: <4DF0F5A1.5080008@oracle.com>
Randy Dunlap wrote:
> But it sounds like virt/ needs virt/host/ and virt/guest/ to me.
I'm okay with that idea, except there's a consensus that drivers should be in
drivers/.
--
Timur Tabi
Linux kernel developer at Freescale
^ permalink raw reply
* Re: [PATCH 7/7] [v4] drivers/virt: introduce Freescale hypervisor management driver
From: Randy Dunlap @ 2011-06-09 16:32 UTC (permalink / raw)
To: Arnd Bergmann
Cc: konrad.wilk, greg, kumar.gala, linux-kernel, cmetcalf,
Linux Virtualization, dsaxena, linux-console, linuxppc-dev,
Timur Tabi, alan
In-Reply-To: <201106090938.23027.arnd@arndb.de>
On 06/09/11 00:38, Arnd Bergmann wrote:
> On Thursday 09 June 2011 01:10:09 Randy Dunlap wrote:
>> On Wed, 8 Jun 2011 17:45:54 -0500 Timur Tabi wrote:
>>
>>> Add the drivers/virt directory, which houses drivers that support
>>> virtualization environments, and add the Freescale hypervisor management
>>> driver.
>>
>> It can't go in linux/virt or linux/virt/fsl instead? why drivers/ ?
>>
>> or maybe linux/virt should be drivers/virt ?
>
> See discussion for v2 of this patch. I suggested that drivers/firmware and virt/
> as options, the counterarguments were that drivers/firmware is for passive
> firmware as opposed to firmware that acts as a hypervisor, and that virt/ is
> for the host side of hypervisors like kvm, not for guests.
OK, I read that thread. Didn't see a real consensus there.
If you were not the drivers/misc/ maintainer, would you mind if this
driver lived in drivers/misc/? I wouldn't.
But it sounds like virt/ needs virt/host/ and virt/guest/ to me.
> The driver in here most closely resembles the xen dom0 model, where a
> priviledged guest controls other guests, but unlike xen there is a single
> driver file, so there is no need to have drivers/fsl-hv directory just
> for this one file. We do have a number of other hypervisors that fit in the
> same category, so they can be added here later.
--
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***
^ permalink raw reply
* Re: [PATCH 1/1] [virt] virtio-blk: Use ida to allocate disk index
From: Mark Wu @ 2011-06-09 10:41 UTC (permalink / raw)
To: Rusty Russell
Cc: Jens Axboe, kvm, Michael S. Tsirkin, Greg Kroah-Hartman,
linux-kernel, virtualization, Tejun Heo
In-Reply-To: <20110609091433.GB11773@htj.dyndns.org>
On 06/09/2011 05:14 AM, Tejun Heo wrote:
> Hello,
>
> On Thu, Jun 09, 2011 at 08:51:05AM +0930, Rusty Russell wrote:
>> On Wed, 08 Jun 2011 09:08:29 -0400, Mark Wu <dwu@redhat.com> wrote:
>>> Hi Rusty,
>>> Yes, I can't figure out an instance of disk probing in parallel either, but as
>>> per the following commit, I think we still need use lock for safety. What's your opinion?
>>>
>>> commit 4034cc68157bfa0b6622efe368488d3d3e20f4e6
>>> Author: Tejun Heo <tj@kernel.org>
>>> Date: Sat Feb 21 11:04:45 2009 +0900
>>>
>>> [SCSI] sd: revive sd_index_lock
>>>
>>> Commit f27bac2761cab5a2e212dea602d22457a9aa6943 which converted sd to
>>> use ida instead of idr incorrectly removed sd_index_lock around id
>>> allocation and free. idr/ida do have internal locks but they protect
>>> their free object lists not the allocation itself. The caller is
>>> responsible for that. This missing synchronization led to the same id
>>> being assigned to multiple devices leading to oops.
>>
>> I'm confused. Tejun, Greg, anyone can probes happen in parallel?
>>
>> If so, I'll have to review all my drivers.
>
> Unless async is explicitly used, probe happens sequentially. IOW, if
> there's no async_schedule() call, things won't happen in parallel.
> That said, I think it wouldn't be such a bad idea to protect ida with
> spinlock regardless unless the probe code explicitly requires
> serialization.
>
> Thanks.
>
Since virtio blk driver doesn't use async probe, it needn't use spinlock to protect ida.
So remove the lock from patch.
From fbb396df9dbf8023f1b268be01b43529a3993d57 Mon Sep 17 00:00:00 2001
From: Mark Wu <dwu@redhat.com>
Date: Thu, 9 Jun 2011 06:34:07 -0400
Subject: [PATCH 1/1] [virt] virtio-blk: Use ida to allocate disk index
Current index allocation in virtio-blk is based on a monotonically
increasing variable "index". It could cause some confusion about disk
name in the case of hot-plugging disks. And it's impossible to find the
lowest available index by just maintaining a simple index. So it's
changed to use ida to allocate index via referring to the index
allocation in scsi disk.
Signed-off-by: Mark Wu <dwu@redhat.com>
---
drivers/block/virtio_blk.c | 28 +++++++++++++++++++++++-----
1 files changed, 23 insertions(+), 5 deletions(-)
diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index 079c088..bf81ab6 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -8,10 +8,13 @@
#include <linux/scatterlist.h>
#include <linux/string_helpers.h>
#include <scsi/scsi_cmnd.h>
+#include <linux/idr.h>
#define PART_BITS 4
-static int major, index;
+static int major;
+static DEFINE_IDA(vd_index_ida);
+
struct workqueue_struct *virtblk_wq;
struct virtio_blk
@@ -23,6 +26,7 @@ struct virtio_blk
/* The disk structure for the kernel. */
struct gendisk *disk;
+ u32 index;
/* Request tracking. */
struct list_head reqs;
@@ -343,12 +347,23 @@ static int __devinit virtblk_probe(struct virtio_device *vdev)
struct request_queue *q;
int err;
u64 cap;
- u32 v, blk_size, sg_elems, opt_io_size;
+ u32 v, blk_size, sg_elems, opt_io_size, index;
u16 min_io_size;
u8 physical_block_exp, alignment_offset;
- if (index_to_minor(index) >= 1 << MINORBITS)
- return -ENOSPC;
+ do {
+ if (!ida_pre_get(&vd_index_ida, GFP_KERNEL))
+ return -ENOMEM;
+ err = ida_get_new(&vd_index_ida, &index);
+ } while (err == -EAGAIN);
+
+ if (err)
+ return err;
+
+ if (index_to_minor(index) >= 1 << MINORBITS) {
+ err = -ENOSPC;
+ goto out_free_index;
+ }
/* We need to know how many segments before we allocate. */
err = virtio_config_val(vdev, VIRTIO_BLK_F_SEG_MAX,
@@ -421,7 +436,7 @@ static int __devinit virtblk_probe(struct virtio_device *vdev)
vblk->disk->private_data = vblk;
vblk->disk->fops = &virtblk_fops;
vblk->disk->driverfs_dev = &vdev->dev;
- index++;
+ vblk->index = index;
/* configure queue flush support */
if (virtio_has_feature(vdev, VIRTIO_BLK_F_FLUSH))
@@ -516,6 +531,8 @@ out_free_vq:
vdev->config->del_vqs(vdev);
out_free_vblk:
kfree(vblk);
+out_free_index:
+ ida_remove(&vd_index_ida, index);
out:
return err;
}
@@ -538,6 +555,7 @@ static void __devexit virtblk_remove(struct virtio_device *vdev)
mempool_destroy(vblk->pool);
vdev->config->del_vqs(vdev);
kfree(vblk);
+ ida_remove(&vd_index_ida, vblk->index);
}
static const struct virtio_device_id id_table[] = {
--
1.7.1
^ permalink raw reply related
* Re: [PATCH 1/1] [virt] virtio-blk: Use ida to allocate disk index
From: Tejun Heo @ 2011-06-09 9:14 UTC (permalink / raw)
To: Rusty Russell
Cc: Jens Axboe, Mark Wu, kvm, Michael S. Tsirkin, Greg Kroah-Hartman,
linux-kernel, virtualization
In-Reply-To: <87mxhrgba6.fsf@rustcorp.com.au>
Hello,
On Thu, Jun 09, 2011 at 08:51:05AM +0930, Rusty Russell wrote:
> On Wed, 08 Jun 2011 09:08:29 -0400, Mark Wu <dwu@redhat.com> wrote:
> > Hi Rusty,
> > Yes, I can't figure out an instance of disk probing in parallel either, but as
> > per the following commit, I think we still need use lock for safety. What's your opinion?
> >
> > commit 4034cc68157bfa0b6622efe368488d3d3e20f4e6
> > Author: Tejun Heo <tj@kernel.org>
> > Date: Sat Feb 21 11:04:45 2009 +0900
> >
> > [SCSI] sd: revive sd_index_lock
> >
> > Commit f27bac2761cab5a2e212dea602d22457a9aa6943 which converted sd to
> > use ida instead of idr incorrectly removed sd_index_lock around id
> > allocation and free. idr/ida do have internal locks but they protect
> > their free object lists not the allocation itself. The caller is
> > responsible for that. This missing synchronization led to the same id
> > being assigned to multiple devices leading to oops.
>
> I'm confused. Tejun, Greg, anyone can probes happen in parallel?
>
> If so, I'll have to review all my drivers.
Unless async is explicitly used, probe happens sequentially. IOW, if
there's no async_schedule() call, things won't happen in parallel.
That said, I think it wouldn't be such a bad idea to protect ida with
spinlock regardless unless the probe code explicitly requires
serialization.
Thanks.
--
tejun
^ permalink raw reply
* Re: [PATCH 7/7] [v4] drivers/virt: introduce Freescale hypervisor management driver
From: Arnd Bergmann @ 2011-06-09 7:38 UTC (permalink / raw)
To: linuxppc-dev, Linux Virtualization
Cc: Randy Dunlap, konrad.wilk, greg, kumar.gala, linux-kernel,
cmetcalf, dsaxena, linux-console, Timur Tabi, alan
In-Reply-To: <20110608161009.7b05d305.randy.dunlap@oracle.com>
On Thursday 09 June 2011 01:10:09 Randy Dunlap wrote:
> On Wed, 8 Jun 2011 17:45:54 -0500 Timur Tabi wrote:
>
> > Add the drivers/virt directory, which houses drivers that support
> > virtualization environments, and add the Freescale hypervisor management
> > driver.
>
> It can't go in linux/virt or linux/virt/fsl instead? why drivers/ ?
>
> or maybe linux/virt should be drivers/virt ?
See discussion for v2 of this patch. I suggested that drivers/firmware and virt/
as options, the counterarguments were that drivers/firmware is for passive
firmware as opposed to firmware that acts as a hypervisor, and that virt/ is
for the host side of hypervisors like kvm, not for guests.
The driver in here most closely resembles the xen dom0 model, where a
priviledged guest controls other guests, but unlike xen there is a single
driver file, so there is no need to have drivers/fsl-hv directory just
for this one file. We do have a number of other hypervisors that fit in the
same category, so they can be added here later.
Arnd
^ permalink raw reply
* Re: virtio scsi host draft specification, v3
From: Paolo Bonzini @ 2011-06-09 6:59 UTC (permalink / raw)
To: Rusty Russell
Cc: Christoph Hellwig, Stefan Hajnoczi, kvm@vger.kernel.org,
Michael S. Tsirkin, qemu-devel, Linux Kernel Mailing List,
Linux Virtualization
In-Reply-To: <87lixbgax9.fsf@rustcorp.com.au>
On 06/09/2011 01:28 AM, Rusty Russell wrote:
>> > after some preliminary discussion on the QEMU mailing list, I present a
>> > draft specification for a virtio-based SCSI host (controller, HBA, you
>> > name it).
>
> OK, I'm impressed. This is very well written and it doesn't make any of
> the obvious mistakes wrt. virtio.
Thanks very much, and thanks to those who corrected my early mistakes.
> I assume you have an implementation, as well?
Unfortunately not; "we're working on it", which means I should start in
July when I come back from vacation.
Do you prefer to wait for one before I make a patch to the LyX source?
In the meanwhile, can you reserve a subsystem ID for me?
Paolo
^ permalink raw reply
* Re: [PATCH 1/1] [virt] virtio-blk: Use ida to allocate disk index
From: Greg KH @ 2011-06-08 23:45 UTC (permalink / raw)
To: Rusty Russell
Cc: Jens Axboe, Mark Wu, kvm, Michael S. Tsirkin, linux-kernel,
virtualization, tj
In-Reply-To: <87mxhrgba6.fsf@rustcorp.com.au>
On Thu, Jun 09, 2011 at 08:51:05AM +0930, Rusty Russell wrote:
> On Wed, 08 Jun 2011 09:08:29 -0400, Mark Wu <dwu@redhat.com> wrote:
> > Hi Rusty,
> > Yes, I can't figure out an instance of disk probing in parallel either, but as
> > per the following commit, I think we still need use lock for safety. What's your opinion?
> >
> > commit 4034cc68157bfa0b6622efe368488d3d3e20f4e6
> > Author: Tejun Heo <tj@kernel.org>
> > Date: Sat Feb 21 11:04:45 2009 +0900
> >
> > [SCSI] sd: revive sd_index_lock
> >
> > Commit f27bac2761cab5a2e212dea602d22457a9aa6943 which converted sd to
> > use ida instead of idr incorrectly removed sd_index_lock around id
> > allocation and free. idr/ida do have internal locks but they protect
> > their free object lists not the allocation itself. The caller is
> > responsible for that. This missing synchronization led to the same id
> > being assigned to multiple devices leading to oops.
>
> I'm confused. Tejun, Greg, anyone can probes happen in parallel?
>
> If so, I'll have to review all my drivers.
I know we've tried it in the past, at the PCI device level, and ran into
some issues, but I don't remember if that code ever made it into the
mainline kernel or not.
greg k-h
^ permalink raw reply
* Re: virtio scsi host draft specification, v3
From: Rusty Russell @ 2011-06-08 23:28 UTC (permalink / raw)
To: Paolo Bonzini, Linux Virtualization, Linux Kernel Mailing List, q
Cc: Christoph Hellwig, Michael S. Tsirkin, Stefan Hajnoczi,
kvm@vger.kernel.org
In-Reply-To: <4DEE2B15.4090809@redhat.com>
On Tue, 07 Jun 2011 15:43:49 +0200, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Hi all,
>
> after some preliminary discussion on the QEMU mailing list, I present a
> draft specification for a virtio-based SCSI host (controller, HBA, you
> name it).
OK, I'm impressed. This is very well written and I doesn't make any of
the obvious mistakes wrt. virtio.
Unfortunately, I know almost nothing of SCSI, so I have to leave it to
others to decide if this is actually useful and sufficient.
I assume you have an implementation, as well?
Thanks,
Rusty.
^ permalink raw reply
* Re: [PATCH 1/1] [virt] virtio-blk: Use ida to allocate disk index
From: Rusty Russell @ 2011-06-08 23:21 UTC (permalink / raw)
To: Mark Wu
Cc: Jens Axboe, kvm, Michael S. Tsirkin, Greg Kroah-Hartman,
linux-kernel, virtualization, tj
In-Reply-To: <4DEF744D.9040702@redhat.com>
On Wed, 08 Jun 2011 09:08:29 -0400, Mark Wu <dwu@redhat.com> wrote:
> Hi Rusty,
> Yes, I can't figure out an instance of disk probing in parallel either, but as
> per the following commit, I think we still need use lock for safety. What's your opinion?
>
> commit 4034cc68157bfa0b6622efe368488d3d3e20f4e6
> Author: Tejun Heo <tj@kernel.org>
> Date: Sat Feb 21 11:04:45 2009 +0900
>
> [SCSI] sd: revive sd_index_lock
>
> Commit f27bac2761cab5a2e212dea602d22457a9aa6943 which converted sd to
> use ida instead of idr incorrectly removed sd_index_lock around id
> allocation and free. idr/ida do have internal locks but they protect
> their free object lists not the allocation itself. The caller is
> responsible for that. This missing synchronization led to the same id
> being assigned to multiple devices leading to oops.
I'm confused. Tejun, Greg, anyone can probes happen in parallel?
If so, I'll have to review all my drivers.
Thanks,
Rusty.
^ permalink raw reply
* Re: [PATCH 12/49] Staging: hv: storvsc: Add a DMI signature to support auto-loading
From: Greg KH @ 2011-06-08 16:22 UTC (permalink / raw)
To: KY Srinivasan
Cc: Christoph Hellwig, gregkh@suse.de, linux-kernel@vger.kernel.org,
devel@linuxdriverproject.org, virtualization@lists.osdl.org,
Haiyang Zhang, Abhishek Kane (Mindtree Consulting PVT LTD)
In-Reply-To: <6E21E5352C11B742B20C142EB499E0480813DFC1@TK5EX14MBXC122.redmond.corp.microsoft.com>
On Wed, Jun 08, 2011 at 02:44:35AM +0000, KY Srinivasan wrote:
> > On Tue, Jun 07, 2011 at 10:19:06PM +0000, KY Srinivasan wrote:
> > > Thanks Greg. Can you give me some pointers here as to what needs to be
> > > done here. Looking at the code for virtio, it looks like I would need to add stuff
> > > to scripts/mod/file2alias.c as well as to include/linux/mod_devicetable.h. Will
> > you
> > > accept patches for these files to support an ID space for Hyper-V vmbus drivers.
> >
> > Yes, I will take patches for those files to implement this.
> >
> > Along with those files, you also need a way to tell userspace that new
> > UIDS are found before the individual drivers are loaded. Hopefully you
> > can do that now, but if not, it will need to be implemented.
>
> Could you elaborate on what user level changes I will have to do.
It's all kernel code, but you need to be emiting "device found" messages
to userspace through the hotplug bus event when you find the devices.
Just fill in the correct fields in your hotplug callback and you should
be fine.
Look at the PCI bus hotplug callback as an example of what is needed if
you need an example.
hope this helps,
greg k-h
^ permalink raw reply
* Re: [PATCH 1/1] [virt] virtio-blk: Use ida to allocate disk index
From: Mark Wu @ 2011-06-08 13:12 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: linux-kernel, kvm, virtualization
In-Reply-To: <20110602103406.GD7943@redhat.com>
On 06/02/2011 06:34 AM, Michael S. Tsirkin wrote:
> On Wed, Jun 01, 2011 at 04:25:48AM -0400, Mark Wu wrote:
>> On 06/01/2011 03:24 AM, Mark Wu wrote:
>>> - if (index_to_minor(index)>= 1<< MINORBITS)
>>> - return -ENOSPC;
>>> + do {
>>> + if (!ida_pre_get(&vd_index_ida, GFP_KERNEL))
>>> + return err;
>>> +
>> There's a problem in above code: err is not initialized before
>> using, so change it to return -1;
>> + do {
>> + if (!ida_pre_get(&vd_index_ida, GFP_KERNEL))
>> + return -1;
>
> Not -1. Pls return -ENOMEM.
>
> --
> 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
Hi Michael,
Thanks for pointing out that. This is the revised patch.
From ffe49efd20938952a09d5a87fe694a6f62937756 Mon Sep 17 00:00:00 2001
From: Mark Wu <dwu@redhat.com>
Date: Wed, 8 Jun 2011 08:25:53 -0400
Subject: [PATCH 1/1] [virt] virtio-blk: Use ida to allocate disk index
Current index allocation in virtio-blk is based on a monotonically
increasing variable "index". It could cause some confusion about disk
name in the case of hot-plugging disks. And it's impossible to find the
lowest available index by just maintaining a simple index. So it's
changed to use ida to allocate index via referring to the index
allocation in scsi disk.
Signed-off-by: Mark Wu <dwu@redhat.com>
---
drivers/block/virtio_blk.c | 37 ++++++++++++++++++++++++++++++++-----
1 files changed, 32 insertions(+), 5 deletions(-)
diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index 079c088..f13b758 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -8,10 +8,14 @@
#include <linux/scatterlist.h>
#include <linux/string_helpers.h>
#include <scsi/scsi_cmnd.h>
+#include <linux/idr.h>
#define PART_BITS 4
-static int major, index;
+static int major;
+static DEFINE_SPINLOCK(vd_index_lock);
+static DEFINE_IDA(vd_index_ida);
+
struct workqueue_struct *virtblk_wq;
struct virtio_blk
@@ -23,6 +27,7 @@ struct virtio_blk
/* The disk structure for the kernel. */
struct gendisk *disk;
+ u32 index;
/* Request tracking. */
struct list_head reqs;
@@ -343,12 +348,26 @@ static int __devinit virtblk_probe(struct virtio_device *vdev)
struct request_queue *q;
int err;
u64 cap;
- u32 v, blk_size, sg_elems, opt_io_size;
+ u32 v, blk_size, sg_elems, opt_io_size, index;
u16 min_io_size;
u8 physical_block_exp, alignment_offset;
- if (index_to_minor(index) >= 1 << MINORBITS)
- return -ENOSPC;
+ do {
+ if (!ida_pre_get(&vd_index_ida, GFP_KERNEL))
+ return -ENOMEM;
+
+ spin_lock(&vd_index_lock);
+ err = ida_get_new(&vd_index_ida, &index);
+ spin_unlock(&vd_index_lock);
+ } while (err == -EAGAIN);
+
+ if (err)
+ return err;
+
+ if (index_to_minor(index) >= 1 << MINORBITS) {
+ err = -ENOSPC;
+ goto out_free_index;
+ }
/* We need to know how many segments before we allocate. */
err = virtio_config_val(vdev, VIRTIO_BLK_F_SEG_MAX,
@@ -421,7 +440,7 @@ static int __devinit virtblk_probe(struct virtio_device *vdev)
vblk->disk->private_data = vblk;
vblk->disk->fops = &virtblk_fops;
vblk->disk->driverfs_dev = &vdev->dev;
- index++;
+ vblk->index = index;
/* configure queue flush support */
if (virtio_has_feature(vdev, VIRTIO_BLK_F_FLUSH))
@@ -516,6 +535,10 @@ out_free_vq:
vdev->config->del_vqs(vdev);
out_free_vblk:
kfree(vblk);
+out_free_index:
+ spin_lock(&vd_index_lock);
+ ida_remove(&vd_index_ida, index);
+ spin_unlock(&vd_index_lock);
out:
return err;
}
@@ -538,6 +561,10 @@ static void __devexit virtblk_remove(struct virtio_device *vdev)
mempool_destroy(vblk->pool);
vdev->config->del_vqs(vdev);
kfree(vblk);
+
+ spin_lock(&vd_index_lock);
+ ida_remove(&vd_index_ida, vblk->index);
+ spin_unlock(&vd_index_lock);
}
static const struct virtio_device_id id_table[] = {
--
1.7.1
^ permalink raw reply related
* Re: [PATCH 1/1] [virt] virtio-blk: Use ida to allocate disk index
From: Mark Wu @ 2011-06-08 13:08 UTC (permalink / raw)
To: Rusty Russell
Cc: Jens Axboe, kvm, Michael S. Tsirkin, linux-kernel, virtualization,
tj
In-Reply-To: <87mxi1xfz0.fsf@rustcorp.com.au>
On 06/01/2011 07:57 PM, Rusty Russell wrote:
> On Wed, 1 Jun 2011 03:24:29 -0400, Mark Wu <dwu@redhat.com> wrote:
>> Current index allocation in virtio-blk is based on a monotonically
>> increasing variable "index". It could cause some confusion about
>> disk name in the case of hot-plugging disks. And it's impossible
>> to find the lowest available index by just maintaining a simple
>> index. So it's changed to use ida to allocate index via referring
>> to the index allocation in scsi disk.
>>
>> Signed-off-by: Mark Wu <dwu@redhat.com>
>
> Hi Mark,
>
> I don't believe that we do disk probes in parallel, so the spinlock
> is unnecessary. Otherwise, this looks good.
>
> Thanks, Rusty.
Hi Rusty,
Yes, I can't figure out an instance of disk probing in parallel either, but as
per the following commit, I think we still need use lock for safety. What's your opinion?
commit 4034cc68157bfa0b6622efe368488d3d3e20f4e6
Author: Tejun Heo <tj@kernel.org>
Date: Sat Feb 21 11:04:45 2009 +0900
[SCSI] sd: revive sd_index_lock
Commit f27bac2761cab5a2e212dea602d22457a9aa6943 which converted sd to
use ida instead of idr incorrectly removed sd_index_lock around id
allocation and free. idr/ida do have internal locks but they protect
their free object lists not the allocation itself. The caller is
responsible for that. This missing synchronization led to the same id
being assigned to multiple devices leading to oops.
^ permalink raw reply
* RE: [PATCH 12/49] Staging: hv: storvsc: Add a DMI signature to support auto-loading
From: KY Srinivasan @ 2011-06-08 2:44 UTC (permalink / raw)
To: Greg KH
Cc: Christoph Hellwig, gregkh@suse.de, linux-kernel@vger.kernel.org,
devel@linuxdriverproject.org, virtualization@lists.osdl.org,
Haiyang Zhang, Abhishek Kane (Mindtree Consulting PVT LTD)
In-Reply-To: <20110607222444.GA26968@kroah.com>
> -----Original Message-----
> From: Greg KH [mailto:greg@kroah.com]
> Sent: Tuesday, June 07, 2011 6:25 PM
> To: KY Srinivasan
> Cc: Christoph Hellwig; gregkh@suse.de; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; virtualization@lists.osdl.org; Haiyang Zhang;
> Abhishek Kane (Mindtree Consulting PVT LTD)
> Subject: Re: [PATCH 12/49] Staging: hv: storvsc: Add a DMI signature to support
> auto-loading
>
> On Tue, Jun 07, 2011 at 10:19:06PM +0000, KY Srinivasan wrote:
> > Thanks Greg. Can you give me some pointers here as to what needs to be
> > done here. Looking at the code for virtio, it looks like I would need to add stuff
> > to scripts/mod/file2alias.c as well as to include/linux/mod_devicetable.h. Will
> you
> > accept patches for these files to support an ID space for Hyper-V vmbus drivers.
>
> Yes, I will take patches for those files to implement this.
>
> Along with those files, you also need a way to tell userspace that new
> UIDS are found before the individual drivers are loaded. Hopefully you
> can do that now, but if not, it will need to be implemented.
Could you elaborate on what user level changes I will have to do.
Thanks,
K. Y
^ permalink raw reply
* Re: [PATCHv2 RFC 4/4] Revert "virtio: make add_buf return capacity remaining:
From: Rusty Russell @ 2011-06-08 0:19 UTC (permalink / raw)
To: Michael S. Tsirkin, linux-kernel
Cc: Krishna Kumar, Carsten Otte, lguest, Shirley Ma, kvm, linux-s390,
netdev, habanero, Heiko Carstens, virtualization, steved,
Christian Borntraeger, Tom Lendacky, Martin Schwidefsky, linux390
In-Reply-To: <20110607155457.GA17436@redhat.com>
On Tue, 7 Jun 2011 18:54:57 +0300, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Thu, Jun 02, 2011 at 06:43:25PM +0300, Michael S. Tsirkin wrote:
> > This reverts commit 3c1b27d5043086a485f8526353ae9fe37bfa1065.
> > The only user was virtio_net, and it switched to
> > min_capacity instead.
> >
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>
> It turns out another place in virtio_net: receive
> buf processing - relies on the old behaviour:
>
> try_fill_recv:
> do {
> if (vi->mergeable_rx_bufs)
> err = add_recvbuf_mergeable(vi, gfp);
> else if (vi->big_packets)
> err = add_recvbuf_big(vi, gfp);
> else
> err = add_recvbuf_small(vi, gfp);
>
> oom = err == -ENOMEM;
> if (err < 0)
> break;
> ++vi->num;
> } while (err > 0);
>
> The point is to avoid allocating a buf if
> the ring is out of space and we are sure
> add_buf will fail.
>
> It works well for mergeable buffers and for big
> packets if we are not OOM. small packets and
> oom will do extra get_page/put_page calls
> (but maybe we don't care).
>
> So this is RX, I intend to drop it from this patchset and focus on the
> TX side for starters.
We could do some hack where we get the capacity, and estimate how many
packets we need to fill it, then try to do that many.
I say hack, because knowing whether we're doing indirect buffers is a
layering violation. But that's life when you're trying to do
microoptimizations.
Cheers,
Rusty.
^ permalink raw reply
* Re: [PATCH 12/49] Staging: hv: storvsc: Add a DMI signature to support auto-loading
From: Greg KH @ 2011-06-07 22:24 UTC (permalink / raw)
To: KY Srinivasan
Cc: Christoph Hellwig, gregkh@suse.de, linux-kernel@vger.kernel.org,
devel@linuxdriverproject.org, virtualization@lists.osdl.org,
Haiyang Zhang, Abhishek Kane (Mindtree Consulting PVT LTD)
In-Reply-To: <6E21E5352C11B742B20C142EB499E0480813DD09@TK5EX14MBXC122.redmond.corp.microsoft.com>
On Tue, Jun 07, 2011 at 10:19:06PM +0000, KY Srinivasan wrote:
> Thanks Greg. Can you give me some pointers here as to what needs to be
> done here. Looking at the code for virtio, it looks like I would need to add stuff
> to scripts/mod/file2alias.c as well as to include/linux/mod_devicetable.h. Will you
> accept patches for these files to support an ID space for Hyper-V vmbus drivers.
Yes, I will take patches for those files to implement this.
Along with those files, you also need a way to tell userspace that new
UIDS are found before the individual drivers are loaded. Hopefully you
can do that now, but if not, it will need to be implemented.
thanks,
greg k-h
^ permalink raw reply
* RE: [PATCH 12/49] Staging: hv: storvsc: Add a DMI signature to support auto-loading
From: KY Srinivasan @ 2011-06-07 22:19 UTC (permalink / raw)
To: Greg KH
Cc: Christoph Hellwig, gregkh@suse.de, linux-kernel@vger.kernel.org,
devel@linuxdriverproject.org, virtualization@lists.osdl.org,
Haiyang Zhang, Abhishek Kane (Mindtree Consulting PVT LTD)
In-Reply-To: <20110607185813.GA3531@kroah.com>
> -----Original Message-----
> From: Greg KH [mailto:greg@kroah.com]
> Sent: Tuesday, June 07, 2011 2:58 PM
> To: KY Srinivasan
> Cc: Christoph Hellwig; gregkh@suse.de; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; virtualization@lists.osdl.org; Haiyang Zhang;
> Abhishek Kane (Mindtree Consulting PVT LTD)
> Subject: Re: [PATCH 12/49] Staging: hv: storvsc: Add a DMI signature to support
> auto-loading
>
> On Tue, Jun 07, 2011 at 02:54:25PM +0000, KY Srinivasan wrote:
> >
> >
> > > -----Original Message-----
> > > From: Christoph Hellwig [mailto:hch@infradead.org]
> > > Sent: Monday, June 06, 2011 7:13 PM
> > > To: KY Srinivasan
> > > Cc: gregkh@suse.de; linux-kernel@vger.kernel.org;
> > > devel@linuxdriverproject.org; virtualization@lists.osdl.org; Haiyang Zhang;
> > > Abhishek Kane (Mindtree Consulting PVT LTD)
> > > Subject: Re: [PATCH 12/49] Staging: hv: storvsc: Add a DMI signature to
> support
> > > auto-loading
> > >
> > > On Mon, Jun 06, 2011 at 03:49:36PM -0700, K. Y. Srinivasan wrote:
> > > > To support auto-loading the storvsc driver, add a DMI signature.
> > >
> > > The storvsc driver is not a DMI driver, but a vmbus driver. As such it
> > > should have a vmbus table that is used for autoloading, not a dmi one.
> > >
> >
> > A while ago, Greg introduced DMI signatures to these drivers to support
> > auto-loading. This signature is not used for anything else. For some reason,
> > this storvsc driver was missing this signature. I added it again to only support
> > auto-loading.
>
> Yes, that was there to solve the original problem of autoloading the
> driver.
>
> But Christoph is correct here, the vmbus needs a way to autoload its own
> drivers based on the GUID signatures of the devices it finds on the
> hyperv bus. So the correct thing to do in the long-run is to implement
> this (which is one of the things the bus needs to do before it can move
> out of staging).
>
> I'll take this patch as-is for now though, as it does solve the
> immediate issue.
Thanks Greg. Can you give me some pointers here as to what needs to be
done here. Looking at the code for virtio, it looks like I would need to add stuff
to scripts/mod/file2alias.c as well as to include/linux/mod_devicetable.h. Will you
accept patches for these files to support an ID space for Hyper-V vmbus drivers.
Regards,
K. Y
^ permalink raw reply
* RE: [PATCH 49/49] Staging: hv: vmbus: Increase the timeout value in vmbus_request_offers()
From: KY Srinivasan @ 2011-06-07 21:39 UTC (permalink / raw)
To: Greg KH
Cc: Greg KH, linux-kernel@vger.kernel.org,
devel@linuxdriverproject.org, virtualization@lists.osdl.org,
Haiyang Zhang, Abhishek Kane (Mindtree Consulting PVT LTD)
In-Reply-To: <20110607212800.GA3331@suse.de>
> -----Original Message-----
> From: Greg KH [mailto:gregkh@suse.de]
> Sent: Tuesday, June 07, 2011 5:28 PM
> To: KY Srinivasan
> Cc: Greg KH; linux-kernel@vger.kernel.org; devel@linuxdriverproject.org;
> virtualization@lists.osdl.org; Haiyang Zhang; Abhishek Kane (Mindtree Consulting
> PVT LTD)
> Subject: Re: [PATCH 49/49] Staging: hv: vmbus: Increase the timeout value in
> vmbus_request_offers()
>
> On Tue, Jun 07, 2011 at 09:20:16PM +0000, KY Srinivasan wrote:
> >
> >
> > > -----Original Message-----
> > > From: Greg KH [mailto:greg@kroah.com]
> > > Sent: Tuesday, June 07, 2011 4:45 PM
> > > To: KY Srinivasan
> > > Cc: gregkh@suse.de; linux-kernel@vger.kernel.org;
> > > devel@linuxdriverproject.org; virtualization@lists.osdl.org; Haiyang Zhang;
> > > Abhishek Kane (Mindtree Consulting PVT LTD)
> > > Subject: Re: [PATCH 49/49] Staging: hv: vmbus: Increase the timeout value in
> > > vmbus_request_offers()
> > >
> > > On Mon, Jun 06, 2011 at 03:50:13PM -0700, K. Y. Srinivasan wrote:
> > > > Increase the timeout value in vmbus_request_offers().
> > >
> > > Why? What does this solve/fix/prevent?
> >
> > On some very loaded systems, when we tested the 1 second timeout that
> > we had prior to this patch was insufficient. So I bumped it to 5 seconds.
>
> Ok, then this should be something that goes into 3.0 and older kernels,
> right?
>
> If so, please say so, with this type of description, when you submit it.
>
> Remember, don't say only what you did in the changelog comment, but why
> you did it if it's not obvious (and it wasn't here.)
>
> Care to resend this one with this information?
Will do.
Thanks,
K. Y
^ permalink raw reply
* Re: [PATCH 49/49] Staging: hv: vmbus: Increase the timeout value in vmbus_request_offers()
From: Greg KH @ 2011-06-07 21:28 UTC (permalink / raw)
To: KY Srinivasan
Cc: Greg KH, linux-kernel@vger.kernel.org,
devel@linuxdriverproject.org, virtualization@lists.osdl.org,
Haiyang Zhang, Abhishek Kane (Mindtree Consulting PVT LTD)
In-Reply-To: <6E21E5352C11B742B20C142EB499E0480813DC6F@TK5EX14MBXC122.redmond.corp.microsoft.com>
On Tue, Jun 07, 2011 at 09:20:16PM +0000, KY Srinivasan wrote:
>
>
> > -----Original Message-----
> > From: Greg KH [mailto:greg@kroah.com]
> > Sent: Tuesday, June 07, 2011 4:45 PM
> > To: KY Srinivasan
> > Cc: gregkh@suse.de; linux-kernel@vger.kernel.org;
> > devel@linuxdriverproject.org; virtualization@lists.osdl.org; Haiyang Zhang;
> > Abhishek Kane (Mindtree Consulting PVT LTD)
> > Subject: Re: [PATCH 49/49] Staging: hv: vmbus: Increase the timeout value in
> > vmbus_request_offers()
> >
> > On Mon, Jun 06, 2011 at 03:50:13PM -0700, K. Y. Srinivasan wrote:
> > > Increase the timeout value in vmbus_request_offers().
> >
> > Why? What does this solve/fix/prevent?
>
> On some very loaded systems, when we tested the 1 second timeout that
> we had prior to this patch was insufficient. So I bumped it to 5 seconds.
Ok, then this should be something that goes into 3.0 and older kernels,
right?
If so, please say so, with this type of description, when you submit it.
Remember, don't say only what you did in the changelog comment, but why
you did it if it's not obvious (and it wasn't here.)
Care to resend this one with this information?
thanks,
greg k-h
^ permalink raw reply
* RE: [PATCH 49/49] Staging: hv: vmbus: Increase the timeout value in vmbus_request_offers()
From: KY Srinivasan @ 2011-06-07 21:20 UTC (permalink / raw)
To: Greg KH
Cc: gregkh@suse.de, linux-kernel@vger.kernel.org,
devel@linuxdriverproject.org, virtualization@lists.osdl.org,
Haiyang Zhang, Abhishek Kane (Mindtree Consulting PVT LTD)
In-Reply-To: <20110607204514.GB16429@kroah.com>
> -----Original Message-----
> From: Greg KH [mailto:greg@kroah.com]
> Sent: Tuesday, June 07, 2011 4:45 PM
> To: KY Srinivasan
> Cc: gregkh@suse.de; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; virtualization@lists.osdl.org; Haiyang Zhang;
> Abhishek Kane (Mindtree Consulting PVT LTD)
> Subject: Re: [PATCH 49/49] Staging: hv: vmbus: Increase the timeout value in
> vmbus_request_offers()
>
> On Mon, Jun 06, 2011 at 03:50:13PM -0700, K. Y. Srinivasan wrote:
> > Increase the timeout value in vmbus_request_offers().
>
> Why? What does this solve/fix/prevent?
On some very loaded systems, when we tested the 1 second timeout that
we had prior to this patch was insufficient. So I bumped it to 5 seconds.
Regards,
K. Y
^ permalink raw reply
* Re: [PATCH 00/49] Staging: hv: Driver cleanup
From: Greg KH @ 2011-06-07 20:48 UTC (permalink / raw)
To: K. Y. Srinivasan; +Cc: devel, gregkh, linux-kernel, virtualization
In-Reply-To: <1307400540-13193-1-git-send-email-kys@microsoft.com>
On Mon, Jun 06, 2011 at 03:49:00PM -0700, K. Y. Srinivasan wrote:
> Further cleanup of the hv drivers:
>
> 1) Continue to cleanup our drivers to conform to the Linux Driver
> Model.
>
> 2) Fix some long standing bugs with regards to unloading and
> reloading the drivers - block, net and stor.
>
> 3) VMBUS is an ACPI enumerated device; make VMBUS an ACPI bus driver.
Ok, if that's the way the bus really is, it's good to get rid of the pci
code.
> 4) Get rid of channel polling code; instead the channel receive paths
> will be purely interrupt drive.
>
> 5) Cleanup error return values in the vmbus driver and general cleanup.
>
> 6) Fix memory barrier calls in the ring buffer code.
>
> 7) Increase the timeout values for some critical guest to host calls.
I applied all of these but the last one.
For the last patch, please provide more information about it. Should it
also go to older kernel releases to resolve issues found there?
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH 49/49] Staging: hv: vmbus: Increase the timeout value in vmbus_request_offers()
From: Greg KH @ 2011-06-07 20:45 UTC (permalink / raw)
To: K. Y. Srinivasan
Cc: Abhishek Kane, Haiyang Zhang, gregkh, linux-kernel,
virtualization, devel
In-Reply-To: <1307400613-13234-49-git-send-email-kys@microsoft.com>
On Mon, Jun 06, 2011 at 03:50:13PM -0700, K. Y. Srinivasan wrote:
> Increase the timeout value in vmbus_request_offers().
Why? What does this solve/fix/prevent?
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH 15/49] Staging: hv: vmbus: Make vmbus an acpi bus driver
From: Greg KH @ 2011-06-07 20:44 UTC (permalink / raw)
To: K. Y. Srinivasan
Cc: gregkh, linux-kernel, devel, virtualization, Haiyang Zhang,
Abhishek Kane
In-Reply-To: <1307400613-13234-15-git-send-email-kys@microsoft.com>
On Mon, Jun 06, 2011 at 03:49:39PM -0700, K. Y. Srinivasan wrote:
> Now, make the vmbus driver an ACPI bus driver.
Why an ACPI bus driver and not a PCI one? Is that what this really is?
For some reason, I thought it was a PCI device that we hooked our bus
off of, is that not true?
greg k-h
^ permalink raw reply
* Re: [PATCH 24/49] Staging: hv: vmbus: Get rid of the unused wrapper - vmbus_onchannel_event()
From: Greg KH @ 2011-06-07 20:09 UTC (permalink / raw)
To: KY Srinivasan
Cc: Greg KH, Christoph Hellwig, linux-kernel@vger.kernel.org,
devel@linuxdriverproject.org, virtualization@lists.osdl.org,
Haiyang Zhang, Abhishek Kane (Mindtree Consulting PVT LTD)
In-Reply-To: <6E21E5352C11B742B20C142EB499E0480813DAF6@TK5EX14MBXC122.redmond.corp.microsoft.com>
On Tue, Jun 07, 2011 at 07:58:50PM +0000, KY Srinivasan wrote:
>
>
> > -----Original Message-----
> > From: Greg KH [mailto:greg@kroah.com]
> > Sent: Tuesday, June 07, 2011 2:59 PM
> > To: KY Srinivasan
> > Cc: Christoph Hellwig; gregkh@suse.de; linux-kernel@vger.kernel.org;
> > devel@linuxdriverproject.org; virtualization@lists.osdl.org; Haiyang Zhang;
> > Abhishek Kane (Mindtree Consulting PVT LTD)
> > Subject: Re: [PATCH 24/49] Staging: hv: vmbus: Get rid of the unused wrapper -
> > vmbus_onchannel_event()
> >
> > On Tue, Jun 07, 2011 at 02:59:32PM +0000, KY Srinivasan wrote:
> > >
> > >
> > > > -----Original Message-----
> > > > From: Christoph Hellwig [mailto:hch@infradead.org]
> > > > Sent: Monday, June 06, 2011 7:15 PM
> > > > To: KY Srinivasan
> > > > Cc: gregkh@suse.de; linux-kernel@vger.kernel.org;
> > > > devel@linuxdriverproject.org; virtualization@lists.osdl.org; Haiyang Zhang;
> > > > Abhishek Kane (Mindtree Consulting PVT LTD)
> > > > Subject: Re: [PATCH 24/49] Staging: hv: vmbus: Get rid of the unused wrapper
> > -
> > > > vmbus_onchannel_event()
> > > >
> > > > On Mon, Jun 06, 2011 at 03:49:48PM -0700, K. Y. Srinivasan wrote:
> > > > > Now, get rid of the unused wrapper - vmbus_onchannel_event().
> > > >
> > > > I'd merge this into the previous patch. In general your patch split
> > > > seem a little too fine grained to me in general. When you remove a
> > > > wrapper you can inline it into the callsite directly, if you clean up a
> > > > function directly inlining it into the helper is fine, etc.
> > > >
> > > I agree with you that some of these patches are too fine grained; but
> > > I thought that was what was expected - "one change per patch".
> >
> > Yes, but don't take it to an extreme, like you have done here :)
>
> Will do in the future. Should I re-spin any of the patches in this set to make them less
> "fine-grained".
No, they are fine as-is.
^ 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