find_vqs operation starting at arbitrary index

find_vqs operation starting at arbitrary index

[Amit Shah]

Hello,

The recent find_vqs operation doesn't allow for a vq to be found at an
arbitrary location; it's meant to be called once at startup to find all
possible queues and never called again.

This doesn't work for devices which can have queues hot-plugged at
run-time. This can be made to work by passing the 'start_index' value as
was done earlier for find_vq, but I doubt something like the following
will work. The MSI vectors might need some changing as well.

If this indeed is the right way to do it, I can add a

virtio_find_vqs helper along similar lines as virtio_find_single_vq and
pass '0' as the start index to the ->find_vqs operation.

Or is there another way to do it?

diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
index 193c8f0..cb3f8df 100644
--- a/drivers/virtio/virtio_pci.c
+++ b/drivers/virtio/virtio_pci.c
@@ -499,7 +499,7 @@ static void vp_del_vqs(struct virtio_device *vdev)
 
 /* the config->find_vqs() implementation */
 static int vp_find_vqs(struct virtio_device *vdev, unsigned nvqs,
-		       struct virtqueue *vqs[],
+		       unsigned start_index, struct virtqueue *vqs[],
 		       vq_callback_t *callbacks[],
 		       const char *names[])
 {
@@ -516,7 +516,8 @@ static int vp_find_vqs(struct virtio_device *vdev, unsigned nvqs,
 		goto error_request;
 
 	for (i = 0; i < nvqs; ++i) {
-		vqs[i] = vp_find_vq(vdev, i, callbacks[i], names[i]);
+		vqs[i] = vp_find_vq(vdev, start_index + i, callbacks[i],
+				    names[i]);
 		if (IS_ERR(vqs[i]))
 			goto error_find;
 	}



		Amit

Re: find_vqs operation starting at arbitrary index

[Michael S. Tsirkin]

On Mon, Jun 01, 2009 at 01:33:48PM +0530, Amit Shah wrote:
> Hello,
> 
> The recent find_vqs operation doesn't allow for a vq to be found at an
> arbitrary location; it's meant to be called once at startup to find all
> possible queues and never called again.
> 
> This doesn't work for devices which can have queues hot-plugged at
> run-time. This can be made to work by passing the 'start_index' value as
> was done earlier for find_vq, but I doubt something like the following
> will work. The MSI vectors might need some changing as well.

How, specifically?

> If this indeed is the right way to do it, I can add a
> 
> virtio_find_vqs helper along similar lines as virtio_find_single_vq and
> pass '0' as the start index to the ->find_vqs operation.
> 
> Or is there another way to do it?
> 
> diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
> index 193c8f0..cb3f8df 100644
> --- a/drivers/virtio/virtio_pci.c
> +++ b/drivers/virtio/virtio_pci.c
> @@ -499,7 +499,7 @@ static void vp_del_vqs(struct virtio_device *vdev)
>  
>  /* the config->find_vqs() implementation */
>  static int vp_find_vqs(struct virtio_device *vdev, unsigned nvqs,
> -		       struct virtqueue *vqs[],
> +		       unsigned start_index, struct virtqueue *vqs[],
>  		       vq_callback_t *callbacks[],
>  		       const char *names[])
>  {
> @@ -516,7 +516,8 @@ static int vp_find_vqs(struct virtio_device *vdev, unsigned nvqs,
>  		goto error_request;
>  
>  	for (i = 0; i < nvqs; ++i) {
> -		vqs[i] = vp_find_vq(vdev, i, callbacks[i], names[i]);
> +		vqs[i] = vp_find_vq(vdev, start_index + i, callbacks[i],
> +				    names[i]);
>  		if (IS_ERR(vqs[i]))
>  			goto error_find;
>  	}
> 
> 
> 
> 		Amit

Re: find_vqs operation starting at arbitrary index

[Amit Shah]

On (Mon) Jun 01 2009 [11:11:06], Michael S. Tsirkin wrote:
> On Mon, Jun 01, 2009 at 01:33:48PM +0530, Amit Shah wrote:
> > Hello,
> > 
> > The recent find_vqs operation doesn't allow for a vq to be found at an
> > arbitrary location; it's meant to be called once at startup to find all
> > possible queues and never called again.
> > 
> > This doesn't work for devices which can have queues hot-plugged at
> > run-time. This can be made to work by passing the 'start_index' value as
> > was done earlier for find_vq, but I doubt something like the following
> > will work. The MSI vectors might need some changing as well.
> 
> How, specifically?

I'm not sure; I was wanting to know if they will. I suspect this piece
of code though:

in vp_find_vqs, just before calling vp_find_vq:

	/* How many vectors would we like? */
	for (i = 0; i < nvqs; ++i)
		if (callbacks[i])
			++vectors;

	err = vp_request_vectors(vdev, vectors);
	if (err)
		goto error_request;

Will any adjusting be needed for the 'vectors' argument (since it's
considered to be the max value one can specify)?

		Amit

Re: find_vqs operation starting at arbitrary index

[Michael S. Tsirkin]

On Mon, Jun 01, 2009 at 02:05:10PM +0530, Amit Shah wrote:
> On (Mon) Jun 01 2009 [11:11:06], Michael S. Tsirkin wrote:
> > On Mon, Jun 01, 2009 at 01:33:48PM +0530, Amit Shah wrote:
> > > Hello,
> > > 
> > > The recent find_vqs operation doesn't allow for a vq to be found at an
> > > arbitrary location; it's meant to be called once at startup to find all
> > > possible queues and never called again.
> > > 
> > > This doesn't work for devices which can have queues hot-plugged at
> > > run-time. This can be made to work by passing the 'start_index' value as
> > > was done earlier for find_vq, but I doubt something like the following
> > > will work. The MSI vectors might need some changing as well.
> > 
> > How, specifically?
> 
> I'm not sure; I was wanting to know if they will.

Yes, probably.

> I suspect this piece
> of code though:
> 
> in vp_find_vqs, just before calling vp_find_vq:
> 
> 	/* How many vectors would we like? */
> 	for (i = 0; i < nvqs; ++i)
> 		if (callbacks[i])
> 			++vectors;
> 
> 	err = vp_request_vectors(vdev, vectors);
> 	if (err)
> 		goto error_request;
> 
> Will any adjusting be needed for the 'vectors' argument (since it's
> considered to be the max value one can specify)?
> 
> 		Amit

Right. And it can only be called once. And something'll have to be done
on cleanup as well.

-- 
MST

Re: find_vqs operation starting at arbitrary index

[Amit Shah]

On (Mon) Jun 01 2009 [11:43:27], Michael S. Tsirkin wrote:
> On Mon, Jun 01, 2009 at 02:05:10PM +0530, Amit Shah wrote:
> > On (Mon) Jun 01 2009 [11:11:06], Michael S. Tsirkin wrote:
> > > On Mon, Jun 01, 2009 at 01:33:48PM +0530, Amit Shah wrote:
> > > > Hello,
> > > > 
> > > > The recent find_vqs operation doesn't allow for a vq to be found at an
> > > > arbitrary location; it's meant to be called once at startup to find all
> > > > possible queues and never called again.
> > > > 
> > > > This doesn't work for devices which can have queues hot-plugged at
> > > > run-time. This can be made to work by passing the 'start_index' value as
> > > > was done earlier for find_vq, but I doubt something like the following
> > > > will work. The MSI vectors might need some changing as well.
> > > 
> > > How, specifically?
> > 
> > I'm not sure; I was wanting to know if they will.
> 
> Yes, probably.
> 
> > I suspect this piece
> > of code though:
> > 
> > in vp_find_vqs, just before calling vp_find_vq:
> > 
> > 	/* How many vectors would we like? */
> > 	for (i = 0; i < nvqs; ++i)
> > 		if (callbacks[i])
> > 			++vectors;
> > 
> > 	err = vp_request_vectors(vdev, vectors);
> > 	if (err)
> > 		goto error_request;
> > 
> > Will any adjusting be needed for the 'vectors' argument (since it's
> > considered to be the max value one can specify)?
> > 
> > 		Amit
> 
> Right. And it can only be called once. And something'll have to be done
> on cleanup as well.

Yes; that's the assumption find_vqs currently makes. I'll take a stab at
reworking this code.

		Amit

Re: find_vqs operation starting at arbitrary index

[Michael S. Tsirkin]

On Mon, Jun 01, 2009 at 02:21:28PM +0530, Amit Shah wrote:
> On (Mon) Jun 01 2009 [11:43:27], Michael S. Tsirkin wrote:
> > On Mon, Jun 01, 2009 at 02:05:10PM +0530, Amit Shah wrote:
> > > On (Mon) Jun 01 2009 [11:11:06], Michael S. Tsirkin wrote:
> > > > On Mon, Jun 01, 2009 at 01:33:48PM +0530, Amit Shah wrote:
> > > > > Hello,
> > > > > 
> > > > > The recent find_vqs operation doesn't allow for a vq to be found at an
> > > > > arbitrary location; it's meant to be called once at startup to find all
> > > > > possible queues and never called again.
> > > > > 
> > > > > This doesn't work for devices which can have queues hot-plugged at
> > > > > run-time. This can be made to work by passing the 'start_index' value as
> > > > > was done earlier for find_vq, but I doubt something like the following
> > > > > will work. The MSI vectors might need some changing as well.
> > > > 
> > > > How, specifically?
> > > 
> > > I'm not sure; I was wanting to know if they will.
> > 
> > Yes, probably.
> > 
> > > I suspect this piece
> > > of code though:
> > > 
> > > in vp_find_vqs, just before calling vp_find_vq:
> > > 
> > > 	/* How many vectors would we like? */
> > > 	for (i = 0; i < nvqs; ++i)
> > > 		if (callbacks[i])
> > > 			++vectors;
> > > 
> > > 	err = vp_request_vectors(vdev, vectors);
> > > 	if (err)
> > > 		goto error_request;
> > > 
> > > Will any adjusting be needed for the 'vectors' argument (since it's
> > > considered to be the max value one can specify)?
> > > 
> > > 		Amit
> > 
> > Right. And it can only be called once. And something'll have to be done
> > on cleanup as well.
> 
> Yes; that's the assumption find_vqs currently makes.

The reason really is with pci_enable_msix which is the underlying
limitation here.

> I'll take a stab at reworking this code.
> 
> 		Amit

Re: find_vqs operation starting at arbitrary index

[Michael S. Tsirkin]

On Mon, Jun 01, 2009 at 02:21:28PM +0530, Amit Shah wrote:
> On (Mon) Jun 01 2009 [11:43:27], Michael S. Tsirkin wrote:
> > On Mon, Jun 01, 2009 at 02:05:10PM +0530, Amit Shah wrote:
> > > On (Mon) Jun 01 2009 [11:11:06], Michael S. Tsirkin wrote:
> > > > On Mon, Jun 01, 2009 at 01:33:48PM +0530, Amit Shah wrote:
> > > > > Hello,
> > > > > 
> > > > > The recent find_vqs operation doesn't allow for a vq to be found at an
> > > > > arbitrary location; it's meant to be called once at startup to find all
> > > > > possible queues and never called again.
> > > > > 
> > > > > This doesn't work for devices which can have queues hot-plugged at
> > > > > run-time. This can be made to work by passing the 'start_index' value as
> > > > > was done earlier for find_vq, but I doubt something like the following
> > > > > will work. The MSI vectors might need some changing as well.

Another possible approach would be to add enable_vq/disable_vq operations
(or possibly resize_vq).
Thus you would create an empty queue (size 0) upfront, and associate it with
a vector, and then enable it/set the real size when you really want to use it.
disable/resize to 0 when you don't need it anymore.


Actually, resizing queues might come in handy for virtio_net:
I think tx queue length might be tunable by user,
and maybe we can even auto-tune rx queue size to avoid packet loss.

Re: find_vqs operation starting at arbitrary index

[Rusty Russell]

On Mon, 1 Jun 2009 05:33:48 pm Amit Shah wrote:
> Hello,
>
> The recent find_vqs operation doesn't allow for a vq to be found at an
> arbitrary location; it's meant to be called once at startup to find all
> possible queues and never called again.
>
> This doesn't work for devices which can have queues hot-plugged at
> run-time. This can be made to work by passing the 'start_index' value as
> was done earlier for find_vq, but I doubt something like the following
> will work. The MSI vectors might need some changing as well.

There's a fundamental conflict here: find_vqs was added so the PCI MSI code 
knows exactly how many virtqueues there are.

So you'll need to sort this out with Michael...

Thanks,
Rusty.

Re: find_vqs operation starting at arbitrary index

[Michael S. Tsirkin]

On Tue, Jun 02, 2009 at 08:53:07AM +0930, Rusty Russell wrote:
> On Mon, 1 Jun 2009 05:33:48 pm Amit Shah wrote:
> > Hello,
> >
> > The recent find_vqs operation doesn't allow for a vq to be found at an
> > arbitrary location; it's meant to be called once at startup to find all
> > possible queues and never called again.
> >
> > This doesn't work for devices which can have queues hot-plugged at
> > run-time. This can be made to work by passing the 'start_index' value as
> > was done earlier for find_vq, but I doubt something like the following
> > will work. The MSI vectors might need some changing as well.
> 
> There's a fundamental conflict here: find_vqs was added so the PCI MSI code 
> knows exactly how many virtqueues there are.
> 
> So you'll need to sort this out with Michael...
> 
> Thanks,
> Rusty.

I'd like to understand the expected usage some more.  If there's still a
small # of hotpluggable queues known in advance, you can request all
vectors but have the queues disabled, and then enable as queues arrive.
In that case, something like resize_queue or enable_queue might be a
good idea.

OTOH, if you want to have a potentially unlimited number of queues,
you will have to allocate a common vector for all of them
as on intel we are limited to 256 total vectors for all devices
taken together.  Not sure what a good API for that would be.

-- 
MST

Re: find_vqs operation starting at arbitrary index

[Amit Shah]

On (Tue) Jun 02 2009 [19:32:27], Michael S. Tsirkin wrote:
> On Tue, Jun 02, 2009 at 08:53:07AM +0930, Rusty Russell wrote:
> > On Mon, 1 Jun 2009 05:33:48 pm Amit Shah wrote:
> > > Hello,
> > >
> > > The recent find_vqs operation doesn't allow for a vq to be found at an
> > > arbitrary location; it's meant to be called once at startup to find all
> > > possible queues and never called again.
> > >
> > > This doesn't work for devices which can have queues hot-plugged at
> > > run-time. This can be made to work by passing the 'start_index' value as
> > > was done earlier for find_vq, but I doubt something like the following
> > > will work. The MSI vectors might need some changing as well.
> > 
> > There's a fundamental conflict here: find_vqs was added so the PCI MSI code 
> > knows exactly how many virtqueues there are.
> > 
> > So you'll need to sort this out with Michael...
> > 
> > Thanks,
> > Rusty.
> 
> I'd like to understand the expected usage some more.  If there's still a
> small # of hotpluggable queues known in advance, you can request all
> vectors but have the queues disabled, and then enable as queues arrive.
> In that case, something like resize_queue or enable_queue might be a
> good idea.

Yes, it sounds like a good idea and I was debating this exact question
-- how many queues should I expect. I currently don't expect more than
20 but even that sounds like a largish number.

> OTOH, if you want to have a potentially unlimited number of queues,
> you will have to allocate a common vector for all of them
> as on intel we are limited to 256 total vectors for all devices
> taken together.  Not sure what a good API for that would be.

A common vector for all the queues in the device, right? And will that
also mean each queue gets woken up on any activity on any one of them?

		Amit

Re: find_vqs operation starting at arbitrary index

[Michael S. Tsirkin]

On Tue, Jun 02, 2009 at 10:39:16PM +0530, Amit Shah wrote:
> On (Tue) Jun 02 2009 [19:32:27], Michael S. Tsirkin wrote:
> > On Tue, Jun 02, 2009 at 08:53:07AM +0930, Rusty Russell wrote:
> > > On Mon, 1 Jun 2009 05:33:48 pm Amit Shah wrote:
> > > > Hello,
> > > >
> > > > The recent find_vqs operation doesn't allow for a vq to be found at an
> > > > arbitrary location; it's meant to be called once at startup to find all
> > > > possible queues and never called again.
> > > >
> > > > This doesn't work for devices which can have queues hot-plugged at
> > > > run-time. This can be made to work by passing the 'start_index' value as
> > > > was done earlier for find_vq, but I doubt something like the following
> > > > will work. The MSI vectors might need some changing as well.
> > > 
> > > There's a fundamental conflict here: find_vqs was added so the PCI MSI code 
> > > knows exactly how many virtqueues there are.
> > > 
> > > So you'll need to sort this out with Michael...
> > > 
> > > Thanks,
> > > Rusty.
> > 
> > I'd like to understand the expected usage some more.  If there's still a
> > small # of hotpluggable queues known in advance, you can request all
> > vectors but have the queues disabled, and then enable as queues arrive.
> > In that case, something like resize_queue or enable_queue might be a
> > good idea.
> 
> Yes, it sounds like a good idea and I was debating this exact question
> -- how many queues should I expect. I currently don't expect more than
> 20 but even that sounds like a largish number.
> 
> > OTOH, if you want to have a potentially unlimited number of queues,
> > you will have to allocate a common vector for all of them
> > as on intel we are limited to 256 total vectors for all devices
> > taken together.  Not sure what a good API for that would be.
> 
> A common vector for all the queues in the device, right? And will that
> also mean each queue gets woken up on any activity on any one of them?
> 
> 		Amit

Yes. This is what find_vqs currently does if it can't allocate a queue
per VQ (actually, per queue with callback: your queues do have
callbacks?).

Of course we can start contemplating more complicated options
for mapping queues to vectors, and the guest/host ABI
supports this.

-- 
MST