* Re: [RFC PATCH TRIVIAL] Reading the virtio code...
From: Rusty Russell @ 2011-04-27 5:29 UTC (permalink / raw)
To: Rob Landley, virtualization; +Cc: Michael S. Tsirkin
In-Reply-To: <4DB35D1E.3030509@parallels.com>
On Sat, 23 Apr 2011 18:13:34 -0500, Rob Landley <rlandley@parallels.com> wrote:
> From: Rob Landley <rlandley@parallels.com>
>
> Going indirect for only two buffers isn't likely to be a performance win
> because the kmalloc/kfree overhead for the indirect block can't be cheaper
> than one extra linked list traversal.
Unfortunately it's not completely clear. QEMU sets fairly small rings,
and the virtio-net driver uses 2 descriptors minimum. The effect can be
a real bottleneck for small packets.
Now, virtio-net could often stuff the virtio_net_hdr in the space before
the packet data (saving a descriptor) but I think that will need a
feature bit since qemu (incorrectly) used to insist on a separate
descriptor for that header.
> Properly "tuning" the threshold would probably be workload-specific.
> (One big downside of not going indirect is extra pressure on the table
> entries, and table size varies.) But I think that in the general case,
> 2 is a defensible minimum?
I'd be tempted to say that once we fill the ring, we should drop the
threshold.
Michael?
Thanks,
Rusty.
^ permalink raw reply
* Re: [RFC PATCH TRIVIAL] Reading the virtio code...
From: Michael S. Tsirkin @ 2011-04-27 6:26 UTC (permalink / raw)
To: Rusty Russell; +Cc: Rob Landley, virtualization
In-Reply-To: <87aafcl0so.fsf@rustcorp.com.au>
On Wed, Apr 27, 2011 at 02:59:27PM +0930, Rusty Russell wrote:
> On Sat, 23 Apr 2011 18:13:34 -0500, Rob Landley <rlandley@parallels.com> wrote:
> > From: Rob Landley <rlandley@parallels.com>
> >
> > Going indirect for only two buffers isn't likely to be a performance win
> > because the kmalloc/kfree overhead for the indirect block can't be cheaper
> > than one extra linked list traversal.
>
> Unfortunately it's not completely clear. QEMU sets fairly small rings,
> and the virtio-net driver uses 2 descriptors minimum. The effect can be
> a real bottleneck for small packets.
>
> Now, virtio-net could often stuff the virtio_net_hdr in the space before
> the packet data (saving a descriptor) but I think that will need a
> feature bit since qemu (incorrectly) used to insist on a separate
> descriptor for that header.
>
> > Properly "tuning" the threshold would probably be workload-specific.
> > (One big downside of not going indirect is extra pressure on the table
> > entries, and table size varies.) But I think that in the general case,
> > 2 is a defensible minimum?
>
> I'd be tempted to say that once we fill the ring, we should drop the
> threshold.
>
> Michael?
>
> Thanks,
> Rusty.
Yes, one idea is to use part of a ring (e.g. 1/4 of a ring) for direct
entries, and the rest for indirect. So we end up with a threshold like
max(1, vq->num_free - in - out - vq->num * N)
(above is pseudo-code, must take care of unsigned vs signed etc)
and I think I'd try with N = 3/4 or maybe N = 1/2
--
MST
^ permalink raw reply
* Re: [PATCH 00/25] Staging: hv: Cleanup vmbus driver code
From: Christoph Hellwig @ 2011-04-27 6:45 UTC (permalink / raw)
To: KY Srinivasan
Cc: Greg KH, gregkh@suse.de, linux-kernel@vger.kernel.org,
devel@linuxdriverproject.org, virtualization@lists.osdl.org
In-Reply-To: <6E21E5352C11B742B20C142EB499E0481DD185@TK5EX14MBXC124.redmond.corp.microsoft.com>
On Wed, Apr 27, 2011 at 01:54:02AM +0000, KY Srinivasan wrote:
> I would prefer that we go through the review process. What is the process for
> this review? Is there a time window for people to respond. I am hoping I will be able
> to address all the review comments well in advance of the next closing of the tree,
> with the hope of taking the vmbus driver out of staging this go around (hope springs
> eternal in the human breast ...)!
It would be useful if you'd send one driver at a time to the list as the
full source to review.
Did we make any progress on the naming discussion? In my opinion hv is
a far to generic name for your drivers. Why not call it mshv dor the
driver directory and prefixes?
As far as the core code is concerned, can you explain the use of the
dev_add, dev_rm and cleanup methods and how they relate to the
normal probe/remove/shutdown methods?
As far as the storage drivers are concerned I still have issues with the
architecture. I haven't seen any good explanation why you want to have
the blkvsc and storvsc drivers different from each other. They both
speak the same vmbus-level protocol and tunnel scsi commands over it.
Why would you sometimes expose this SCSI protocol as a SCSI LLDD and
sometimes as a block driver? What decides that a device is exported
in a way to that blkvsc is bound to them vs storvsc? How do they look
like on the windows side? From my understanding of the windows driver
models both the recent storport model and the older scsiport model are
more or less talking scsi to the driver anyway, so what is the
difference between the two for a Windows guest?
Also pleae get rid of struct storvsc_driver_object, it's just a very
strange way to store file-scope variables, and useless indirection
for the I/O submission handler.
^ permalink raw reply
* RE: [PATCH 00/25] Staging: hv: Cleanup vmbus driver code
From: KY Srinivasan @ 2011-04-27 11:47 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Greg KH, gregkh@suse.de, linux-kernel@vger.kernel.org,
devel@linuxdriverproject.org, virtualization@lists.osdl.org
In-Reply-To: <20110427064550.GA597@infradead.org>
> -----Original Message-----
> From: Christoph Hellwig [mailto:hch@infradead.org]
> Sent: Wednesday, April 27, 2011 2:46 AM
> To: KY Srinivasan
> Cc: Greg KH; gregkh@suse.de; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; virtualization@lists.osdl.org
> Subject: Re: [PATCH 00/25] Staging: hv: Cleanup vmbus driver code
>
> On Wed, Apr 27, 2011 at 01:54:02AM +0000, KY Srinivasan wrote:
> > I would prefer that we go through the review process. What is the process for
> > this review? Is there a time window for people to respond. I am hoping I will be
> able
> > to address all the review comments well in advance of the next closing of the
> tree,
> > with the hope of taking the vmbus driver out of staging this go around (hope
> springs
> > eternal in the human breast ...)!
>
> It would be useful if you'd send one driver at a time to the list as the
> full source to review.
>
> Did we make any progress on the naming discussion? In my opinion hv is
> a far to generic name for your drivers. Why not call it mshv dor the
> driver directory and prefixes?
This topic was discussed at some great length back in Feb/March when I
did a bunch of cleanup with regards how the driver and device data structures
were layered. At that point, the consensus was to keep the hv prefix.
>
> As far as the core code is concerned, can you explain the use of the
> dev_add, dev_rm and cleanup methods and how they relate to the
> normal probe/remove/shutdown methods?
While I am currently cleaning up our block drivers, my goal this go around is to
work on getting the vmbus driver out of staging. I am hoping when I am ready for
having you guys review the storage drivers, I will have dealt with the issues you
raise here.
>
> As far as the storage drivers are concerned I still have issues with the
> architecture. I haven't seen any good explanation why you want to have
> the blkvsc and storvsc drivers different from each other. They both
> speak the same vmbus-level protocol and tunnel scsi commands over it.
> Why would you sometimes expose this SCSI protocol as a SCSI LLDD and
> sometimes as a block driver? What decides that a device is exported
> in a way to that blkvsc is bound to them vs storvsc? How do they look
> like on the windows side? From my understanding of the windows driver
> models both the recent storport model and the older scsiport model are
> more or less talking scsi to the driver anyway, so what is the
> difference between the two for a Windows guest?
I had written up a brief note that I had sent out setting the stage for the
first patch-set for cleaning up the block drivers. I am copying it here for your
convenience:
From: K. Y. Srinivasan <kys@microsoft.com>
Date: Tue, 22 Mar 2011 11:54:46 -0700
Subject: [PATCH 00/16] Staging: hv: Cleanup storage drivers - Phase I
This is first in a series of patch-sets aimed at cleaning up the storage
drivers for Hyper-V. Before I get into the details of this patch-set, I think
it is useful to give a brief overview of the storage related front-end
drivers currently in the tree for Linux on Hyper-V:
On the host side, Windows emulates the standard PC hardware
to permit hosting of fully virtualized operating systems.
To enhance disk I/O performance, we support a virtual block driver.
This block driver currently handles disks that have been setup as IDE
disks for the guest - as specified in the guest configuration.
On the SCSI side, we emulate a SCSI HBA. Devices configured
under the SCSI controller for the guest are handled via this
emulated HBA (SCSI front-end). So, SCSI disks configured for
the guest are handled through native SCSI upper-level drivers.
If this SCSI front-end driver is not loaded, currently, the guest
cannot see devices that have been configured as SCSI devices.
So, while the virtual block driver described earlier could potentially
handle all block devices, the implementation choices made on the host
will not permit it. Also, the only SCSI device that can be currently configured
for the guest is a disk device.
Both the block device driver (hv_blkvsc) and the SCSI front-end
driver (hv_storvsc) communicate with the host via unique channels
that are implemented as bi-directional ring buffers. Each (storage)
channel carries with it enough state to uniquely identify the device on
the host side. Microsoft has chosen to use SCSI verbs for this storage channel
communication.
>
> Also pleae get rid of struct storvsc_driver_object, it's just a very
> strange way to store file-scope variables, and useless indirection
> for the I/O submission handler.
>
I will do this as part of storage cleanup I am currently doing. Thank you
for taking the time to review the code.
Regards,
K. Y
^ permalink raw reply
* Re: [PATCH 00/25] Staging: hv: Cleanup vmbus driver code
From: Christoph Hellwig @ 2011-04-27 12:18 UTC (permalink / raw)
To: KY Srinivasan
Cc: Christoph Hellwig, Greg KH, gregkh@suse.de,
linux-kernel@vger.kernel.org, devel@linuxdriverproject.org,
virtualization@lists.osdl.org
In-Reply-To: <6E21E5352C11B742B20C142EB499E0481DD266@TK5EX14MBXC124.redmond.corp.microsoft.com>
On Wed, Apr 27, 2011 at 11:47:03AM +0000, KY Srinivasan wrote:
> On the host side, Windows emulates the standard PC hardware
> to permit hosting of fully virtualized operating systems.
> To enhance disk I/O performance, we support a virtual block driver.
> This block driver currently handles disks that have been setup as IDE
> disks for the guest - as specified in the guest configuration.
>
> On the SCSI side, we emulate a SCSI HBA. Devices configured
> under the SCSI controller for the guest are handled via this
> emulated HBA (SCSI front-end). So, SCSI disks configured for
> the guest are handled through native SCSI upper-level drivers.
> If this SCSI front-end driver is not loaded, currently, the guest
> cannot see devices that have been configured as SCSI devices.
> So, while the virtual block driver described earlier could potentially
> handle all block devices, the implementation choices made on the host
> will not permit it. Also, the only SCSI device that can be currently configured
> for the guest is a disk device.
>
> Both the block device driver (hv_blkvsc) and the SCSI front-end
> driver (hv_storvsc) communicate with the host via unique channels
> that are implemented as bi-directional ring buffers. Each (storage)
> channel carries with it enough state to uniquely identify the device on
> the host side. Microsoft has chosen to use SCSI verbs for this storage channel
> communication.
This doesn't really explain much at all. The only important piece
of information I can read from this statement is that both blkvsc
and storvsc only support disks, but not any other kind of device,
and that chosing either one is an arbitrary seletin when setting up
a VM configuration.
But this still isn't an excuse to implement a block layer driver for
a SCSI protocol, and it doesn't not explain in what way the two
protocols actually differ. You really should implement blksvs as a SCSI
LLDD, too - and from the looks of it it doesn't even have to be a
separate one, but just adding the ids to storvsc would do the work.
^ permalink raw reply
* Re: [PATCH 08/25] Staging: hv: vmbus_driver cannot be unloaded; cleanup accordingly
From: Greg KH @ 2011-04-28 0:20 UTC (permalink / raw)
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)
In-Reply-To: <6E21E5352C11B742B20C142EB499E0481DD1C9@TK5EX14MBXC124.redmond.corp.microsoft.com>
On Wed, Apr 27, 2011 at 02:31:18AM +0000, KY Srinivasan wrote:
>
>
> > -----Original Message-----
> > From: Greg KH [mailto:greg@kroah.com]
> > Sent: Tuesday, April 26, 2011 6:46 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 08/25] Staging: hv: vmbus_driver cannot be unloaded;
> > cleanup accordingly
> >
> > On Tue, Apr 26, 2011 at 09:20:25AM -0700, K. Y. Srinivasan wrote:
> > > The vmbus driver cannot be unloaded; the windows host does not
> > > permit this. Cleanup accordingly.
> >
> > Woah, you just prevented this driver from ever being able to be
> > unloaded.
>
> It was never unloadable; while the driver defined an exit routine,
> there were couple of issues unloading the vmbus driver:
>
> 1) All guest resources given to the host could not be recovered.
Is this a problem in the Linux side? If so, that could easily be fixed.
> 2) Windows host would not permit reloading the driver without
> rebooting the guest.
That's a different issue, and one that I am very surprised to hear.
That kind of invalidates ever being able to update the driver in a guest
for a long-running system that you want to migrate and not reboot. That
sounds like a major bug in hyper-v, don't you agree?
> All I did was acknowledge the current state and cleanup
> accordingly. This is not unique to Hyper-V; for what it is worth,
> the Xen platform_pci driver which is equivalent to the vmbus driver
> is also not unlodable (the last time I checked).
Why isn't that allowed to be unloaded? What happens if it does?
I would like to see the following be possible from Linux:
- running Linux guest on hyperv
- need to migrate to a newer version of hyper-v
- pause long-running userspace processes.
- unload hyperv modules
- migrate guest to newer hyperv version (possible different host
machine)
- load newer hyperv modules
- resume long-running guest processes
If this isn't possible due to hyper-v bugs, then I guess we need to be
able to live with it, but we had better advertise it pretty well as I
know people will want to be able to do the above sequence for their
guest instances.
If so, can you expand this patch to say more in the changelog entry, and
resend the remaining patches that I didn't apply as they are now gone
from my pending-patch queue.
> > That's not a "cleanup" that's a major change in how things work. I'm
> > sure, if you want to continue down this line, there are more things you
> > can remove from the code, right?
> >
> > What is the real issue here? What happens if you unload the bus? What
> > goes wrong? Can it be fixed?
>
> This needs to be fixed on the host side. I have notified them of the issue.
Ok, so if this is going to be fixed, why do we need to prevent this from
ever being possible to have happen on our side?
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH 12/25] Staging: hv: Cleanup error handling in vmbus_child_device_register()
From: Greg KH @ 2011-04-28 0:25 UTC (permalink / raw)
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)
In-Reply-To: <6E21E5352C11B742B20C142EB499E0481DD1AF@TK5EX14MBXC124.redmond.corp.microsoft.com>
On Wed, Apr 27, 2011 at 02:11:48AM +0000, KY Srinivasan wrote:
>
>
> > -----Original Message-----
> > From: Greg KH [mailto:greg@kroah.com]
> > Sent: Tuesday, April 26, 2011 6:51 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/25] Staging: hv: Cleanup error handling in
> > vmbus_child_device_register()
> >
> > On Tue, Apr 26, 2011 at 09:20:29AM -0700, K. Y. Srinivasan wrote:
> > > Cleanup error handling in vmbus_child_device_register().
> > >
> > > Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> > > Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
> > > Signed-off-by: Abhishek Kane <v-abkane@microsoft.com>
> > > Signed-off-by: Hank Janssen <hjanssen@microsoft.com>
> > > ---
> > > drivers/staging/hv/vmbus_drv.c | 7 ++++++-
> > > 1 files changed, 6 insertions(+), 1 deletions(-)
> > >
> > > diff --git a/drivers/staging/hv/vmbus_drv.c b/drivers/staging/hv/vmbus_drv.c
> > > index d597dd4..4d569ad 100644
> > > --- a/drivers/staging/hv/vmbus_drv.c
> > > +++ b/drivers/staging/hv/vmbus_drv.c
> > > @@ -720,11 +720,16 @@ int vmbus_child_device_register(struct hv_device
> > *child_device_obj)
> > > */
> > > ret = device_register(&child_device_obj->device);
> > >
> > > + if (ret)
> > > + return ret;
> > > +
> > > /* vmbus_probe() error does not get propergate to device_register(). */
> > > ret = child_device_obj->probe_error;
> >
> > Wait, why not? Why is the probe_error have to be saved off like this?
> > That seems like something is wrong here, this patch should not be
> > needed.
> >
> > Well, you should check the return value of device_register, that is
> > needed, but this seems broken somehow.
>
> The current code had comments claiming that the probe error was not
> correctly propagated. Looking at the kernel side of the code, it was not clear
> if device_register() could succeed while the probe might fail.
Of course it can, device_register() has nothing to do with the probe
callback of the device itself. To think otherwise is to not understand
the driver model and assume things that you should never be caring
about.
Think about it, if you register a device, you don't know at that point
in time if a driver is currently loaded for it, and that it will be
bound to that device. Nor do you care, as any needed notifications for
new drivers will be sent to userspace, and they will be loaded at some
random time in the future. So a probe() call might never be called for
this device until some other time, running on some other processor, in
some other thread.
Drivers are allowed to return errors from their probe functions for
valid reasons (i.e. this driver shouldn't bind to this device for a
variety of good reasons.) No one cares about this, as the driver core
handles it properly and will pass on to the next driver in the list that
might be able to be bound to this device.
So why do you care about the return value of the probe() call? It gets
properly handled already by the driver core, why would your bus ever
care about it? (Hint, no other bus does, as it makes no sense.)
> In any event, if you can guarantee that device_register() can return
> any probe related errors, I agree with you that saving the probe error
> is an overkill. The current code saved the probe error and with new
> check I added with regards to the return value of device_register,
> there is no correctness issue with this patch.
As explained above, no, it will not return a probe error, as that makes
no sense. If the code is wanting to rely on this, it is broken and must
be fixed.
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH 00/25] Staging: hv: Cleanup vmbus driver code
From: Greg KH @ 2011-04-28 0:28 UTC (permalink / raw)
To: KY Srinivasan
Cc: gregkh@suse.de, linux-kernel@vger.kernel.org,
devel@linuxdriverproject.org, virtualization@lists.osdl.org
In-Reply-To: <6E21E5352C11B742B20C142EB499E0481DD185@TK5EX14MBXC124.redmond.corp.microsoft.com>
On Wed, Apr 27, 2011 at 01:54:02AM +0000, KY Srinivasan wrote:
> > After that, do you want another round of review of the code, or do
> > you have more things you want to send in (like the name[64] removal?)
>
> I would prefer that we go through the review process. What is the process for
> this review?
The same as always, just ask.
> Is there a time window for people to respond.
No. We don't have time limits here, this is a community, we don't have
deadlines, you know that.
> I am hoping I will be able to address all the review comments well in
> advance of the next closing of the tree, with the hope of taking the
> vmbus driver out of staging this go around (hope springs eternal in
> the human breast ...)!
Yes, it would be nice, and I understand your the corporate pressures you
are under to get this done, and I am doing my best to fit the patch
review and apply cycle into my very-very-limited-at-the-moment spare
time.
As always, if you miss this kernel release, there's always another one 3
months away, so it's no big deal in the long-run.
thanks,
greg k-h
^ permalink raw reply
* RE: [PATCH 08/25] Staging: hv: vmbus_driver cannot be unloaded; cleanup accordingly
From: KY Srinivasan @ 2011-04-29 13:49 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: <20110428002018.GC14294@kroah.com>
> -----Original Message-----
> From: Greg KH [mailto:greg@kroah.com]
> Sent: Wednesday, April 27, 2011 8:20 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 08/25] Staging: hv: vmbus_driver cannot be unloaded;
> cleanup accordingly
>
> On Wed, Apr 27, 2011 at 02:31:18AM +0000, KY Srinivasan wrote:
> >
> >
> > > -----Original Message-----
> > > From: Greg KH [mailto:greg@kroah.com]
> > > Sent: Tuesday, April 26, 2011 6:46 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 08/25] Staging: hv: vmbus_driver cannot be unloaded;
> > > cleanup accordingly
> > >
> > > On Tue, Apr 26, 2011 at 09:20:25AM -0700, K. Y. Srinivasan wrote:
> > > > The vmbus driver cannot be unloaded; the windows host does not
> > > > permit this. Cleanup accordingly.
> > >
> > > Woah, you just prevented this driver from ever being able to be
> > > unloaded.
> >
> > It was never unloadable; while the driver defined an exit routine,
> > there were couple of issues unloading the vmbus driver:
> >
> > 1) All guest resources given to the host could not be recovered.
>
> Is this a problem in the Linux side? If so, that could easily be fixed.
This is not an issue on the Linux side. From what I remember, this was
Guest/Host protocol related. Once I confirmed the second issue, I did
not pursue this issue further.
>
> > 2) Windows host would not permit reloading the driver without
> > rebooting the guest.
>
> That's a different issue, and one that I am very surprised to hear.
> That kind of invalidates ever being able to update the driver in a guest
> for a long-running system that you want to migrate and not reboot. That
> sounds like a major bug in hyper-v, don't you agree?
In practical terms, I am not sure this is a major problem. If the root device
Is managed by a Hyper-V driver, then you cannot unload that driver and
drivers it depends on anyway.
>
> > All I did was acknowledge the current state and cleanup
> > accordingly. This is not unique to Hyper-V; for what it is worth,
> > the Xen platform_pci driver which is equivalent to the vmbus driver
> > is also not unlodable (the last time I checked).
>
> Why isn't that allowed to be unloaded? What happens if it does?
On the Xen side, from as long as I can remember, the platform_pci
has been unlodable (does not define an exit routiune). I don't recall
what the issues have been.
>
> I would like to see the following be possible from Linux:
> - running Linux guest on hyperv
> - need to migrate to a newer version of hyper-v
> - pause long-running userspace processes.
> - unload hyperv modules
> - migrate guest to newer hyperv version (possible different host
> machine)
> - load newer hyperv modules
> - resume long-running guest processes
Many hyper-V modules are un-lodable; it is just the vmbus_driver module
has this issue. Also, since vmbus_driver happens to be the foundational
driver for most of the hyper-v drivers, and since ultimately, we do
want to handle the root device via hyper-v block driver, vmbus_driver will
become unlodable for other reasons.
>
> If this isn't possible due to hyper-v bugs, then I guess we need to be
> able to live with it, but we had better advertise it pretty well as I
> know people will want to be able to do the above sequence for their
> guest instances.
>
> If so, can you expand this patch to say more in the changelog entry, and
> resend the remaining patches that I didn't apply as they are now gone
> from my pending-patch queue.
Will do.
>
> > > That's not a "cleanup" that's a major change in how things work. I'm
> > > sure, if you want to continue down this line, there are more things you
> > > can remove from the code, right?
> > >
> > > What is the real issue here? What happens if you unload the bus? What
> > > goes wrong? Can it be fixed?
> >
> > This needs to be fixed on the host side. I have notified them of the issue.
>
> Ok, so if this is going to be fixed, why do we need to prevent this from
> ever being possible to have happen on our side?
While I have notified the windows team about this issue, I do not know
If/or when this issue may be fixed. As, I noted earlier, having an exit function
gives the impression that the module is unloadable while it is not - today because
of the issue on the host side. Even if the host side issue is fixed, we may not be
able to unload the vmbus driver for other dependency related issues.
All I did was get rid of the exit function so there
would be no confusion as to the state of this module. We could easily add the exit
function, if and when the host side issues are fixed. Even then, this module may
be unlodable if we are driving the root device with the hyper-v block driver.
Greg, I am open to either approach here: 1) I could drop this patch and restore the
exit function. 2) I could keep the patch as is, but add additional comments to
capture this discussion. Let me know what you prefer and I will send the remaining
patch-set with the agreed upon changes.
Regards,
K. Y
^ permalink raw reply
* RE: [PATCH 00/25] Staging: hv: Cleanup vmbus driver code
From: KY Srinivasan @ 2011-04-29 14:26 UTC (permalink / raw)
To: Greg KH
Cc: devel@linuxdriverproject.org, gregkh@suse.de,
linux-kernel@vger.kernel.org, virtualization@lists.osdl.org
In-Reply-To: <20110428002817.GE14294@kroah.com>
> -----Original Message-----
> From: Greg KH [mailto:greg@kroah.com]
> Sent: Wednesday, April 27, 2011 8:28 PM
> To: KY Srinivasan
> Cc: gregkh@suse.de; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; virtualization@lists.osdl.org
> Subject: Re: [PATCH 00/25] Staging: hv: Cleanup vmbus driver code
>
> On Wed, Apr 27, 2011 at 01:54:02AM +0000, KY Srinivasan wrote:
> > > After that, do you want another round of review of the code, or do
> > > you have more things you want to send in (like the name[64] removal?)
> >
> > I would prefer that we go through the review process. What is the process for
> > this review?
>
> The same as always, just ask.
>
> > Is there a time window for people to respond.
>
> No. We don't have time limits here, this is a community, we don't have
> deadlines, you know that.
Perhaps I did not properly formulate my question here. The review
process itself may be open-ended, and that is fine - we will fix all legitimate
issues/concerns in our drivers whether they are in the staging area or not.
My question was specifically with regards to the review process that may gate exiting
staging. I am hoping to re-spin the remaining patches of the last patch-set and send it to
you by early next week and ask for a review. I fully intend to address whatever review
comments I may get in a very timely manner. Assuming at some point in time after I ask
for this review there are no outstanding issues, would that be sufficient to exit staging?
>
> > I am hoping I will be able to address all the review comments well in
> > advance of the next closing of the tree, with the hope of taking the
> > vmbus driver out of staging this go around (hope springs eternal in
> > the human breast ...)!
>
> Yes, it would be nice, and I understand your the corporate pressures you
> are under to get this done, and I am doing my best to fit the patch
> review and apply cycle into my very-very-limited-at-the-moment spare
> time.
Greg, I do appreciate your ongoing help here.
Regards,
K. Y
^ permalink raw reply
* Re: [PATCH 00/25] Staging: hv: Cleanup vmbus driver code
From: Greg KH @ 2011-04-29 15:08 UTC (permalink / raw)
To: KY Srinivasan
Cc: Greg KH, linux-kernel@vger.kernel.org,
devel@linuxdriverproject.org, virtualization@lists.osdl.org
In-Reply-To: <6E21E5352C11B742B20C142EB499E0481DD5AA@TK5EX14MBXC124.redmond.corp.microsoft.com>
On Fri, Apr 29, 2011 at 02:26:13PM +0000, KY Srinivasan wrote:
> Perhaps I did not properly formulate my question here. The review
> process itself may be open-ended, and that is fine - we will fix all legitimate
> issues/concerns in our drivers whether they are in the staging area or not.
> My question was specifically with regards to the review process that may gate exiting
> staging. I am hoping to re-spin the remaining patches of the last patch-set and send it to
> you by early next week and ask for a review. I fully intend to address whatever review
> comments I may get in a very timely manner. Assuming at some point in time after I ask
> for this review there are no outstanding issues, would that be sufficient to exit staging?
If it looks acceptable to me, and there are no other objections from
other developers, then yes, that would be sufficient to move it out of
staging.
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH 08/25] Staging: hv: vmbus_driver cannot be unloaded; cleanup accordingly
From: Greg KH @ 2011-04-29 15:10 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: <6E21E5352C11B742B20C142EB499E0481DD55F@TK5EX14MBXC124.redmond.corp.microsoft.com>
On Fri, Apr 29, 2011 at 01:49:21PM +0000, KY Srinivasan wrote:
> > > 2) Windows host would not permit reloading the driver without
> > > rebooting the guest.
> >
> > That's a different issue, and one that I am very surprised to hear.
> > That kind of invalidates ever being able to update the driver in a guest
> > for a long-running system that you want to migrate and not reboot. That
> > sounds like a major bug in hyper-v, don't you agree?
>
> In practical terms, I am not sure this is a major problem. If the root device
> Is managed by a Hyper-V driver, then you cannot unload that driver and
> drivers it depends on anyway.
I don't run my hyper-v guests using the hyper-v driver for my root
devices, so in my setup, it is possible to unload the whole vmbus
subsystem and drivers and reload it without any system interruption.
Now I have never tried that... :)
> Greg, I am open to either approach here: 1) I could drop this patch and restore the
> exit function. 2) I could keep the patch as is, but add additional comments to
> capture this discussion. Let me know what you prefer and I will send the remaining
> patch-set with the agreed upon changes.
As you all are happy with not having this be unloaded, I'll go with 2)
as it is your code to maintain and support, not mine.
Just resend it with some more comments as I explained, and the rest, and
I'll queue them up when I get caught up on my patch queue (I only have
381 to get through, the end is near!)
thanks,
greg k-h
^ permalink raw reply
* RE: [PATCH 12/25] Staging: hv: Cleanup error handling in vmbus_child_device_register()
From: KY Srinivasan @ 2011-04-29 15:45 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: <20110428002552.GD14294@kroah.com>
> -----Original Message-----
> From: Greg KH [mailto:greg@kroah.com]
> Sent: Wednesday, April 27, 2011 8:26 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/25] Staging: hv: Cleanup error handling in
> vmbus_child_device_register()
>
> On Wed, Apr 27, 2011 at 02:11:48AM +0000, KY Srinivasan wrote:
> >
> >
> > > -----Original Message-----
> > > From: Greg KH [mailto:greg@kroah.com]
> > > Sent: Tuesday, April 26, 2011 6:51 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/25] Staging: hv: Cleanup error handling in
> > > vmbus_child_device_register()
> > >
> > > On Tue, Apr 26, 2011 at 09:20:29AM -0700, K. Y. Srinivasan wrote:
> > > > Cleanup error handling in vmbus_child_device_register().
> > > >
> > > > Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
> > > > Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
> > > > Signed-off-by: Abhishek Kane <v-abkane@microsoft.com>
> > > > Signed-off-by: Hank Janssen <hjanssen@microsoft.com>
> > > > ---
> > > > drivers/staging/hv/vmbus_drv.c | 7 ++++++-
> > > > 1 files changed, 6 insertions(+), 1 deletions(-)
> > > >
> > > > diff --git a/drivers/staging/hv/vmbus_drv.c
> b/drivers/staging/hv/vmbus_drv.c
> > > > index d597dd4..4d569ad 100644
> > > > --- a/drivers/staging/hv/vmbus_drv.c
> > > > +++ b/drivers/staging/hv/vmbus_drv.c
> > > > @@ -720,11 +720,16 @@ int vmbus_child_device_register(struct hv_device
> > > *child_device_obj)
> > > > */
> > > > ret = device_register(&child_device_obj->device);
> > > >
> > > > + if (ret)
> > > > + return ret;
> > > > +
> > > > /* vmbus_probe() error does not get propergate to device_register(). */
> > > > ret = child_device_obj->probe_error;
> > >
> > > Wait, why not? Why is the probe_error have to be saved off like this?
> > > That seems like something is wrong here, this patch should not be
> > > needed.
> > >
> > > Well, you should check the return value of device_register, that is
> > > needed, but this seems broken somehow.
> >
> > The current code had comments claiming that the probe error was not
> > correctly propagated. Looking at the kernel side of the code, it was not clear
> > if device_register() could succeed while the probe might fail.
>
> Of course it can, device_register() has nothing to do with the probe
> callback of the device itself. To think otherwise is to not understand
> the driver model and assume things that you should never be caring
> about.
>
> Think about it, if you register a device, you don't know at that point
> in time if a driver is currently loaded for it, and that it will be
> bound to that device. Nor do you care, as any needed notifications for
> new drivers will be sent to userspace, and they will be loaded at some
> random time in the future. So a probe() call might never be called for
> this device until some other time, running on some other processor, in
> some other thread.
>
> Drivers are allowed to return errors from their probe functions for
> valid reasons (i.e. this driver shouldn't bind to this device for a
> variety of good reasons.) No one cares about this, as the driver core
> handles it properly and will pass on to the next driver in the list that
> might be able to be bound to this device.
>
> So why do you care about the return value of the probe() call? It gets
> properly handled already by the driver core, why would your bus ever
> care about it? (Hint, no other bus does, as it makes no sense.)
>
> > In any event, if you can guarantee that device_register() can return
> > any probe related errors, I agree with you that saving the probe error
> > is an overkill. The current code saved the probe error and with new
> > check I added with regards to the return value of device_register,
> > there is no correctness issue with this patch.
>
> As explained above, no, it will not return a probe error, as that makes
> no sense. If the code is wanting to rely on this, it is broken and must
> be fixed.
I did not say that device_register would return the probe_error. On the
contrary, the code I added explicitly ensures that proper cleanup is done
for the failure of device_register and if the probe function were
called on the same context executing the device_register call,
the failure of the probe call as well (looking at the stack trace while in
the probe function, this appeared to be the case currently).
If you look at the existing code; the current code did not deal with the failure of
device_register() - it over-writes the return value of device_register with
that of the probe error. This patch fixed that problem.
The current code dealt with the probe error by spinning up a work item to
deal with the probe failure. This work item would try to unregister the device
that was not fully registered and this caused a problem. I tried to fix this
problem by getting rid of the work item and dealing with the probe
failure cleanup in the same context as the call to device_register.
Since no other driver deals with probe failures this way, I will get rid
of this code in the next version of this patch.
Regards,
K. Y
>
> thanks,
>
> greg k-h
^ permalink raw reply
* RE: [PATCH 00/25] Staging: hv: Cleanup vmbus driver code
From: KY Srinivasan @ 2011-04-29 16:32 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Greg KH, gregkh@suse.de, linux-kernel@vger.kernel.org,
devel@linuxdriverproject.org, virtualization@lists.osdl.org
In-Reply-To: <20110427121856.GA20633@infradead.org>
> -----Original Message-----
> From: Christoph Hellwig [mailto:hch@infradead.org]
> Sent: Wednesday, April 27, 2011 8:19 AM
> To: KY Srinivasan
> Cc: Christoph Hellwig; Greg KH; gregkh@suse.de; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; virtualization@lists.osdl.org
> Subject: Re: [PATCH 00/25] Staging: hv: Cleanup vmbus driver code
>
> On Wed, Apr 27, 2011 at 11:47:03AM +0000, KY Srinivasan wrote:
> > On the host side, Windows emulates the standard PC hardware
> > to permit hosting of fully virtualized operating systems.
> > To enhance disk I/O performance, we support a virtual block driver.
> > This block driver currently handles disks that have been setup as IDE
> > disks for the guest - as specified in the guest configuration.
> >
> > On the SCSI side, we emulate a SCSI HBA. Devices configured
> > under the SCSI controller for the guest are handled via this
> > emulated HBA (SCSI front-end). So, SCSI disks configured for
> > the guest are handled through native SCSI upper-level drivers.
> > If this SCSI front-end driver is not loaded, currently, the guest
> > cannot see devices that have been configured as SCSI devices.
> > So, while the virtual block driver described earlier could potentially
> > handle all block devices, the implementation choices made on the host
> > will not permit it. Also, the only SCSI device that can be currently configured
> > for the guest is a disk device.
> >
> > Both the block device driver (hv_blkvsc) and the SCSI front-end
> > driver (hv_storvsc) communicate with the host via unique channels
> > that are implemented as bi-directional ring buffers. Each (storage)
> > channel carries with it enough state to uniquely identify the device on
> > the host side. Microsoft has chosen to use SCSI verbs for this storage channel
> > communication.
>
> This doesn't really explain much at all. The only important piece
> of information I can read from this statement is that both blkvsc
> and storvsc only support disks, but not any other kind of device,
> and that chosing either one is an arbitrary seletin when setting up
> a VM configuration.
>
> But this still isn't an excuse to implement a block layer driver for
> a SCSI protocol, and it doesn't not explain in what way the two
> protocols actually differ. You really should implement blksvs as a SCSI
> LLDD, too - and from the looks of it it doesn't even have to be a
> separate one, but just adding the ids to storvsc would do the work.
On the host-side, as part of configuring a guest you can specify block devices
as being under an IDE controller or under a
SCSI controller. Those are the only options you have. Devices configured under
the IDE controller cannot be seen in the guest under the emulated SCSI front-end which is
the scsi driver (storvsc_drv). So, when you do a bus scan in the emulated scsi front-end,
the devices enumerated will not include block devices configured under the IDE
controller. So, it is not clear to me how I can do what you are proposing given the
restrictions imposed by the host.
Regards,
K. Y
^ permalink raw reply
* Re: [PATCH 00/25] Staging: hv: Cleanup vmbus driver code
From: Greg KH @ 2011-04-29 16:40 UTC (permalink / raw)
To: KY Srinivasan
Cc: Christoph Hellwig, gregkh@suse.de, linux-kernel@vger.kernel.org,
devel@linuxdriverproject.org, virtualization@lists.osdl.org
In-Reply-To: <6E21E5352C11B742B20C142EB499E0481DD676@TK5EX14MBXC124.redmond.corp.microsoft.com>
On Fri, Apr 29, 2011 at 04:32:35PM +0000, KY Srinivasan wrote:
>
>
> > -----Original Message-----
> > From: Christoph Hellwig [mailto:hch@infradead.org]
> > Sent: Wednesday, April 27, 2011 8:19 AM
> > To: KY Srinivasan
> > Cc: Christoph Hellwig; Greg KH; gregkh@suse.de; linux-kernel@vger.kernel.org;
> > devel@linuxdriverproject.org; virtualization@lists.osdl.org
> > Subject: Re: [PATCH 00/25] Staging: hv: Cleanup vmbus driver code
> >
> > On Wed, Apr 27, 2011 at 11:47:03AM +0000, KY Srinivasan wrote:
> > > On the host side, Windows emulates the standard PC hardware
> > > to permit hosting of fully virtualized operating systems.
> > > To enhance disk I/O performance, we support a virtual block driver.
> > > This block driver currently handles disks that have been setup as IDE
> > > disks for the guest - as specified in the guest configuration.
> > >
> > > On the SCSI side, we emulate a SCSI HBA. Devices configured
> > > under the SCSI controller for the guest are handled via this
> > > emulated HBA (SCSI front-end). So, SCSI disks configured for
> > > the guest are handled through native SCSI upper-level drivers.
> > > If this SCSI front-end driver is not loaded, currently, the guest
> > > cannot see devices that have been configured as SCSI devices.
> > > So, while the virtual block driver described earlier could potentially
> > > handle all block devices, the implementation choices made on the host
> > > will not permit it. Also, the only SCSI device that can be currently configured
> > > for the guest is a disk device.
> > >
> > > Both the block device driver (hv_blkvsc) and the SCSI front-end
> > > driver (hv_storvsc) communicate with the host via unique channels
> > > that are implemented as bi-directional ring buffers. Each (storage)
> > > channel carries with it enough state to uniquely identify the device on
> > > the host side. Microsoft has chosen to use SCSI verbs for this storage channel
> > > communication.
> >
> > This doesn't really explain much at all. The only important piece
> > of information I can read from this statement is that both blkvsc
> > and storvsc only support disks, but not any other kind of device,
> > and that chosing either one is an arbitrary seletin when setting up
> > a VM configuration.
> >
> > But this still isn't an excuse to implement a block layer driver for
> > a SCSI protocol, and it doesn't not explain in what way the two
> > protocols actually differ. You really should implement blksvs as a SCSI
> > LLDD, too - and from the looks of it it doesn't even have to be a
> > separate one, but just adding the ids to storvsc would do the work.
>
> On the host-side, as part of configuring a guest you can specify block devices
> as being under an IDE controller or under a
> SCSI controller. Those are the only options you have. Devices configured under
> the IDE controller cannot be seen in the guest under the emulated SCSI front-end which is
> the scsi driver (storvsc_drv).
Are you sure the libata core can't see this ide controller and connect
to it? That way you would use the scsi system if you do that and you
would need a much smaller ide driver, perhaps being able to merge it
with your scsi driver.
We really don't want to write new IDE drivers anymore that don't use
libata.
thanks,
greg k-h
^ permalink raw reply
* RE: [PATCH 00/25] Staging: hv: Cleanup vmbus driver code
From: KY Srinivasan @ 2011-04-29 17:32 UTC (permalink / raw)
To: Greg KH
Cc: Christoph Hellwig, gregkh@suse.de, linux-kernel@vger.kernel.org,
devel@linuxdriverproject.org, virtualization@lists.osdl.org
In-Reply-To: <20110429164025.GA7698@kroah.com>
> -----Original Message-----
> From: Greg KH [mailto:greg@kroah.com]
> Sent: Friday, April 29, 2011 12:40 PM
> To: KY Srinivasan
> Cc: Christoph Hellwig; gregkh@suse.de; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; virtualization@lists.osdl.org
> Subject: Re: [PATCH 00/25] Staging: hv: Cleanup vmbus driver code
>
> On Fri, Apr 29, 2011 at 04:32:35PM +0000, KY Srinivasan wrote:
> >
> >
> > > -----Original Message-----
> > > From: Christoph Hellwig [mailto:hch@infradead.org]
> > > Sent: Wednesday, April 27, 2011 8:19 AM
> > > To: KY Srinivasan
> > > Cc: Christoph Hellwig; Greg KH; gregkh@suse.de; linux-
> kernel@vger.kernel.org;
> > > devel@linuxdriverproject.org; virtualization@lists.osdl.org
> > > Subject: Re: [PATCH 00/25] Staging: hv: Cleanup vmbus driver code
> > >
> > > On Wed, Apr 27, 2011 at 11:47:03AM +0000, KY Srinivasan wrote:
> > > > On the host side, Windows emulates the standard PC hardware
> > > > to permit hosting of fully virtualized operating systems.
> > > > To enhance disk I/O performance, we support a virtual block driver.
> > > > This block driver currently handles disks that have been setup as IDE
> > > > disks for the guest - as specified in the guest configuration.
> > > >
> > > > On the SCSI side, we emulate a SCSI HBA. Devices configured
> > > > under the SCSI controller for the guest are handled via this
> > > > emulated HBA (SCSI front-end). So, SCSI disks configured for
> > > > the guest are handled through native SCSI upper-level drivers.
> > > > If this SCSI front-end driver is not loaded, currently, the guest
> > > > cannot see devices that have been configured as SCSI devices.
> > > > So, while the virtual block driver described earlier could potentially
> > > > handle all block devices, the implementation choices made on the host
> > > > will not permit it. Also, the only SCSI device that can be currently configured
> > > > for the guest is a disk device.
> > > >
> > > > Both the block device driver (hv_blkvsc) and the SCSI front-end
> > > > driver (hv_storvsc) communicate with the host via unique channels
> > > > that are implemented as bi-directional ring buffers. Each (storage)
> > > > channel carries with it enough state to uniquely identify the device on
> > > > the host side. Microsoft has chosen to use SCSI verbs for this storage
> channel
> > > > communication.
> > >
> > > This doesn't really explain much at all. The only important piece
> > > of information I can read from this statement is that both blkvsc
> > > and storvsc only support disks, but not any other kind of device,
> > > and that chosing either one is an arbitrary seletin when setting up
> > > a VM configuration.
> > >
> > > But this still isn't an excuse to implement a block layer driver for
> > > a SCSI protocol, and it doesn't not explain in what way the two
> > > protocols actually differ. You really should implement blksvs as a SCSI
> > > LLDD, too - and from the looks of it it doesn't even have to be a
> > > separate one, but just adding the ids to storvsc would do the work.
> >
> > On the host-side, as part of configuring a guest you can specify block devices
> > as being under an IDE controller or under a
> > SCSI controller. Those are the only options you have. Devices configured under
> > the IDE controller cannot be seen in the guest under the emulated SCSI front-
> end which is
> > the scsi driver (storvsc_drv).
>
> Are you sure the libata core can't see this ide controller and connect
> to it? That way you would use the scsi system if you do that and you
> would need a much smaller ide driver, perhaps being able to merge it
> with your scsi driver.
If we don't load the blkvsc driver, the emulated IDE controller exposed to
the guest can and will be seen by the libata core. In this case though, your
disk I/O will be taking the emulated path with the usual performance hit.
When you load the blkvsc driver, the device access does not go through the emulated
IDE controller. Blkvsc is truly a generic block driver that registers as a block driver in
the guest and talks to an appropriate device driver on the host, communicating over
the vmbus. In this respect, it is identical to block drivers we have for guests in other
virtualization platforms (Xen etc.). The only difference is that on the host side,
the only way you can assign a scsi disk to the guest is to configure this scsi disk
under the scsi controller. So, while blkvsc is a generic block driver, because of the
restrictions on the host side, it only ends up managing block devices that have IDE majors.
>
> We really don't want to write new IDE drivers anymore that don't use
> libata.
As I noted earlier, it is incorrect to view Hyper-V blkvsc driver as an IDE driver. There
is nothing IDE specific about it. It is very much like other block front-end drivers
(like in Xen) that get their device information from the host and register the block
device accordingly with the guest. It just happens that in the current version of the
Windows host, only devices that are configured as IDE devices in the host end up being
managed by this driver. To make this clear, in my recent cleanup of this driver (these patches
have been applied), all IDE major information has been properly consolidated.
Regards,
K. Y
^ permalink raw reply
* RE: [PATCH 08/25] Staging: hv: vmbus_driver cannot be unloaded; cleanup accordingly
From: KY Srinivasan @ 2011-04-29 17:40 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: <20110429151052.GC12142@suse.de>
> -----Original Message-----
> From: Greg KH [mailto:gregkh@suse.de]
> Sent: Friday, April 29, 2011 11:11 AM
> 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 08/25] Staging: hv: vmbus_driver cannot be unloaded;
> cleanup accordingly
>
> On Fri, Apr 29, 2011 at 01:49:21PM +0000, KY Srinivasan wrote:
>
> Just resend it with some more comments as I explained, and the rest, and
> I'll queue them up when I get caught up on my patch queue (I only have
> 381 to get through, the end is near!)
Thanks Greg; soon after I sent you this patch-set, I had also sent a single patch to
address a redundant assignment:
0001-Staging-hv-Do-not-re-set-the-bus-name.patch
Please drop this patch. I will deal with this issue as part of the remaining patch-set
I will be sending now.
Regards,
K. Y
>
> thanks,
>
> greg k-h
^ permalink raw reply
* [RESEND] [PATCH 00/18] Staging: hv: Cleanup vmbus driver code
From: K. Y. Srinivasan @ 2011-04-29 20:42 UTC (permalink / raw)
To: gregkh, linux-kernel, devel, virtualization; +Cc: K. Y. Srinivasan
This is a resend of the patches yet to be applied.
This patch-set addresses some of the bus/driver model cleanup that
Greg sugested over the last couple of days. In this patch-set we
deal with the following issues:
1) Cleanup error handling in the vmbus_probe() and
vmbus_child_device_register() functions. Fixed a
bug in the probe failure path as part of this cleanup.
2) The Windows host cannot handle the vmbus_driver being
unloaded and subsequently loaded. Cleanup the driver with
this in mind.
3) Get rid of struct hv_bus that embedded struct bus_type to
conform with the LDM.
4) Add probe/remove/shutdown functions to struct hv_driver to
conform to LDM.
5) On some older Hyper-V hosts, the Linux PCI sub-sytem is not able
to allocate irq resources to the vmbus driver. I recently learnt
that the vmbus driver is an acpi enumerated device on the Hyper-V
platform. Added code to retrieve irq information from DSDT.
Regards,
K. Y
^ permalink raw reply
* [PATCH 01/18] Staging: hv: vmbus_driver cannot be unloaded; cleanup accordingly
From: K. Y. Srinivasan @ 2011-04-29 20:44 UTC (permalink / raw)
To: gregkh, linux-kernel, devel, virtualization; +Cc: Haiyang Zhang, Abhishek Kane
In-Reply-To: <1304109779-24819-1-git-send-email-kys@microsoft.com>
The vmbus driver cannot be unloaded; the windows host does not
permit this: A) All guest resources given to the host cannot
be recovered and B) Windows host does not permit reloading the
vmbus_driver without re-booting the guest. Both these issues
are host related. Acknowledge this reality and cleanup the
vmbus driver accordingly. Note that, ideally we will want to handle
the root device through the Hyper-V block driver. In this case
unloading the vmbus driver will not be possible because of the
dependency issues.
Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
Signed-off-by: Abhishek Kane <v-abkane@microsoft.com>
Signed-off-by: Hank Janssen <hjanssen@microsoft.com>
---
drivers/staging/hv/vmbus_drv.c | 32 --------------------------------
1 files changed, 0 insertions(+), 32 deletions(-)
diff --git a/drivers/staging/hv/vmbus_drv.c b/drivers/staging/hv/vmbus_drv.c
index bf124a7..d597dd4 100644
--- a/drivers/staging/hv/vmbus_drv.c
+++ b/drivers/staging/hv/vmbus_drv.c
@@ -622,30 +622,6 @@ cleanup:
return ret;
}
-/*
- * vmbus_bus_exit - Terminate the vmbus driver.
- *
- * This routine is opposite of vmbus_bus_init()
- */
-static void vmbus_bus_exit(void)
-{
-
-
- vmbus_release_unattached_channels();
- vmbus_disconnect();
- on_each_cpu(hv_synic_cleanup, NULL, 1);
-
- hv_cleanup();
-
- bus_unregister(&hv_bus.bus);
-
- free_irq(hv_pci_dev->irq, hv_pci_dev);
-
- tasklet_kill(&hv_bus.msg_dpc);
- tasklet_kill(&hv_bus.event_dpc);
-}
-
-
/**
* vmbus_child_driver_register() - Register a vmbus's child driver
* @drv: Pointer to driver structure you want to register
@@ -814,17 +790,9 @@ static int __init hv_pci_init(void)
return pci_register_driver(&hv_bus_driver);
}
-static void __exit hv_pci_exit(void)
-{
- vmbus_bus_exit();
- pci_unregister_driver(&hv_bus_driver);
-}
-
-
MODULE_LICENSE("GPL");
MODULE_VERSION(HV_DRV_VERSION);
module_param(vmbus_loglevel, int, S_IRUGO|S_IWUSR);
module_init(hv_pci_init);
-module_exit(hv_pci_exit);
--
1.7.4.1
^ permalink raw reply related
* [PATCH 02/18] Staging: hv: Get rid of vmbus_release_unattached_channels() as it is not used
From: K. Y. Srinivasan @ 2011-04-29 20:45 UTC (permalink / raw)
To: gregkh, linux-kernel, devel, virtualization; +Cc: Haiyang Zhang, Abhishek Kane
In-Reply-To: <1304109916-24874-1-git-send-email-kys@microsoft.com>
Since vmbus_release_unattached_channels() is only used in module
unload path and since the vmbus driver cannot be unloaded,
get rid of this "dead" code.
Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
Signed-off-by: Abhishek Kane <v-abkane@microsoft.com>
Signed-off-by: Hank Janssen <hjanssen@microsoft.com>
---
drivers/staging/hv/channel_mgmt.c | 33 ---------------------------------
drivers/staging/hv/channel_mgmt.h | 2 --
2 files changed, 0 insertions(+), 35 deletions(-)
diff --git a/drivers/staging/hv/channel_mgmt.c b/drivers/staging/hv/channel_mgmt.c
index fe32f7e..1929ab3 100644
--- a/drivers/staging/hv/channel_mgmt.c
+++ b/drivers/staging/hv/channel_mgmt.c
@@ -791,37 +791,4 @@ cleanup:
return ret;
}
-/*
- * vmbus_release_unattached_channels - Release channels that are
- * unattached/unconnected ie (no drivers associated)
- */
-void vmbus_release_unattached_channels(void)
-{
- struct vmbus_channel *channel, *pos;
- struct vmbus_channel *start = NULL;
- unsigned long flags;
-
- spin_lock_irqsave(&vmbus_connection.channel_lock, flags);
-
- list_for_each_entry_safe(channel, pos, &vmbus_connection.chn_list,
- listentry) {
- if (channel == start)
- break;
-
- if (!channel->device_obj->drv) {
- list_del(&channel->listentry);
-
- pr_err("Releasing unattached device object\n");
-
- vmbus_child_device_unregister(channel->device_obj);
- free_channel(channel);
- } else {
- if (!start)
- start = channel;
- }
- }
-
- spin_unlock_irqrestore(&vmbus_connection.channel_lock, flags);
-}
-
/* eof */
diff --git a/drivers/staging/hv/channel_mgmt.h b/drivers/staging/hv/channel_mgmt.h
index 96f74e2..3b2c393 100644
--- a/drivers/staging/hv/channel_mgmt.h
+++ b/drivers/staging/hv/channel_mgmt.h
@@ -315,6 +315,4 @@ void vmbus_onmessage(void *context);
int vmbus_request_offers(void);
-void vmbus_release_unattached_channels(void);
-
#endif /* _CHANNEL_MGMT_H_ */
--
1.7.4.1
^ permalink raw reply related
* [PATCH 03/18] Staging: hv: Get rid of the drv field in struct hv_device
From: K. Y. Srinivasan @ 2011-04-29 20:45 UTC (permalink / raw)
To: gregkh, linux-kernel, devel, virtualization; +Cc: Haiyang Zhang, Abhishek Kane
In-Reply-To: <1304109916-24874-1-git-send-email-kys@microsoft.com>
Now, we can rid of the drv field in struct hv_device.
Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
Signed-off-by: Abhishek Kane <v-abkane@microsoft.com>
Signed-off-by: Hank Janssen <hjanssen@microsoft.com>
---
drivers/staging/hv/vmbus_api.h | 3 ---
1 files changed, 0 insertions(+), 3 deletions(-)
diff --git a/drivers/staging/hv/vmbus_api.h b/drivers/staging/hv/vmbus_api.h
index 51fa952..02e3587 100644
--- a/drivers/staging/hv/vmbus_api.h
+++ b/drivers/staging/hv/vmbus_api.h
@@ -103,9 +103,6 @@ struct hv_driver {
/* Base device object */
struct hv_device {
- /* the driver for this device */
- struct hv_driver *drv;
-
char name[64];
struct work_struct probe_failed_work_item;
--
1.7.4.1
^ permalink raw reply related
* [PATCH 04/18] Staging: hv: Cleanup error handling in vmbus_child_device_register()
From: K. Y. Srinivasan @ 2011-04-29 20:45 UTC (permalink / raw)
To: gregkh, linux-kernel, devel, virtualization; +Cc: Haiyang Zhang, Abhishek Kane
In-Reply-To: <1304109916-24874-1-git-send-email-kys@microsoft.com>
Cleanup error handling in vmbus_child_device_register().
Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
Signed-off-by: Abhishek Kane <v-abkane@microsoft.com>
Signed-off-by: Hank Janssen <hjanssen@microsoft.com>
---
drivers/staging/hv/vmbus_drv.c | 3 ---
1 files changed, 0 insertions(+), 3 deletions(-)
diff --git a/drivers/staging/hv/vmbus_drv.c b/drivers/staging/hv/vmbus_drv.c
index d597dd4..1183459 100644
--- a/drivers/staging/hv/vmbus_drv.c
+++ b/drivers/staging/hv/vmbus_drv.c
@@ -720,9 +720,6 @@ int vmbus_child_device_register(struct hv_device *child_device_obj)
*/
ret = device_register(&child_device_obj->device);
- /* vmbus_probe() error does not get propergate to device_register(). */
- ret = child_device_obj->probe_error;
-
if (ret)
pr_err("Unable to register child device\n");
else
--
1.7.4.1
^ permalink raw reply related
* [PATCH 05/18] Staging: hv: Cleanup vmbus_probe() function
From: K. Y. Srinivasan @ 2011-04-29 20:45 UTC (permalink / raw)
To: gregkh, linux-kernel, devel, virtualization; +Cc: Haiyang Zhang, Abhishek Kane
In-Reply-To: <1304109916-24874-1-git-send-email-kys@microsoft.com>
The logic for handling probe failure was broken. Now that we have
cleaned up error handling, get rid of the vmbus_probe_failed_cb()
function.
Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
Signed-off-by: Abhishek Kane <v-abkane@microsoft.com>
Signed-off-by: Hank Janssen <hjanssen@microsoft.com>
---
drivers/staging/hv/vmbus_api.h | 4 ----
drivers/staging/hv/vmbus_drv.c | 31 ++-----------------------------
2 files changed, 2 insertions(+), 33 deletions(-)
diff --git a/drivers/staging/hv/vmbus_api.h b/drivers/staging/hv/vmbus_api.h
index 02e3587..14df762 100644
--- a/drivers/staging/hv/vmbus_api.h
+++ b/drivers/staging/hv/vmbus_api.h
@@ -105,10 +105,6 @@ struct hv_driver {
struct hv_device {
char name[64];
- struct work_struct probe_failed_work_item;
-
- int probe_error;
-
/* the device type id of this device */
struct hv_guid dev_type;
diff --git a/drivers/staging/hv/vmbus_drv.c b/drivers/staging/hv/vmbus_drv.c
index 1183459..5f88249 100644
--- a/drivers/staging/hv/vmbus_drv.c
+++ b/drivers/staging/hv/vmbus_drv.c
@@ -313,27 +313,6 @@ static int vmbus_match(struct device *device, struct device_driver *driver)
return match;
}
-
-/*
- * vmbus_probe_failed_cb - Callback when a driver probe failed in vmbus_probe()
- *
- * We need a callback because we cannot invoked device_unregister() inside
- * vmbus_probe() since vmbus_probe() may be invoked inside device_register()
- * i.e. we cannot call device_unregister() inside device_register()
- */
-static void vmbus_probe_failed_cb(struct work_struct *context)
-{
- struct hv_device *device_ctx = (struct hv_device *)context;
-
- /*
- * Kick off the process of unregistering the device.
- * This will call vmbus_remove() and eventually vmbus_device_release()
- */
- device_unregister(&device_ctx->device);
-
- /* put_device(&device_ctx->device); */
-}
-
/*
* vmbus_probe - Add the new vmbus's child device
*/
@@ -342,20 +321,14 @@ static int vmbus_probe(struct device *child_device)
int ret = 0;
struct hv_driver *drv =
drv_to_hv_drv(child_device->driver);
- struct hv_device *dev = device_to_hv_device(child_device);
/* Let the specific open-source driver handles the probe if it can */
if (drv->driver.probe) {
- ret = dev->probe_error =
- drv->driver.probe(child_device);
- if (ret != 0) {
+ ret = drv->driver.probe(child_device);
+ if (ret != 0)
pr_err("probe failed for device %s (%d)\n",
dev_name(child_device), ret);
- INIT_WORK(&dev->probe_failed_work_item,
- vmbus_probe_failed_cb);
- schedule_work(&dev->probe_failed_work_item);
- }
} else {
pr_err("probe not set for driver %s\n",
dev_name(child_device));
--
1.7.4.1
^ permalink raw reply related
* [PATCH 06/18] Staging: hv: Properly handle errors in hv_pci_probe()
From: K. Y. Srinivasan @ 2011-04-29 20:45 UTC (permalink / raw)
To: gregkh, linux-kernel, devel, virtualization; +Cc: Haiyang Zhang, Abhishek Kane
In-Reply-To: <1304109916-24874-1-git-send-email-kys@microsoft.com>
Much of the vmbus driver initialization is done within the hv_pci_probe()
function. Properly handle errors in hv_pci_probe so that we can
appropriately deal with loading of the vmbus driver.
Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
Signed-off-by: Abhishek Kane <v-abkane@microsoft.com>
Signed-off-by: Hank Janssen <hjanssen@microsoft.com>
---
drivers/staging/hv/vmbus_drv.c | 34 +++++++++++++++++++++++++---------
1 files changed, 25 insertions(+), 9 deletions(-)
diff --git a/drivers/staging/hv/vmbus_drv.c b/drivers/staging/hv/vmbus_drv.c
index 5f88249..8663f64 100644
--- a/drivers/staging/hv/vmbus_drv.c
+++ b/drivers/staging/hv/vmbus_drv.c
@@ -54,6 +54,8 @@ EXPORT_SYMBOL(vmbus_loglevel);
/* (ALL_MODULES << 16 | DEBUG_LVL_ENTEREXIT); */
/* (((VMBUS | VMBUS_DRV)<<16) | DEBUG_LVL_ENTEREXIT); */
+static int pci_probe_error;
+static struct completion probe_event;
static void get_channel_info(struct hv_device *device,
struct hv_device_info *info)
@@ -722,19 +724,19 @@ void vmbus_child_device_unregister(struct hv_device *device_obj)
static int __devinit hv_pci_probe(struct pci_dev *pdev,
const struct pci_device_id *ent)
{
- int err;
-
hv_pci_dev = pdev;
- err = pci_enable_device(pdev);
- if (err)
- return err;
+ pci_probe_error = pci_enable_device(pdev);
+ if (pci_probe_error)
+ goto probe_cleanup;
- err = vmbus_bus_init(pdev);
- if (err)
+ pci_probe_error = vmbus_bus_init(pdev);
+ if (pci_probe_error)
pci_disable_device(pdev);
- return err;
+probe_cleanup:
+ complete(&probe_event);
+ return pci_probe_error;
}
/*
@@ -757,7 +759,21 @@ static struct pci_driver hv_bus_driver = {
static int __init hv_pci_init(void)
{
- return pci_register_driver(&hv_bus_driver);
+ int ret;
+ init_completion(&probe_event);
+ ret = pci_register_driver(&hv_bus_driver);
+ if (ret)
+ return ret;
+ /*
+ * All the vmbus initialization occurs within the
+ * hv_pci_probe() function. Wait for hv_pci_probe()
+ * to complete.
+ */
+ wait_for_completion(&probe_event);
+
+ if (pci_probe_error)
+ pci_unregister_driver(&hv_bus_driver);
+ return pci_probe_error;
}
--
1.7.4.1
^ permalink raw reply related
* [PATCH 07/18] Staging: hv: Make hv_pci_dev a static variable
From: K. Y. Srinivasan @ 2011-04-29 20:45 UTC (permalink / raw)
To: gregkh, linux-kernel, devel, virtualization; +Cc: Haiyang Zhang, Abhishek Kane
In-Reply-To: <1304109916-24874-1-git-send-email-kys@microsoft.com>
Make hv_pci_dev a static variable.
Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
Signed-off-by: Abhishek Kane <v-abkane@microsoft.com>
Signed-off-by: Hank Janssen <hjanssen@microsoft.com>
---
drivers/staging/hv/vmbus_drv.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/staging/hv/vmbus_drv.c b/drivers/staging/hv/vmbus_drv.c
index 8663f64..4106dd3 100644
--- a/drivers/staging/hv/vmbus_drv.c
+++ b/drivers/staging/hv/vmbus_drv.c
@@ -40,7 +40,7 @@
#include "vmbus_private.h"
-struct pci_dev *hv_pci_dev;
+static struct pci_dev *hv_pci_dev;
/* Main vmbus driver data structure */
struct hv_bus {
--
1.7.4.1
^ permalink raw reply related
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