Linux virtualization list
 help / color / mirror / Atom feed
* Re: [Qemu-devel] [PATCH RFC] virtio: put last seen used index into ring itself
From: Rusty Russell @ 2010-05-20  5:08 UTC (permalink / raw)
  To: Avi Kivity
  Cc: qemu-devel, virtualization, linux-kernel, kvm, Michael S. Tsirkin
In-Reply-To: <201005201431.51142.rusty@rustcorp.com.au>

On Thu, 20 May 2010 02:31:50 pm Rusty Russell wrote:
> On Wed, 19 May 2010 05:36:42 pm Avi Kivity wrote:
> > > Note that this is a exclusive->shared->exclusive bounce only, too.
> > >    
> > 
> > A bounce is a bounce.
> 
> I tried to measure this to show that you were wrong, but I was only able
> to show that you're right.  How annoying.  Test code below.

This time for sure!

#define _GNU_SOURCE
#include <unistd.h>
#include <sched.h>
#include <err.h>
#include <stdbool.h>
#include <stdint.h>
#include <string.h>
#include <stdlib.h>
#include <stdio.h>
#include <sys/time.h>
#include <sys/mman.h>

/* We share memory via an mmap. */
struct counter {
	unsigned int cacheline1;
	char pad[256];
	unsigned int cacheline2;
};

#define MAX_BOUNCES 100000000

enum mode {
	SHARE,
	UNSHARE,
	LOCKSHARE,
	LOCKUNSHARE,
};

int main(int argc, char *argv[])
{
	cpu_set_t cpuset;
	volatile struct counter *counter;
	struct timeval start, stop;
	bool child;
	unsigned int count;
	uint64_t usec;
	enum mode mode;

	if (argc != 4)
		errx(1, "Usage: cachebounce share|unshare|lockshare|lockunshare <cpu0> <cpu1>");

	if (strcmp(argv[1], "share") == 0)
		mode = SHARE;
	else if (strcmp(argv[1], "unshare") == 0)
		mode = UNSHARE;
	else if (strcmp(argv[1], "lockshare") == 0)
		mode = LOCKSHARE;
	else if (strcmp(argv[1], "lockunshare") == 0)
		mode = LOCKSHARE;
	else
		errx(1, "Usage: cachebounce share|unshare|lockshare|lockunshare <cpu0> <cpu1>");

	CPU_ZERO(&cpuset);

	counter = mmap(NULL, getpagesize(), PROT_READ|PROT_WRITE, MAP_ANONYMOUS|MAP_SHARED, -1, 0);
	if (counter == MAP_FAILED)
		err(1, "Mapping page");

	/* Fault it in. */
	counter->cacheline1 = counter->cacheline2 = 0;

	child = (fork() == 0);

	CPU_SET(atoi(argv[2 + child]), &cpuset);
	if (sched_setaffinity(getpid(), sizeof(cpu_set_t), &cpuset) != 0)
		err(1, "Calling sched_setaffinity()");

	gettimeofday(&start, NULL);

	if (child) {
		count = 1;
		switch (mode) {
		case SHARE:
			while (count < MAX_BOUNCES) {
				/* Spin waiting for other side to change it. */
				while (counter->cacheline1 != count);
				count++;
				counter->cacheline1 = count;
				count++;
			}
			break;
		case UNSHARE:
			while (count < MAX_BOUNCES) {
				/* Spin waiting for other side to change it. */
				while (counter->cacheline1 != count);
				count++;
				counter->cacheline2 = count;
				count++;
			}
			break;
		case LOCKSHARE:
			while (count < MAX_BOUNCES) {
				/* Spin waiting for other side to change it. */
				while (__sync_val_compare_and_swap(&counter->cacheline1, count, count+1)
				       != count);
				count += 2;
			}
			break;
		case LOCKUNSHARE:
			while (count < MAX_BOUNCES) {
				/* Spin waiting for other side to change it. */
				while (counter->cacheline1 != count);
				__sync_val_compare_and_swap(&counter->cacheline2, count, count+1);
				count += 2;
			}
			break;
		}
	} else {
		count = 0;
		switch (mode) {
		case SHARE:
			while (count < MAX_BOUNCES) {
				/* Spin waiting for other side to change it. */
				while (counter->cacheline1 != count);
				count++;
				counter->cacheline1 = count;
				count++;
			}
			break;
		case UNSHARE:
			while (count < MAX_BOUNCES) {
				/* Spin waiting for other side to change it. */
				while (counter->cacheline2 != count);
				count++;
				counter->cacheline1 = count;
				count++;
			}
			break;
		case LOCKSHARE:
			while (count < MAX_BOUNCES) {
				/* Spin waiting for other side to change it. */
				while (__sync_val_compare_and_swap(&counter->cacheline1, count, count+1)
				       != count);
				count += 2;
			}
			break;
		case LOCKUNSHARE:
			while (count < MAX_BOUNCES) {
				/* Spin waiting for other side to change it. */
				while (counter->cacheline2 != count);
				__sync_val_compare_and_swap(&counter->cacheline1, count, count+1);
				count += 2;
			}
			break;
		}
	}
	gettimeofday(&stop, NULL);
	usec = (stop.tv_sec * 1000000LL + stop.tv_usec)
		- (start.tv_sec * 1000000LL + start.tv_usec);
	printf("CPU %s: %s cacheline: %llu usec\n", argv[2+child], argv[1], usec);
	return 0;
}

^ permalink raw reply

* Re: [Qemu-devel] [PATCH RFC] virtio: put last seen used index into ring itself
From: Rusty Russell @ 2010-05-20  5:01 UTC (permalink / raw)
  To: Avi Kivity
  Cc: qemu-devel, virtualization, linux-kernel, kvm, Michael S. Tsirkin
In-Reply-To: <4BF39C12.7090407@redhat.com>

On Wed, 19 May 2010 05:36:42 pm Avi Kivity wrote:
> > Note that this is a exclusive->shared->exclusive bounce only, too.
> >    
> 
> A bounce is a bounce.

I tried to measure this to show that you were wrong, but I was only able
to show that you're right.  How annoying.  Test code below.

> Virtio is already way too bouncy due to the indirection between the 
> avail/used rings and the descriptor pool.

I tried to do a more careful analysis below, and I think this is an
overstatement.

> A device with out of order 
> completion (like virtio-blk) will quickly randomize the unused 
> descriptor indexes, so every descriptor fetch will require a bounce.
> 
> In contrast, if the rings hold the descriptors themselves instead of 
> pointers, we bounce (sizeof(descriptor)/cache_line_size) cache lines for 
> every descriptor, amortized.

We already have indirect, this would be a logical next step.  So let's
think about it. The avail ring would contain 64 bit values, the used ring
would contain indexes into the avail ring.

So client writes descriptor page and adds to avail ring, then writes to
index.  Server reads index, avail ring, descriptor page (3).  Writes used
entry (1).  Updates last_used (1).  Client reads used (1), derefs avail (1),
updates last_used (1), cleans descriptor page (1).

That's 9 cacheline transfers, worst case.  Best case of a half-full ring
in steady state, assuming 128-byte cache lines, the avail ring costs are
1/16, the used entry is 1/64.  This drops it to 6 and 9/64 transfers.

(Note, the current scheme adds 2 more cacheline transfers, for the descriptor
table, worst case.  Assuming indirect, we get 2/8 xfer best case.  Either way,
it's not the main source of cacheline xfers).

Can we do better?  The obvious idea is to try to get rid of last_used and
used, and use the ring itself.  We would use an invalid entry to mark the
head of the ring.

Any other thoughts?
Rusty.

^ permalink raw reply

* Re: [PATCH] vhost-net: utilize PUBLISH_USED_IDX feature
From: Rusty Russell @ 2010-05-20  4:18 UTC (permalink / raw)
  To: Michael S. Tsirkin
  Cc: Avi Kivity, davem, Juan Quintela, Paul E. McKenney, Arnd Bergmann,
	kvm, virtualization, netdev, linux-kernel, alex.williamson,
	amit.shah
In-Reply-To: <20100519222718.GB4111@redhat.com>

On Thu, 20 May 2010 07:57:18 am Michael S. Tsirkin wrote:
> On Wed, May 19, 2010 at 08:04:51PM +0300, Avi Kivity wrote:
> > On 05/18/2010 04:19 AM, Michael S. Tsirkin wrote:
> >> With PUBLISH_USED_IDX, guest tells us which used entries
> >> it has consumed. This can be used to reduce the number
> >> of interrupts: after we write a used entry, if the guest has not yet
> >> consumed the previous entry, or if the guest has already consumed the
> >> new entry, we do not need to interrupt.
> >> This imporves bandwidth by 30% under some workflows.
> >>
> >> Signed-off-by: Michael S. Tsirkin<mst@redhat.com>
> >> ---
> >>
> >> Rusty, Dave, this patch depends on the patch
> >> "virtio: put last seen used index into ring itself"
> >> which is currently destined at Rusty's tree.
> >> Rusty, if you are taking that one for 2.6.35, please
> >> take this one as well.
> >> Dave, any objections?
> >>    
> >
> > I object: I think the index should have its own cacheline,
> 
> The issue here is that host/guest do not know each
> other's cache line size. I guess we could just put it
> at offset 128 or something like that ... Rusty?

I was assuming you'd put it at the end of the padding.

I think it's a silly optimization, but Avi obviously feels strongly about
it and I respect his opinion.

Please resubmit...
Thanks,
Rusty.

^ permalink raw reply

* Re: [Qemu-devel] [PATCH RFC] virtio: put last seen used index into ring itself
From: Michael S. Tsirkin @ 2010-05-19 22:33 UTC (permalink / raw)
  To: Avi Kivity; +Cc: qemu-devel, linux-kernel, kvm, virtualization
In-Reply-To: <4BF39C12.7090407@redhat.com>

On Wed, May 19, 2010 at 11:06:42AM +0300, Avi Kivity wrote:
> On 05/19/2010 10:39 AM, Rusty Russell wrote:
>>
>> I think we're talking about the last 2 entries of the avail ring.  That means
>> the worst case is 1 false bounce every time around the ring.
>
> It's low, but why introduce an inefficiency when you can avoid doing it  
> for the same effort?
> 
>> I think that's
>> why we're debating it instead of measuring it :)
>>    
>
> Measure before optimize is good for code but not for protocols.   
> Protocols have to be robust against future changes.  Virtio is warty  
> enough already, we can't keep doing local optimizations.
>
>> Note that this is a exclusive->shared->exclusive bounce only, too.
>>    
>
> A bounce is a bounce.
>
> Virtio is already way too bouncy due to the indirection between the  
> avail/used rings and the descriptor pool.  A device with out of order  
> completion (like virtio-blk) will quickly randomize the unused  
> descriptor indexes, so every descriptor fetch will require a bounce.
>
> In contrast, if the rings hold the descriptors themselves instead of  
> pointers, we bounce (sizeof(descriptor)/cache_line_size) cache lines for  
> every descriptor, amortized.

On the other hand, consider that on fast path we are never using all
of the ring. With a good allocator we might be able to keep
reusing only small part of the ring, instead of wrapping around
all of it all of the time.


> -- 
> Do not meddle in the internals of kernels, for they are subtle and quick to panic.

^ permalink raw reply

* Re: [PATCH] vhost-net: utilize PUBLISH_USED_IDX feature
From: Michael S. Tsirkin @ 2010-05-19 22:27 UTC (permalink / raw)
  To: Avi Kivity
  Cc: davem, Juan Quintela, Rusty Russell, Paul E. McKenney,
	Arnd Bergmann, kvm, virtualization, netdev, linux-kernel,
	alex.williamson, amit.shah
In-Reply-To: <4BF41A33.8090309@redhat.com>

On Wed, May 19, 2010 at 08:04:51PM +0300, Avi Kivity wrote:
> On 05/18/2010 04:19 AM, Michael S. Tsirkin wrote:
>> With PUBLISH_USED_IDX, guest tells us which used entries
>> it has consumed. This can be used to reduce the number
>> of interrupts: after we write a used entry, if the guest has not yet
>> consumed the previous entry, or if the guest has already consumed the
>> new entry, we do not need to interrupt.
>> This imporves bandwidth by 30% under some workflows.
>>
>> Signed-off-by: Michael S. Tsirkin<mst@redhat.com>
>> ---
>>
>> Rusty, Dave, this patch depends on the patch
>> "virtio: put last seen used index into ring itself"
>> which is currently destined at Rusty's tree.
>> Rusty, if you are taking that one for 2.6.35, please
>> take this one as well.
>> Dave, any objections?
>>    
>
> I object: I think the index should have its own cacheline,

The issue here is that host/guest do not know each
other's cache line size. I guess we could just put it
at offset 128 or something like that ... Rusty?

> and that it should be documented before merging.

I think you meant to object to the virtio patch, not this one.  This
patch does not introduce new layout, just implements host support.
virtio spec patch will follow: it is not part of linux tree so
there is no patch dependency.

> -- 
> Do not meddle in the internals of kernels, for they are subtle and quick to panic.

^ permalink raw reply

* Re: [PATCH 1/2] staging: hv: Fix race condition in hv_utils module initialization.
From: Greg KH @ 2010-05-19 22:21 UTC (permalink / raw)
  To: Haiyang Zhang
  Cc: 'linux-kernel@vger.kernel.org',
	'devel@driverdev.osuosl.org',
	'virtualization@lists.osdl.org', Hank Janssen
In-Reply-To: <1FB5E1D5CA062146B38059374562DF7266B8951F@TK5EX14MBXC128.redmond.corp.microsoft.com>

On Wed, May 19, 2010 at 10:12:51PM +0000, Haiyang Zhang wrote:
> > > Actually, we already assign a default callback function,
> > chn_cb_negotiate(),
> > > when the channels are opened in vmbus module. It's a real function
> > and can
> > > handle common negotiation messages.
> > 
> > Then why don't you use it here?
> 
> When vmbus is loaded and channel is offered from HyperV host, the default
> callback function, chn_cb_negotiate(), is assigned to the function ptr, and
> used to do basic responses of negotiation messages. 

Great, so that works.

> After hv_utils modules is loaded the callback function ptr is overridden by 
> a specialized function in hv_utils module, and handles each feature (shutdown, 
> timesync, etc.) differently.

That's the problem.  Provide a "correct" interface to properly change
the callback function.  Just setting function pointers in a random
manner is ripe for all sorts of bad problems, don't you agree?  Heck, I
don't see any locking happening here which could cause messages to be
handled when things are only half-way set up.  Also, what's to say your
function pointer write is atomic in the first place :)

In short, use proper locking for something like this.

> > I still think there's a real problem somewhere else in the architecture
> > if such a sleep is necessary...
> > 
> > Is the issue that the modprobe of the hv_vmbus can return before the
> > bus
> > is really all set up and ready to go?  If so, just fix that, then you
> > will not need any "sleep" calls anywhere, right?
> 
> After vmbus is loaded, the channel offering will come from the host, then
> it initializes the channel. The channel offering can happen a little later
> after vmbus_init() is done and modprobe returns. So I think we should let
> vmbus_init function wait(sleep) until all channel offerings are received before
> returning. This will ensure all channels are ready before modprobe returns.

That sounds like a good idea.

thanks,

greg k-h

^ permalink raw reply

* RE: [PATCH 1/2] staging: hv: Fix race condition in hv_utils module initialization.
From: Haiyang Zhang @ 2010-05-19 22:12 UTC (permalink / raw)
  To: Greg KH
  Cc: 'linux-kernel@vger.kernel.org',
	'devel@driverdev.osuosl.org',
	'virtualization@lists.osdl.org', Hank Janssen
In-Reply-To: <20100519203901.GA23214@suse.de>

> > Actually, we already assign a default callback function,
> chn_cb_negotiate(),
> > when the channels are opened in vmbus module. It's a real function
> and can
> > handle common negotiation messages.
> 
> Then why don't you use it here?

When vmbus is loaded and channel is offered from HyperV host, the default
callback function, chn_cb_negotiate(), is assigned to the function ptr, and
used to do basic responses of negotiation messages. 

After hv_utils modules is loaded the callback function ptr is overridden by 
a specialized function in hv_utils module, and handles each feature (shutdown, 
timesync, etc.) differently.

> I still think there's a real problem somewhere else in the architecture
> if such a sleep is necessary...
> 
> Is the issue that the modprobe of the hv_vmbus can return before the
> bus
> is really all set up and ready to go?  If so, just fix that, then you
> will not need any "sleep" calls anywhere, right?

After vmbus is loaded, the channel offering will come from the host, then
it initializes the channel. The channel offering can happen a little later
after vmbus_init() is done and modprobe returns. So I think we should let
vmbus_init function wait(sleep) until all channel offerings are received before
returning. This will ensure all channels are ready before modprobe returns.

Thanks,

- Haiyang

^ permalink raw reply

* Re: [PATCH 1/2] staging: hv: Fix race condition in hv_utils module initialization.
From: Greg KH @ 2010-05-19 20:39 UTC (permalink / raw)
  To: Haiyang Zhang
  Cc: 'linux-kernel@vger.kernel.org',
	'devel@driverdev.osuosl.org',
	'virtualization@lists.osdl.org', Hank Janssen
In-Reply-To: <1FB5E1D5CA062146B38059374562DF7266B894B0@TK5EX14MBXC128.redmond.corp.microsoft.com>

On Wed, May 19, 2010 at 08:30:25PM +0000, Haiyang Zhang wrote:
> > > +	/* Wait until all IC channels are initialized */
> > > +	while (atomic_read(&hv_utils_initcnt) < MAX_MSG_TYPES)
> > > +		msleep(100);
> > 
> > No, don't do this here, do something in your hv_vmbus core to handle
> > registering sub-drivers properly.  Perhaps you need to sleep there
> > before you can succeed on a initialization.
> 
> Thanks for the recommendation. I will put the sleep into vmbus_init to 
> ensure all channels are ready before the vmbus_init function exits.
> 
> > >  	hv_cb_utils[HV_SHUTDOWN_MSG].channel->OnChannelCallback =
> > >  		&shutdown_onchannelcallback;
> > 
> > The problem is that you just have a bunch of callbacks you are setting
> > up, it's not a "real" function call.  Please change it over to a
> > function call, like all other subsystems have.  Then, you can handle
> > any
> > "sleep until we are set up properly" issues in the vmbus code, not in
> > each and every individual bus driver.
> 
> Actually, we already assign a default callback function, chn_cb_negotiate(),
> when the channels are opened in vmbus module. It's a real function and can 
> handle common negotiation messages.

Then why don't you use it here?

> I will move the sleep into vmbus module as well.

I still think there's a real problem somewhere else in the architecture
if such a sleep is necessary...

Is the issue that the modprobe of the hv_vmbus can return before the bus
is really all set up and ready to go?  If so, just fix that, then you
will not need any "sleep" calls anywhere, right?

thanks,

greg k-h

^ permalink raw reply

* RE: [PATCH 1/2] staging: hv: Fix race condition in hv_utils module initialization.
From: Haiyang Zhang @ 2010-05-19 20:30 UTC (permalink / raw)
  To: Greg KH
  Cc: 'linux-kernel@vger.kernel.org',
	'devel@driverdev.osuosl.org',
	'virtualization@lists.osdl.org', Hank Janssen
In-Reply-To: <20100519161011.GA20266@suse.de>

> > +	/* Wait until all IC channels are initialized */
> > +	while (atomic_read(&hv_utils_initcnt) < MAX_MSG_TYPES)
> > +		msleep(100);
> 
> No, don't do this here, do something in your hv_vmbus core to handle
> registering sub-drivers properly.  Perhaps you need to sleep there
> before you can succeed on a initialization.

Thanks for the recommendation. I will put the sleep into vmbus_init to 
ensure all channels are ready before the vmbus_init function exits.

> >  	hv_cb_utils[HV_SHUTDOWN_MSG].channel->OnChannelCallback =
> >  		&shutdown_onchannelcallback;
> 
> The problem is that you just have a bunch of callbacks you are setting
> up, it's not a "real" function call.  Please change it over to a
> function call, like all other subsystems have.  Then, you can handle
> any
> "sleep until we are set up properly" issues in the vmbus code, not in
> each and every individual bus driver.

Actually, we already assign a default callback function, chn_cb_negotiate(),
when the channels are opened in vmbus module. It's a real function and can 
handle common negotiation messages.
I will move the sleep into vmbus module as well.

Thanks,

- Haiyang

^ permalink raw reply

* Re: [PATCH] vhost-net: utilize PUBLISH_USED_IDX feature
From: Avi Kivity @ 2010-05-19 17:04 UTC (permalink / raw)
  To: Michael S. Tsirkin
  Cc: davem, Juan Quintela, Rusty Russell, Paul E. McKenney,
	Arnd Bergmann, kvm, virtualization, netdev, linux-kernel,
	alex.williamson, amit.shah
In-Reply-To: <20100518011931.GA21918@redhat.com>

On 05/18/2010 04:19 AM, Michael S. Tsirkin wrote:
> With PUBLISH_USED_IDX, guest tells us which used entries
> it has consumed. This can be used to reduce the number
> of interrupts: after we write a used entry, if the guest has not yet
> consumed the previous entry, or if the guest has already consumed the
> new entry, we do not need to interrupt.
> This imporves bandwidth by 30% under some workflows.
>
> Signed-off-by: Michael S. Tsirkin<mst@redhat.com>
> ---
>
> Rusty, Dave, this patch depends on the patch
> "virtio: put last seen used index into ring itself"
> which is currently destined at Rusty's tree.
> Rusty, if you are taking that one for 2.6.35, please
> take this one as well.
> Dave, any objections?
>    

I object: I think the index should have its own cacheline, and that it 
should be documented before merging.

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.

^ permalink raw reply

* Re: [PATCHv2] vhost-net: utilize PUBLISH_USED_IDX feature
From: Avi Kivity @ 2010-05-19 16:15 UTC (permalink / raw)
  To: Michael S. Tsirkin
  Cc: davem, Juan Quintela, Rusty Russell, Paul E. McKenney,
	Arnd Bergmann, kvm, virtualization, netdev, linux-kernel,
	alex.williamson, amit.shah
In-Reply-To: <4BF2D2A7.8030803@redhat.com>

On 05/18/2010 08:47 PM, Avi Kivity wrote:
> On 05/18/2010 05:21 AM, Michael S. Tsirkin wrote:
>> With PUBLISH_USED_IDX, guest tells us which used entries
>> it has consumed. This can be used to reduce the number
>> of interrupts: after we write a used entry, if the guest has not yet
>> consumed the previous entry, or if the guest has already consumed the
>> new entry, we do not need to interrupt.
>> This imporves bandwidth by 30% under some workflows.
>
> Seems to be missing the cacheline alignment.
>
> Rusty's clarification did not satisfy me, I think it's needed.
>

Oh, and this should definitely follow the patch to the virtio spec, not 
precede it.

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.

^ permalink raw reply

* Re: [PATCH 1/2] staging: hv: Fix race condition in hv_utils module initialization.
From: Greg KH @ 2010-05-19 16:10 UTC (permalink / raw)
  To: Haiyang Zhang
  Cc: 'linux-kernel@vger.kernel.org',
	'devel@driverdev.osuosl.org',
	'virtualization@lists.osdl.org', Hank Janssen
In-Reply-To: <1FB5E1D5CA062146B38059374562DF7266B8930E@TK5EX14MBXC128.redmond.corp.microsoft.com>

On Wed, May 19, 2010 at 03:54:17PM +0000, Haiyang Zhang wrote:
> From: Haiyang Zhang <haiyangz@microsoft.com>
> 
> Subject: [PATCH 1/2] staging: hv: Fix race condition in hv_utils module initialization.
> There is a possible race condition when hv_utils starts to load immediately
> after hv_vmbus is loading - null pointer error could happen.
> This patch added an atomic counter to ensure the hv_utils module initialization
> happens after all vmbus IC channels are initialized.
> 
> Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
> Signed-off-by: Hank Janssen <hjanssen@microsoft.com>
> 
> ---
>  drivers/staging/hv/channel_mgmt.c |   26 +++++++++++++++-----------
>  drivers/staging/hv/hv_utils.c     |   11 ++++++++---
>  drivers/staging/hv/utils.h        |    5 +++++
>  3 files changed, 28 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/staging/hv/channel_mgmt.c b/drivers/staging/hv/channel_mgmt.c
> index 3f53b4d..b5b6a70 100644
> --- a/drivers/staging/hv/channel_mgmt.c
> +++ b/drivers/staging/hv/channel_mgmt.c
> @@ -33,7 +33,6 @@ struct vmbus_channel_message_table_entry {
>  	void (*messageHandler)(struct vmbus_channel_message_header *msg);
>  };
>  
> -#define MAX_MSG_TYPES                    3
>  #define MAX_NUM_DEVICE_CLASSES_SUPPORTED 7
>  
>  static const struct hv_guid
> @@ -233,6 +232,11 @@ struct hyperv_service_callback hv_cb_utils[MAX_MSG_TYPES] = {
>  };
>  EXPORT_SYMBOL(hv_cb_utils);
>  
> +/* Counter of IC channels initialized */
> +atomic_t hv_utils_initcnt = ATOMIC_INIT(0);
> +EXPORT_SYMBOL(hv_utils_initcnt);
> +
> +
>  /*
>   * AllocVmbusChannel - Allocate and initialize a vmbus channel object
>   */
> @@ -373,22 +377,22 @@ static void VmbusChannelProcessOffer(void *context)
>  		 * can cleanup properly
>  		 */
>  		newChannel->State = CHANNEL_OPEN_STATE;
> -		cnt = 0;
>  
> -		while (cnt != MAX_MSG_TYPES) {
> +		/* Open IC channels */
> +		for (cnt = 0; cnt < MAX_MSG_TYPES; cnt++) {
>  			if (memcmp(&newChannel->OfferMsg.Offer.InterfaceType,
>  				   &hv_cb_utils[cnt].data,
> -				   sizeof(struct hv_guid)) == 0) {
> +				   sizeof(struct hv_guid)) == 0 &&
> +			    VmbusChannelOpen(newChannel, 2 * PAGE_SIZE,
> +					     2 * PAGE_SIZE, NULL, 0,
> +					     hv_cb_utils[cnt].callback,
> +					     newChannel) == 0) {
> +				hv_cb_utils[cnt].channel = newChannel;
> +				mb();
>  				DPRINT_INFO(VMBUS, "%s",
>  					    hv_cb_utils[cnt].log_msg);
> -
> -				if (VmbusChannelOpen(newChannel, 2 * PAGE_SIZE,
> -						    2 * PAGE_SIZE, NULL, 0,
> -						    hv_cb_utils[cnt].callback,
> -						    newChannel) == 0)
> -					hv_cb_utils[cnt].channel = newChannel;
> +				atomic_inc(&hv_utils_initcnt);
>  			}
> -			cnt++;
>  		}
>  	}
>  	DPRINT_EXIT(VMBUS);
> diff --git a/drivers/staging/hv/hv_utils.c b/drivers/staging/hv/hv_utils.c
> index 8a49aaf..f9826c7 100644
> --- a/drivers/staging/hv/hv_utils.c
> +++ b/drivers/staging/hv/hv_utils.c
> @@ -253,7 +253,11 @@ static void heartbeat_onchannelcallback(void *context)
>  
>  static int __init init_hyperv_utils(void)
>  {
> -	printk(KERN_INFO "Registering HyperV Utility Driver\n");
> +	printk(KERN_INFO "Registering HyperV Utility Driver...\n");
> +
> +	/* Wait until all IC channels are initialized */
> +	while (atomic_read(&hv_utils_initcnt) < MAX_MSG_TYPES)
> +		msleep(100);

No, don't do this here, do something in your hv_vmbus core to handle
registering sub-drivers properly.  Perhaps you need to sleep there
before you can succeed on a initialization.

>  
>  	hv_cb_utils[HV_SHUTDOWN_MSG].channel->OnChannelCallback =
>  		&shutdown_onchannelcallback;

The problem is that you just have a bunch of callbacks you are setting
up, it's not a "real" function call.  Please change it over to a
function call, like all other subsystems have.  Then, you can handle any
"sleep until we are set up properly" issues in the vmbus code, not in
each and every individual bus driver.

thanks,

greg k-h

^ permalink raw reply

* [PATCH 2/2] staging: hv: Add autoloading to hv_utils module
From: Haiyang Zhang @ 2010-05-19 15:56 UTC (permalink / raw)
  To: 'linux-kernel@vger.kernel.org',
	'devel@driverdev.osuosl.org',
	"'virtualization@lists.osdl.org'" <virtualiz>

[-- Attachment #1: Type: text/plain, Size: 1834 bytes --]

From: Haiyang Zhang <haiyangz@microsoft.com>

Subject: [PATCH 2/2] staging: hv: Add autoloading to hv_utils module
Added autoloading based on pci id and dmi strings.

Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
Signed-off-by: Hank Janssen <hjanssen@microsoft.com>

---
 drivers/staging/hv/hv_utils.c |   29 +++++++++++++++++++++++++++++
 1 files changed, 29 insertions(+), 0 deletions(-)

diff --git a/drivers/staging/hv/hv_utils.c b/drivers/staging/hv/hv_utils.c
index f9826c7..b831a65 100644
--- a/drivers/staging/hv/hv_utils.c
+++ b/drivers/staging/hv/hv_utils.c
@@ -24,6 +24,8 @@
 #include <linux/slab.h>
 #include <linux/sysctl.h>
 #include <linux/reboot.h>
+#include <linux/dmi.h>
+#include <linux/pci.h>
 
 #include "logging.h"
 #include "osd.h"
@@ -251,10 +253,37 @@ static void heartbeat_onchannelcallback(void *context)
 	DPRINT_EXIT(VMBUS);
 }
 
+
+static const struct pci_device_id __initconst
+hv_utils_pci_table[] __maybe_unused = {
+	{ PCI_DEVICE(0x1414, 0x5353) }, /* Hyper-V emulated VGA controller */
+	{ 0 }
+};
+MODULE_DEVICE_TABLE(pci, hv_utils_pci_table);
+
+
+static const struct dmi_system_id __initconst
+hv_utils_dmi_table[] __maybe_unused  = {
+	{
+		.ident = "Hyper-V",
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "Microsoft Corporation"),
+			DMI_MATCH(DMI_PRODUCT_NAME, "Virtual Machine"),
+			DMI_MATCH(DMI_BOARD_NAME, "Virtual Machine"),
+		},
+	},
+	{ },
+};
+MODULE_DEVICE_TABLE(dmi, hv_utils_dmi_table);
+
+
 static int __init init_hyperv_utils(void)
 {
 	printk(KERN_INFO "Registering HyperV Utility Driver...\n");
 
+	if (!dmi_check_system(hv_utils_dmi_table))
+		return -ENODEV;
+
 	/* Wait until all IC channels are initialized */
 	while (atomic_read(&hv_utils_initcnt) < MAX_MSG_TYPES)
 		msleep(100);
-- 
1.6.3.2


[-- Attachment #2: 0518-2-Add-autoloading-to-hv_utils-module.patch --]
[-- Type: application/octet-stream, Size: 1768 bytes --]

From: Haiyang Zhang <haiyangz@microsoft.com>

Subject: [PATCH 2/2] staging: hv: Add autoloading to hv_utils module
Added autoloading based on pci id and dmi strings.

Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
Signed-off-by: Hank Janssen <hjanssen@microsoft.com>

---
 drivers/staging/hv/hv_utils.c |   29 +++++++++++++++++++++++++++++
 1 files changed, 29 insertions(+), 0 deletions(-)

diff --git a/drivers/staging/hv/hv_utils.c b/drivers/staging/hv/hv_utils.c
index f9826c7..b831a65 100644
--- a/drivers/staging/hv/hv_utils.c
+++ b/drivers/staging/hv/hv_utils.c
@@ -24,6 +24,8 @@
 #include <linux/slab.h>
 #include <linux/sysctl.h>
 #include <linux/reboot.h>
+#include <linux/dmi.h>
+#include <linux/pci.h>
 
 #include "logging.h"
 #include "osd.h"
@@ -251,10 +253,37 @@ static void heartbeat_onchannelcallback(void *context)
 	DPRINT_EXIT(VMBUS);
 }
 
+
+static const struct pci_device_id __initconst
+hv_utils_pci_table[] __maybe_unused = {
+	{ PCI_DEVICE(0x1414, 0x5353) }, /* Hyper-V emulated VGA controller */
+	{ 0 }
+};
+MODULE_DEVICE_TABLE(pci, hv_utils_pci_table);
+
+
+static const struct dmi_system_id __initconst
+hv_utils_dmi_table[] __maybe_unused  = {
+	{
+		.ident = "Hyper-V",
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "Microsoft Corporation"),
+			DMI_MATCH(DMI_PRODUCT_NAME, "Virtual Machine"),
+			DMI_MATCH(DMI_BOARD_NAME, "Virtual Machine"),
+		},
+	},
+	{ },
+};
+MODULE_DEVICE_TABLE(dmi, hv_utils_dmi_table);
+
+
 static int __init init_hyperv_utils(void)
 {
 	printk(KERN_INFO "Registering HyperV Utility Driver...\n");
 
+	if (!dmi_check_system(hv_utils_dmi_table))
+		return -ENODEV;
+
 	/* Wait until all IC channels are initialized */
 	while (atomic_read(&hv_utils_initcnt) < MAX_MSG_TYPES)
 		msleep(100);
-- 
1.6.3.2


[-- Attachment #3: Type: text/plain, Size: 184 bytes --]

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization

^ permalink raw reply related

* [PATCH 1/2] staging: hv: Fix race condition in hv_utils module initialization.
From: Haiyang Zhang @ 2010-05-19 15:54 UTC (permalink / raw)
  To: 'linux-kernel@vger.kernel.org',
	'devel@driverdev.osuosl.org',
	"'virtualization@lists.osdl.org'" <virtualiz>

[-- Attachment #1: Type: text/plain, Size: 4803 bytes --]

From: Haiyang Zhang <haiyangz@microsoft.com>

Subject: [PATCH 1/2] staging: hv: Fix race condition in hv_utils module initialization.
There is a possible race condition when hv_utils starts to load immediately
after hv_vmbus is loading - null pointer error could happen.
This patch added an atomic counter to ensure the hv_utils module initialization
happens after all vmbus IC channels are initialized.

Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
Signed-off-by: Hank Janssen <hjanssen@microsoft.com>

---
 drivers/staging/hv/channel_mgmt.c |   26 +++++++++++++++-----------
 drivers/staging/hv/hv_utils.c     |   11 ++++++++---
 drivers/staging/hv/utils.h        |    5 +++++
 3 files changed, 28 insertions(+), 14 deletions(-)

diff --git a/drivers/staging/hv/channel_mgmt.c b/drivers/staging/hv/channel_mgmt.c
index 3f53b4d..b5b6a70 100644
--- a/drivers/staging/hv/channel_mgmt.c
+++ b/drivers/staging/hv/channel_mgmt.c
@@ -33,7 +33,6 @@ struct vmbus_channel_message_table_entry {
 	void (*messageHandler)(struct vmbus_channel_message_header *msg);
 };
 
-#define MAX_MSG_TYPES                    3
 #define MAX_NUM_DEVICE_CLASSES_SUPPORTED 7
 
 static const struct hv_guid
@@ -233,6 +232,11 @@ struct hyperv_service_callback hv_cb_utils[MAX_MSG_TYPES] = {
 };
 EXPORT_SYMBOL(hv_cb_utils);
 
+/* Counter of IC channels initialized */
+atomic_t hv_utils_initcnt = ATOMIC_INIT(0);
+EXPORT_SYMBOL(hv_utils_initcnt);
+
+
 /*
  * AllocVmbusChannel - Allocate and initialize a vmbus channel object
  */
@@ -373,22 +377,22 @@ static void VmbusChannelProcessOffer(void *context)
 		 * can cleanup properly
 		 */
 		newChannel->State = CHANNEL_OPEN_STATE;
-		cnt = 0;
 
-		while (cnt != MAX_MSG_TYPES) {
+		/* Open IC channels */
+		for (cnt = 0; cnt < MAX_MSG_TYPES; cnt++) {
 			if (memcmp(&newChannel->OfferMsg.Offer.InterfaceType,
 				   &hv_cb_utils[cnt].data,
-				   sizeof(struct hv_guid)) == 0) {
+				   sizeof(struct hv_guid)) == 0 &&
+			    VmbusChannelOpen(newChannel, 2 * PAGE_SIZE,
+					     2 * PAGE_SIZE, NULL, 0,
+					     hv_cb_utils[cnt].callback,
+					     newChannel) == 0) {
+				hv_cb_utils[cnt].channel = newChannel;
+				mb();
 				DPRINT_INFO(VMBUS, "%s",
 					    hv_cb_utils[cnt].log_msg);
-
-				if (VmbusChannelOpen(newChannel, 2 * PAGE_SIZE,
-						    2 * PAGE_SIZE, NULL, 0,
-						    hv_cb_utils[cnt].callback,
-						    newChannel) == 0)
-					hv_cb_utils[cnt].channel = newChannel;
+				atomic_inc(&hv_utils_initcnt);
 			}
-			cnt++;
 		}
 	}
 	DPRINT_EXIT(VMBUS);
diff --git a/drivers/staging/hv/hv_utils.c b/drivers/staging/hv/hv_utils.c
index 8a49aaf..f9826c7 100644
--- a/drivers/staging/hv/hv_utils.c
+++ b/drivers/staging/hv/hv_utils.c
@@ -253,7 +253,11 @@ static void heartbeat_onchannelcallback(void *context)
 
 static int __init init_hyperv_utils(void)
 {
-	printk(KERN_INFO "Registering HyperV Utility Driver\n");
+	printk(KERN_INFO "Registering HyperV Utility Driver...\n");
+
+	/* Wait until all IC channels are initialized */
+	while (atomic_read(&hv_utils_initcnt) < MAX_MSG_TYPES)
+		msleep(100);
 
 	hv_cb_utils[HV_SHUTDOWN_MSG].channel->OnChannelCallback =
 		&shutdown_onchannelcallback;
@@ -267,13 +271,12 @@ static int __init init_hyperv_utils(void)
 		&heartbeat_onchannelcallback;
 	hv_cb_utils[HV_HEARTBEAT_MSG].callback = &heartbeat_onchannelcallback;
 
+	printk(KERN_INFO "Registered HyperV Utility Driver.\n");
 	return 0;
 }
 
 static void exit_hyperv_utils(void)
 {
-	printk(KERN_INFO "De-Registered HyperV Utility Driver\n");
-
 	hv_cb_utils[HV_SHUTDOWN_MSG].channel->OnChannelCallback =
 		&chn_cb_negotiate;
 	hv_cb_utils[HV_SHUTDOWN_MSG].callback = &chn_cb_negotiate;
@@ -285,6 +288,8 @@ static void exit_hyperv_utils(void)
 	hv_cb_utils[HV_HEARTBEAT_MSG].channel->OnChannelCallback =
 		&chn_cb_negotiate;
 	hv_cb_utils[HV_HEARTBEAT_MSG].callback = &chn_cb_negotiate;
+
+	printk(KERN_INFO "De-Registered HyperV Utility Driver.\n");
 }
 
 module_init(init_hyperv_utils);
diff --git a/drivers/staging/hv/utils.h b/drivers/staging/hv/utils.h
index 7c07499..3291ab4 100644
--- a/drivers/staging/hv/utils.h
+++ b/drivers/staging/hv/utils.h
@@ -98,6 +98,10 @@ struct ictimesync_data{
 	u8 flags;
 } __attribute__((packed));
 
+
+/* Number of IC types supported */
+#define MAX_MSG_TYPES	3
+
 /* Index for each IC struct in array hv_cb_utils[] */
 #define HV_SHUTDOWN_MSG		0
 #define HV_TIMESYNC_MSG		1
@@ -115,5 +119,6 @@ extern void prep_negotiate_resp(struct icmsg_hdr *,
 				struct icmsg_negotiate *, u8 *);
 extern void chn_cb_negotiate(void *);
 extern struct hyperv_service_callback hv_cb_utils[];
+extern atomic_t hv_utils_initcnt;
 
 #endif /* __HV_UTILS_H_ */
-- 
1.6.3.2


[-- Attachment #2: 0518-1-Fix-race-condition-in-hv_utils-module-initialization.patch --]
[-- Type: application/octet-stream, Size: 4663 bytes --]

From: Haiyang Zhang <haiyangz@microsoft.com>

Subject: [PATCH 1/2] staging: hv: Fix race condition in hv_utils module initialization.
There is a possible race condition when hv_utils starts to load immediately
after hv_vmbus is loading - null pointer error could happen.
This patch added an atomic counter to ensure the hv_utils module initialization
happens after all vmbus IC channels are initialized.

Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
Signed-off-by: Hank Janssen <hjanssen@microsoft.com>

---
 drivers/staging/hv/channel_mgmt.c |   26 +++++++++++++++-----------
 drivers/staging/hv/hv_utils.c     |   11 ++++++++---
 drivers/staging/hv/utils.h        |    5 +++++
 3 files changed, 28 insertions(+), 14 deletions(-)

diff --git a/drivers/staging/hv/channel_mgmt.c b/drivers/staging/hv/channel_mgmt.c
index 3f53b4d..b5b6a70 100644
--- a/drivers/staging/hv/channel_mgmt.c
+++ b/drivers/staging/hv/channel_mgmt.c
@@ -33,7 +33,6 @@ struct vmbus_channel_message_table_entry {
 	void (*messageHandler)(struct vmbus_channel_message_header *msg);
 };
 
-#define MAX_MSG_TYPES                    3
 #define MAX_NUM_DEVICE_CLASSES_SUPPORTED 7
 
 static const struct hv_guid
@@ -233,6 +232,11 @@ struct hyperv_service_callback hv_cb_utils[MAX_MSG_TYPES] = {
 };
 EXPORT_SYMBOL(hv_cb_utils);
 
+/* Counter of IC channels initialized */
+atomic_t hv_utils_initcnt = ATOMIC_INIT(0);
+EXPORT_SYMBOL(hv_utils_initcnt);
+
+
 /*
  * AllocVmbusChannel - Allocate and initialize a vmbus channel object
  */
@@ -373,22 +377,22 @@ static void VmbusChannelProcessOffer(void *context)
 		 * can cleanup properly
 		 */
 		newChannel->State = CHANNEL_OPEN_STATE;
-		cnt = 0;
 
-		while (cnt != MAX_MSG_TYPES) {
+		/* Open IC channels */
+		for (cnt = 0; cnt < MAX_MSG_TYPES; cnt++) {
 			if (memcmp(&newChannel->OfferMsg.Offer.InterfaceType,
 				   &hv_cb_utils[cnt].data,
-				   sizeof(struct hv_guid)) == 0) {
+				   sizeof(struct hv_guid)) == 0 &&
+			    VmbusChannelOpen(newChannel, 2 * PAGE_SIZE,
+					     2 * PAGE_SIZE, NULL, 0,
+					     hv_cb_utils[cnt].callback,
+					     newChannel) == 0) {
+				hv_cb_utils[cnt].channel = newChannel;
+				mb();
 				DPRINT_INFO(VMBUS, "%s",
 					    hv_cb_utils[cnt].log_msg);
-
-				if (VmbusChannelOpen(newChannel, 2 * PAGE_SIZE,
-						    2 * PAGE_SIZE, NULL, 0,
-						    hv_cb_utils[cnt].callback,
-						    newChannel) == 0)
-					hv_cb_utils[cnt].channel = newChannel;
+				atomic_inc(&hv_utils_initcnt);
 			}
-			cnt++;
 		}
 	}
 	DPRINT_EXIT(VMBUS);
diff --git a/drivers/staging/hv/hv_utils.c b/drivers/staging/hv/hv_utils.c
index 8a49aaf..f9826c7 100644
--- a/drivers/staging/hv/hv_utils.c
+++ b/drivers/staging/hv/hv_utils.c
@@ -253,7 +253,11 @@ static void heartbeat_onchannelcallback(void *context)
 
 static int __init init_hyperv_utils(void)
 {
-	printk(KERN_INFO "Registering HyperV Utility Driver\n");
+	printk(KERN_INFO "Registering HyperV Utility Driver...\n");
+
+	/* Wait until all IC channels are initialized */
+	while (atomic_read(&hv_utils_initcnt) < MAX_MSG_TYPES)
+		msleep(100);
 
 	hv_cb_utils[HV_SHUTDOWN_MSG].channel->OnChannelCallback =
 		&shutdown_onchannelcallback;
@@ -267,13 +271,12 @@ static int __init init_hyperv_utils(void)
 		&heartbeat_onchannelcallback;
 	hv_cb_utils[HV_HEARTBEAT_MSG].callback = &heartbeat_onchannelcallback;
 
+	printk(KERN_INFO "Registered HyperV Utility Driver.\n");
 	return 0;
 }
 
 static void exit_hyperv_utils(void)
 {
-	printk(KERN_INFO "De-Registered HyperV Utility Driver\n");
-
 	hv_cb_utils[HV_SHUTDOWN_MSG].channel->OnChannelCallback =
 		&chn_cb_negotiate;
 	hv_cb_utils[HV_SHUTDOWN_MSG].callback = &chn_cb_negotiate;
@@ -285,6 +288,8 @@ static void exit_hyperv_utils(void)
 	hv_cb_utils[HV_HEARTBEAT_MSG].channel->OnChannelCallback =
 		&chn_cb_negotiate;
 	hv_cb_utils[HV_HEARTBEAT_MSG].callback = &chn_cb_negotiate;
+
+	printk(KERN_INFO "De-Registered HyperV Utility Driver.\n");
 }
 
 module_init(init_hyperv_utils);
diff --git a/drivers/staging/hv/utils.h b/drivers/staging/hv/utils.h
index 7c07499..3291ab4 100644
--- a/drivers/staging/hv/utils.h
+++ b/drivers/staging/hv/utils.h
@@ -98,6 +98,10 @@ struct ictimesync_data{
 	u8 flags;
 } __attribute__((packed));
 
+
+/* Number of IC types supported */
+#define MAX_MSG_TYPES	3
+
 /* Index for each IC struct in array hv_cb_utils[] */
 #define HV_SHUTDOWN_MSG		0
 #define HV_TIMESYNC_MSG		1
@@ -115,5 +119,6 @@ extern void prep_negotiate_resp(struct icmsg_hdr *,
 				struct icmsg_negotiate *, u8 *);
 extern void chn_cb_negotiate(void *);
 extern struct hyperv_service_callback hv_cb_utils[];
+extern atomic_t hv_utils_initcnt;
 
 #endif /* __HV_UTILS_H_ */
-- 
1.6.3.2


[-- Attachment #3: Type: text/plain, Size: 184 bytes --]

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization

^ permalink raw reply related

* [PULL] virtio
From: Rusty Russell @ 2010-05-19 12:53 UTC (permalink / raw)
  To: Linus Torvalds; +Cc: linux-kernel, virtualization

The following changes since commit 537b60d17894b7c19a6060feae40299d7109d6e7:
  Linus Torvalds (1):
        Merge branch 'x86-uv-for-linus' of git://git.kernel.org/.../tip/linux-2.6-tip

are available in the git repository at:

  ssh://master.kernel.org/pub/scm/linux/kernel/git/rusty/linux-2.6-for-linus.git virtio

Amit Shah (14):
      virtio: Revert "virtio: disable multiport console support."
      virtio: console: Add a __send_control_msg() that can send messages without a valid port
      virtio: console: Let host know of port or device add failures
      virtio: console: Return -EPIPE to hvc_console if we lost the connection
      virtio: console: Don't call hvc_remove() on unplugging console ports
      virtio: console: Remove config work handler
      virtio: console: Move code around for future patches
      virtio: console: Use a control message to add ports
      virtio: console: Don't always create a port 0 if using multiport
      virtio: console: Rename wait_is_over() to will_read_block()
      virtio: console: Add support for nonblocking write()s
      virtio: console: Resize console port 0 on config intr only if multiport is off
      virtio: console: Store each console's size in the console structure
      virtio: console: Accept console size along with resize control message

Julia Lawall (1):
      drivers/char: Eliminate use after free

Michael S. Tsirkin (9):
      virtio: add virtqueue_ vq_ops wrappers
      virtio_balloon: use virtqueue_xxx wrappers
      virtio_console: use virtqueue_xxx wrappers
      virtio_blk: use virtqueue_xxx wrappers
      virtio_net: use virtqueue_xxx wrappers
      virtio_ring: remove a level of indirection
      virtio-rng: use virtqueue_xxx wrappers
      trans_virtio: use virtqueue_xxx wrappers
      virtio: add_buf_gfp

Rusty Russell (1):
      virtio_blk: remove multichar constant.

john cooper (2):
      Add virtio disk identification support
      Add virtio disk identification ioctl

 drivers/block/virtio_blk.c          |   46 +++-
 drivers/char/hw_random/virtio-rng.c |    6 +-
 drivers/char/virtio_console.c       |  700 +++++++++++++++++++----------------
 drivers/net/virtio_net.c            |   46 ++--
 drivers/virtio/virtio_balloon.c     |   17 +-
 drivers/virtio/virtio_ring.c        |   44 +--
 include/linux/virtio.h              |   55 ++--
 include/linux/virtio_blk.h          |    5 +
 include/linux/virtio_console.h      |   25 ++
 net/9p/trans_virtio.c               |    6 +-
 10 files changed, 541 insertions(+), 409 deletions(-)

^ permalink raw reply

* Re: [Qemu-devel] [PATCH RFC] virtio: put last seen used index into ring itself
From: Avi Kivity @ 2010-05-19  8:06 UTC (permalink / raw)
  To: Rusty Russell
  Cc: qemu-devel, virtualization, linux-kernel, kvm, Michael S. Tsirkin
In-Reply-To: <201005191709.16401.rusty@rustcorp.com.au>

On 05/19/2010 10:39 AM, Rusty Russell wrote:
>
> I think we're talking about the last 2 entries of the avail ring.  That means
> the worst case is 1 false bounce every time around the ring.

It's low, but why introduce an inefficiency when you can avoid doing it 
for the same effort?

> I think that's
> why we're debating it instead of measuring it :)
>    

Measure before optimize is good for code but not for protocols.  
Protocols have to be robust against future changes.  Virtio is warty 
enough already, we can't keep doing local optimizations.

> Note that this is a exclusive->shared->exclusive bounce only, too.
>    

A bounce is a bounce.

Virtio is already way too bouncy due to the indirection between the 
avail/used rings and the descriptor pool.  A device with out of order 
completion (like virtio-blk) will quickly randomize the unused 
descriptor indexes, so every descriptor fetch will require a bounce.

In contrast, if the rings hold the descriptors themselves instead of 
pointers, we bounce (sizeof(descriptor)/cache_line_size) cache lines for 
every descriptor, amortized.

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.

^ permalink raw reply

* Re: [Qemu-devel] [PATCH RFC] virtio: put last seen used index into ring itself
From: Rusty Russell @ 2010-05-19  7:39 UTC (permalink / raw)
  To: Avi Kivity
  Cc: qemu-devel, virtualization, linux-kernel, kvm, Michael S. Tsirkin
In-Reply-To: <4BE9AF9A.8080005@redhat.com>

On Wed, 12 May 2010 04:57:22 am Avi Kivity wrote:
> On 05/07/2010 06:23 AM, Rusty Russell wrote:
> > On Thu, 6 May 2010 07:30:00 pm Avi Kivity wrote:
> >    
> >> On 05/05/2010 11:58 PM, Michael S. Tsirkin wrote:
> >>      
> >>> +	/* We publish the last-seen used index at the end of the available ring.
> >>> +	 * It is at the end for backwards compatibility. */
> >>> +	vr->last_used_idx =&(vr)->avail->ring[num];
> >>> +	/* Verify that last used index does not spill over the used ring. */
> >>> +	BUG_ON((void *)vr->last_used_idx +
> >>> +	       sizeof *vr->last_used_idx>   (void *)vr->used);
> >>>    }
> >>>
> >>>        
> >> Shouldn't this be on its own cache line?
> >>      
> > It's next to the available ring; because that's where the guest publishes
> > its data.  That whole page is guest-write, host-read.
> >
> > Putting it on a cacheline by itself would be a slight pessimization; the host
> > cpu would have to get the last_used_idx cacheline and the avail descriptor
> > cacheline every time.  This way, they are sometimes the same cacheline.
> 
> If one peer writes the tail of the available ring, while the other reads 
> last_used_idx, it's a false bounce, no?

I think we're talking about the last 2 entries of the avail ring.  That means
the worst case is 1 false bounce every time around the ring.  I think that's
why we're debating it instead of measuring it :)

Note that this is a exclusive->shared->exclusive bounce only, too.

Cheers,
Rusty.

^ permalink raw reply

* Re: virtio: imply disable_cb on callbacks
From: Rusty Russell @ 2010-05-19  4:37 UTC (permalink / raw)
  To: Michael S. Tsirkin; +Cc: kvm, virtualization
In-Reply-To: <20100518113316.GA26757@redhat.com>

On Tue, 18 May 2010 09:03:17 pm Michael S. Tsirkin wrote:
> Rusty, the patch "virtio: imply disable_cb on callbacks" is on your tree.
> I'd like to figure out how it works: for example:
> 
> diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
> --- a/drivers/block/virtio_blk.c
> +++ b/drivers/block/virtio_blk.c
> @@ -69,6 +69,8 @@ static void blk_done(struct virtqueue *v
>         /* In case queue is stopped waiting for more buffers. */
>         blk_start_queue(vblk->disk->queue);
>         spin_unlock_irqrestore(&vblk->lock, flags);
> +
> +       vq->vq_ops->enable_cb(vq);
>  }
> 
>  static bool do_req(struct request_queue *q, struct virtio_blk *vblk,
> 
> 
> Since this does not check the return status from enable_cb,
> it seems we could loose an interrupt if it arrives
> between poll and callback enable?

It's been quite a while since I wrote this patch, and never really liked it
enough to polish it.

We would need to enable this *before* reading the queue, AFAICT.

I'll remove it from my series; it's in the wilderness area already :)

Thanks!
Rusty.

^ permalink raw reply

* Re: [PATCHv2] vhost-net: utilize PUBLISH_USED_IDX feature
From: Avi Kivity @ 2010-05-18 17:47 UTC (permalink / raw)
  To: Michael S. Tsirkin
  Cc: davem, Juan Quintela, Rusty Russell, Paul E. McKenney,
	Arnd Bergmann, kvm, virtualization, netdev, linux-kernel,
	alex.williamson, amit.shah
In-Reply-To: <20100518022105.GA23129@redhat.com>

On 05/18/2010 05:21 AM, Michael S. Tsirkin wrote:
> With PUBLISH_USED_IDX, guest tells us which used entries
> it has consumed. This can be used to reduce the number
> of interrupts: after we write a used entry, if the guest has not yet
> consumed the previous entry, or if the guest has already consumed the
> new entry, we do not need to interrupt.
> This imporves bandwidth by 30% under some workflows.
>    

Seems to be missing the cacheline alignment.

Rusty's clarification did not satisfy me, I think it's needed.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

^ permalink raw reply

* CFP: 2nd International Conference on Cloud Computing (CloudComp2010)
From: Ming Zhao @ 2010-05-18 14:08 UTC (permalink / raw)


[Apologies if you receive multiple copies of this announcement]



Reminder:
-------

* Paper submission deadline: May 31, 2010





========================================================================
                            Call for Papers
           The 2nd International Conference on Cloud Computing
                            (CloudComp2010)

                 Barcelona, Spain, October 25 - 28, 2010
========================================================================

Conference Scope:
-----------------

Cloud Computing is an emerging computing paradigm envisioned to change
all IT landscape facets including technology, business, services and
human resources. It is a consumer/delivery model that offers IT
capabilities as services, billed based on usage. Many such cloud
services can be envisioned, but the main ones are IaaS (Infrastructure-
as-a-Service), PaaS (Platform-as-a-Service), and SaaS (Software-as-a-
Service). The underlying cloud architecture includes a pool of
virtualized compute, storage and networking resources that can be
aggregated and launched as platforms to run workloads and satisfy their
Service-Level Agreement (SLA). Cloud architectures also include
provisions to best guarantee service delivery for clients and at the
same time optimize efficiency of resources of providers. Examples of
provisions include, but not limited to, elasticity through scaling
resources up/down to track workload behavior, extensive monitoring,
failure mitigation, and energy optimizations. The two main technologies
enabling clouds are: (i) Virtualization, the foundation of clouds; and
(ii) Manageability (autonomics), the command and control of clouds.

CloudComp is intended to bring together researchers, developers, and
industry professionals to discuss clouds, cloud computing and related
ecosystems support. To that end, papers are solicited from all cloud
related areas, including, but not limited to:

1. Cloud architectures and provisions to optimize providers'
    environments while guaranteeing clients' SLAs.
2. Programming models, applications and middleware suitable for dynamic
    cloud environments.
3. End-to-end techniques for autonomic management of cloud resources
    including monitoring, asset management, process automation and
    others.
4. New cloud delivery models, models' optimizations and associated
    architectural changes.
5. New cloud economic and billing models.
6. Cloud security, privacy and compliance challenges.
7. Toolkits, frameworks and processes to enable clouds and allow
    seamless transitions from traditional IT environments to clouds.
8. Experiences with existing cloud infrastructure, services and uses.
9. Novel human interfaces and browsers for accessing clouds.
10.Interaction of mobile computing, mCommerce and Clouds.

Cloud Computing conference is sponsored by ICST (Institute for Computer
Sciences, Social-Informatics and Telecommunications Engineering).



Important Dates:
----------------

* Paper submission: May 31, 2010
* Authors' notification: July 26, 2010
* Final manuscripts: August 23, 2010



Organization:
-------------

* General Chair:
  o Mazin Yousif, IBM Corporation, Canada


* Program Chairs:
  o Burkhard Neidecker-Lutz, SAP Research, Germany
  o Christine Morin, INRIA, France


* Program Committee:
  o Albert Zomaya, University of Sydney, Australia
  o Cristiana Amza, University of Toronto, Canada
  o Daniel Mosse, University of Pittsburgh, USA
  o Dongyan Xu, Purdue University, USA
  o Erol Gelenbe, Imperial College, UK
  o Franco Zambonelli, Universita di Modena e Reggio Emilia, Italy
  o Frederic Desprez, INRIA
  o George Galambos, IBM Canada, Canada
  o Giovanna Di Marzo Serugendo, University of London, UK
  o Guillaume Pierre, Vrije Universiteit Amsterdam
  o Ignacio Llorente, Universidad Complutense de Madrid
  o Ivona Brandic, Vienna University of Technology, Austria
  o Jose Antonio Lozano, Telefonica I+D, Spain
  o Jose Bernabeu-Auban, Microsoft, USA
  o Juergen Falkner, Fraunhofer, Germany
  o Julie McCann, Imperial College, UK
  o Karsten Schwan, Georgia Institute of Technology, USA
  o Keith Jeffery, Rutherford Appleton Laboratory, UK
  o Mikhail Smirnov, Fraunhofer, Germany
  o Omar Rana, Cardiff University, UK
  o Ozalp Babaoglu, University of Bologna, Italy
  o Rajkumar Buyya, Manjrasoft, Australia
  o Renato Figueiredo, University of Florida, USA
  o Ricardo Bianchini, Rutgers University, USA
  o Ron Brightwell, Sandia National Labs, USA
  o Simon Dobson, University of St Andrews, UK
  o Tamer Aboualy, IBM Corporation, Canada
  o Wu Feng, Virginia Tech University, USA


* Demos/Exhibits Chair:
  o Katerina Goulioutkina, IBM Corporation, Canada

* Workshop/Tutorial Chair:
  o Simon Dobson, University of St Andrews, UK

* CloudComp Summit Chair:
  o Duncan Johnston-Watt, Cloudsoft Corporation, UK

* Publicity Chair:
  o Ming Zhao, Florida International University, USA

* Local Arrangements Chair:
  o Joan Serrat, UPC, Barcelona, Spain

* Industry Session Chair:
  o Duncan Johnston-Watt, Cloudsoft Corporation, UK



Further Information:
--------------------

Web: http://www.cloudcomp.eu
Email: cloudcomp2010@icst.org






-- 
Ming Zhao, Assistant Professor
School of Computing and Information Sciences
Florida International University
Tel: (305) 348-2034, Fax: (305) 348-3549
Web: http://www.cis.fiu.edu/~zhaom

^ permalink raw reply

* Re: [PATCH RFC] virtio_blk: Use blk-iopoll for host->guest notify
From: Jens Axboe @ 2010-05-18 13:07 UTC (permalink / raw)
  To: Stefan Hajnoczi; +Cc: kvm, qemu-devel, linux-kernel, virtualization
In-Reply-To: <20100518080705.GA2467@stefan-thinkpad.transitives.com>

On Tue, May 18 2010, Stefan Hajnoczi wrote:
> On Fri, May 14, 2010 at 05:30:56PM -0500, Brian Jackson wrote:
> > Any preliminary numbers? latency, throughput, cpu use? What about comparing 
> > different "weights"?
> 
> I am running benchmarks and will report results when they are in.

I'm very interested as well, I have been hoping for some more adoption
of this. I have mptsas and mpt2sas patches pending as well.

I have not done enough and fully exhaustive weight analysis, so note me
down for wanting such an analysis on virtio_blk as well.

-- 
Jens Axboe

^ permalink raw reply

* Re: virtio: imply disable_cb on callbacks
From: Michael S. Tsirkin @ 2010-05-18 11:33 UTC (permalink / raw)
  To: Rusty Russell; +Cc: kvm, virtualization

Rusty, the patch "virtio: imply disable_cb on callbacks" is on your tree.
I'd like to figure out how it works: for example:

diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -69,6 +69,8 @@ static void blk_done(struct virtqueue *v
        /* In case queue is stopped waiting for more buffers. */
        blk_start_queue(vblk->disk->queue);
        spin_unlock_irqrestore(&vblk->lock, flags);
+
+       vq->vq_ops->enable_cb(vq);
 }

 static bool do_req(struct request_queue *q, struct virtio_blk *vblk,


Since this does not check the return status from enable_cb,
it seems we could loose an interrupt if it arrives
between poll and callback enable?

Same might apply to other devices.

Thanks,

-- 
MST

^ permalink raw reply

* Re: [PATCHv2] vhost-net: utilize PUBLISH_USED_IDX feature
From: Juan Quintela @ 2010-05-18 11:31 UTC (permalink / raw)
  To: Michael S. Tsirkin
  Cc: davem, Rusty Russell, Paul E. McKenney, Arnd Bergmann, kvm,
	virtualization, netdev, linux-kernel, alex.williamson, amit.shah
In-Reply-To: <20100518022105.GA23129@redhat.com>

"Michael S. Tsirkin" <mst@redhat.com> wrote:
> With PUBLISH_USED_IDX, guest tells us which used entries
> it has consumed. This can be used to reduce the number
> of interrupts: after we write a used entry, if the guest has not yet
> consumed the previous entry, or if the guest has already consumed the
> new entry, we do not need to interrupt.
> This imporves bandwidth by 30% under some workflows.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>
> This is on top of Rusty's tree and depends on the virtio patch.
>
> Changes from v1:
> fix build

Minor nit if you have to respin it:

>  /* This actually signals the guest, using eventfd. */
> -void vhost_signal(struct vhost_dev *dev, struct vhost_virtqueue *vq)
> +static void vhost_signal(struct vhost_dev *dev, struct vhost_virtqueue *vq)
>  {
>  	__u16 flags;
> +	__u16 used;

local var named "used" here

>  	/* Flush out used index updates. This is paired
>  	 * with the barrier that the Guest executes when enabling
>  	 * interrupts. */
> @@ -1053,6 +1057,17 @@ void vhost_signal(struct vhost_dev *dev, struct vhost_virtqueue *vq)
>  	     !vhost_has_feature(dev, VIRTIO_F_NOTIFY_ON_EMPTY)))
>  		return;
>  
> +	if (vhost_has_feature(dev, VIRTIO_RING_F_PUBLISH_USED)) {
> +		__u16 used;

and a "more" local one also named "used" :)

I guess you want to remove the previous declaration, as var is only used
in this block.

> +		if (get_user(used, vq->last_used)) {
> +			vq_err(vq, "Failed to get last used idx");
> +			return;
> +		}
> +
> +		if (used != (u16)(vq->last_used_idx - 1))
> +			return;
> +	}
> +
>  	/* Signal the Guest tell them we used something up. */
>  	if (vq->call_ctx)
>  		eventfd_signal(vq->call_ctx, 1);

Rest of patch looks ok, and as a bonus un-export two functions.

Later, Juan.

^ permalink raw reply

* Re: [PATCH RFC] virtio_blk: Use blk-iopoll for host->guest notify
From: Stefan Hajnoczi @ 2010-05-18  8:07 UTC (permalink / raw)
  To: Brian Jackson; +Cc: kvm, qemu-devel, linux-kernel, Jens Axboe, virtualization
In-Reply-To: <201005141730.57158.iggy@theiggy.com>

On Fri, May 14, 2010 at 05:30:56PM -0500, Brian Jackson wrote:
> Any preliminary numbers? latency, throughput, cpu use? What about comparing 
> different "weights"?

I am running benchmarks and will report results when they are in.

Stefan

^ permalink raw reply

* Re: [PATCH] fix a code style in drivers/char/virtio_console.c
From: Steven Liu @ 2010-05-18  6:41 UTC (permalink / raw)
  To: Amit Shah; +Cc: linux-kernel, virtualization
In-Reply-To: <20100518063241.GB27262@amit-laptop.redhat.com>

okay, i see, thanks for your regards :)

2010/5/18 Amit Shah <amit.shah@redhat.com>:
> Hello,
>
> On (Tue) May 18 2010 [13:41:29], Steven Liu wrote:
>> Hi, Amit,
>>
>>     if 'err' initialised in this path, it needn't do err = -ENOMEM
>> after,isn't it?
>
> What I mean is if we later add some code that just does:
>
>        if (err)
>                goto fail;
>
> then 'ret' can be -ENOMEM, as it was initialised to, which would be
> fine. But if a later patch adds something like:
>
> +       ret = -EIO;
> +       err = ...
> +       if (err)
> +               goto fail;
> +
>        err = ...
>        if (err)
>                goto fail;
>
> In this case, the 2nd if() would now return EIO instead of ENOMEM as
> earlier.
>
> Also, this style of coding can prevent uninitialised usage of 'ret', eg:
>
>
>        int ret;
>
>        if (err)
>                goto fail;
>
> fail:
>        return ret;
>
> In this case, the compiler will warn about 'ret' being used
> uninitialised.
>
> This is just a coding style issue. I had initially coded it the way your
> patch does, but Rusty asked me to change that and I like this new style
> better: there's less scope for surprises.
>
>                Amit
>

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox