Re: [PATCH] virtio-pci: also bind to Amazon PCI vendor ID
On Sun, Sep 14, 2014 at 08:29:33PM -0700, Anthony Liguori wrote:
> From: Anthony Liguori
>
> See https://issues.oasis-open.org/browse/VIRTIO-16 although it
> was prematurely closed.
The reason it was closed is described in the comments. and I quote:
" I think anyone can use a different subsystem vendor id and whql the
driver. virtio-102 will make this even easier, by making the subsystem
id flexible. Let's close this and re-open if someone tries to do this
and runs into a problem. "
Look here for example:
https://github.com/YanVugenfirer/kvm-guest-drivers-windows/blob/master/NetKVM/NDIS5/wxp/netkvm.inf
Replace SUBSYS_00011AF4 with SUBSYS_00011D0F, and you will get
a virtio-net driver that (I think) you should be able to WHQL.
On the host side, you will need a QEMU patch to allow libvirt control of
the subsystem vendor ID.
All this while all Linux guests will keep working without changes,
which seems like a better approach.
Looking on the web, I found:
http://download.microsoft.com/download/1/6/1/161ba512-40e2-4cc9-843a-923143f3456c/pciidspec-11.doc
"Test will read and properly concatenate PCI IDs and verify uniqueness"
this is likely what you are running into: IDs must be unique,
so if you want to put your driver in Microsoft's database,
it must match a different set of IDs.
But you should not need to change the vendor ID to make them unique,
changing subsystem vendor ID will do.
Did you try this?
> Red Hat has non-redistributable Windows drivers and Microsoft
> will not allow anyone else to WHQL certify drivers using that
> vendor ID.
Don't see what Red Hat's windows drivers have to do with Linux really.
Amazon.com can do whatever it wants with its vendor ID, and if there is a
hypervisor with a different vendor ID that can use the virtio drivers, this
patch is required.
The following would be a reasonable commit log in that case:
"Amazon.com uses PV devices with vendor ID 0x1d0f that are otherwise
compatible with Linux virtio drivers. Add 0x1d0f ID to the list
to make Linux work with these devices."
Feel free to use :)
But I'd like to note that by doing this on the hypervisor side,
you lose the ability to run older Linux guests,
and create work for all distros who now need to update their
kernels to work with your ID, apparently for no good reason.
So if this isn't out in the field yet, I would suggest examining
the alternative listed above.
OTOH if it *is* decided this is going to be out there in the field, please add
the new devices to the PCI IDs list.
http://pci-ids.ucw.cz/
Otherwise there's no way to be sure someone won't try to
use these IDs for something else.
> That makes it impossible to use virtio drivers with
> a Windows guest without changing the vendor ID.
Hardly impossible: virtio drivers are available from a
variety of sources.
But this is IMO beside the point.
> Cc: Matt Wilson
> Cc: Rusty Russell
> Cc: Michael Tsirkin
> Signed-off-by: Anthony Liguori
I'd like to see the response/confirmation of the above, and/or the
commit log replaced before this patch is applied.
Thanks!
> ---
> drivers/virtio/virtio_pci.c |2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
> index 101db3f..9cbac33 100644
> --- a/drivers/virtio/virtio_pci.c
> +++ b/drivers/virtio/virtio_pci.c
> @@ -93,6 +93,8 @@ struct virtio_pci_vq_info
> /* Qumranet donated their vendor ID for devices 0x1000 thru 0x10FF. */
> static DEFINE_PCI_DEVICE_TABLE(virtio_pci_id_table) = {
> { PCI_DEVICE(0x1af4, PCI_ANY_ID) },
> + /* Amazon.com vendor ID */
> + { PCI_DEVICE(0x1d0f, PCI_ANY_ID) },
> { 0 }
> };
>
> --
> 1.7.9.5
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] virtio_pci: properly clean up MSI-X state when initialization fails
On Sun, Sep 14, 2014 at 08:23:26PM -0700, Anthony Liguori wrote:
> From: Anthony Liguori
>
> If MSI-X initialization fails after setting msix_enabled = 1, then
> the device is left in an inconsistent state. This would normally
> only happen if there was a bug in the device emulation but it still
> should be handled correctly.
This might happen if host runs out of resources when trying
to map VQs to vectors, so doesn't have to be a bug.
But I don't see what the problem is:
msix_used_vectors reflects the number of used vectors
and msix_enabled is set, thus vp_free_vectors
will free all IRQs and then disable MSIX.
Where is the inconsistency you speak about?
>
> Cc: Matt Wilson
> Cc: Rusty Russell
> Cc: Michael Tsirkin
> Signed-off-by: Anthony Liguori
> ---
> drivers/virtio/virtio_pci.c |8 ++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
> index 9cbac33..3d2c2a5 100644
> --- a/drivers/virtio/virtio_pci.c
> +++ b/drivers/virtio/virtio_pci.c
> @@ -357,7 +357,7 @@ static int vp_request_msix_vectors(struct virtio_device
> *vdev, int nvectors,
> v = ioread16(vp_dev->ioaddr + VIRTIO_MSI_CONFIG_VECTOR);
> if (v == VIRTIO_MSI_NO_VECTOR) {
> err = -EBUSY;
> - goto error;
> + goto error_msix_used;
> }
>
> if (!per_vq_vectors) {
> @@ -369,11 +369,15 @@ static int vp_request_msix_vectors(struct virtio_device
> *vdev, int nvectors,
> vp_vring_interrupt, 0, vp_dev->msix_names[v],
> vp_dev);
> if (err)
> - goto error;
> + goto error_msix_used;
> ++vp_dev->msix_used_vectors;
> }
> return 0;
> +error_msix_used:
> + v = --vp_dev->msix_used_vectors;
> + free_irq(vp_dev->msix_entries[v].vector, vp_dev);
> error:
> + vp_dev->msix_enabled = 0;
As far as I can see, if you do this, guest will not call
pci_disable_msix thus leaving the device with MSIX enabled.
I'm not sure this won't break drivers if they then
try to use the device without MSIX, and it
definitely seems less elegant than reverting the
device to the original state.
> vp_free_vectors(vdev);
> return err;
> }
> --
> 1.7.9.5
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [virtio-dev] [PATCH 2/2] virtio-gpu/2d: add docs/specs/virtio-gpu.txt
On Sa, 2014-09-13 at 07:14 +1000, Dave Airlie wrote: > >> Can the host refuse due to lack of resources? > > > > Yes. virtgpu_ctrl_hdr.type in the response will be set to > > VIRTGPU_RESP_ERR_* then. Current implementation does that only on > > malloc() failure, there is no accounting (yet) to limit the amout of > > memory the guest is allowed to allocate. > > We do probably need to work out some sort of accounting system, it can > probably reliably only be a total value of resources, since we've no > idea if the host driver will store them in VRAM or main memory. Quite > how to fail gracefully is a question, probably need to report to the > guest what context did the allocation and see if we can destroy it. Best would be if virgilrenderer.so just fails virgl_renderer_resource_create() calls. > Not reason I can remember, I think I was thinking of having separate > inval and detach at one point, but it didn't really make any sense, so > renaming to detach is fine with me. Done. cheers, Gerd ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [Qemu-devel] [PATCH 1/2] virtio-gpu/2d: add hardware spec include file
Hi, > >> > >> Why do you think that? We have several header files which > >> use QEMU_BUILD_BUG_ON and I don't see any reason why > >> it would have to be invoked from a .c file. > > > Because Gerd wants to share this with linux uapi, and > > you can't use BUILD_BUG_ON in uapi headers on linux. > > Who owns the "master" copy of the header and commits > to making sure it builds on other things than Linux+gcc > in that case? In my personal repo the master copy is in the linux tree and qemu gets synced. I've placed the QEMU_BUILD_BUG_ON calls in the (qemu-only) virtio-gpu.c now. cheers, Gerd ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 1/2] virtio-gpu/2d: add hardware spec include file
> > > > +
> > > > +enum virtgpu_ctrl_type {
> > > > +VIRTGPU_UNDEFINED = 0,
> > > > +
> > > > +/* 2d commands */
> > > > +VIRTGPU_CMD_GET_DISPLAY_INFO = 0x0100,
> > >
> > > Please consider also adding:
>
> VIRTIO_GPU_ everywhere to make it consistent with other
> virtio headers?
The names are already pretty long as-is, especially considering the
80cols rule in our codestyle. I'd prefer to keep them the way they are
now.
cheers,
Gerd
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 1/2] virtio-gpu/2d: add hardware spec include file
On Mon, Sep 15, 2014 at 12:40:07PM +0200, Gerd Hoffmann wrote:
> > > > > +
> > > > > +enum virtgpu_ctrl_type {
> > > > > +VIRTGPU_UNDEFINED = 0,
> > > > > +
> > > > > +/* 2d commands */
> > > > > +VIRTGPU_CMD_GET_DISPLAY_INFO = 0x0100,
> > > >
> > > > Please consider also adding:
> >
> > VIRTIO_GPU_ everywhere to make it consistent with other
> > virtio headers?
>
> The names are already pretty long as-is, especially considering the
> 80cols rule in our codestyle. I'd prefer to keep them the way they are
> now.
>
> cheers,
> Gerd
>
VIRTIO_GPU_ is not longer than VIRTIO_NET_ is it?
VIRTGRU seems too generic.
--
MST
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] virtio-pci: also bind to Amazon PCI vendor ID
On Sun, Sep 14, 2014 at 08:29:33PM -0700, Anthony Liguori wrote:
> From: Anthony Liguori
>
> See https://issues.oasis-open.org/browse/VIRTIO-16 although it
> was prematurely closed.
>
> Red Hat has non-redistributable Windows drivers and Microsoft
> will not allow anyone else to WHQL certify drivers using that
> vendor ID. That makes it impossible to use virtio drivers with
> a Windows guest without changing the vendor ID.
>
> Cc: Matt Wilson
> Cc: Rusty Russell
> Cc: Michael Tsirkin
> Signed-off-by: Anthony Liguori
Reviewed-by: Matt Wilson
--msw
> ---
> drivers/virtio/virtio_pci.c |2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
> index 101db3f..9cbac33 100644
> --- a/drivers/virtio/virtio_pci.c
> +++ b/drivers/virtio/virtio_pci.c
> @@ -93,6 +93,8 @@ struct virtio_pci_vq_info
> /* Qumranet donated their vendor ID for devices 0x1000 thru 0x10FF. */
> static DEFINE_PCI_DEVICE_TABLE(virtio_pci_id_table) = {
> { PCI_DEVICE(0x1af4, PCI_ANY_ID) },
> + /* Amazon.com vendor ID */
> + { PCI_DEVICE(0x1d0f, PCI_ANY_ID) },
> { 0 }
> };
>
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] virtio_pci: properly clean up MSI-X state when initialization fails
On Sun, Sep 14, 2014 at 08:23:26PM -0700, Anthony Liguori wrote:
> From: Anthony Liguori
>
> If MSI-X initialization fails after setting msix_enabled = 1, then
> the device is left in an inconsistent state. This would normally
> only happen if there was a bug in the device emulation but it still
> should be handled correctly.
>
> Cc: Matt Wilson
> Cc: Rusty Russell
> Cc: Michael Tsirkin
> Signed-off-by: Anthony Liguori
Reviewed-by: Matt Wilson
--msw
> ---
> drivers/virtio/virtio_pci.c |8 ++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
> index 9cbac33..3d2c2a5 100644
> --- a/drivers/virtio/virtio_pci.c
> +++ b/drivers/virtio/virtio_pci.c
> @@ -357,7 +357,7 @@ static int vp_request_msix_vectors(struct virtio_device
> *vdev, int nvectors,
> v = ioread16(vp_dev->ioaddr + VIRTIO_MSI_CONFIG_VECTOR);
> if (v == VIRTIO_MSI_NO_VECTOR) {
> err = -EBUSY;
> - goto error;
> + goto error_msix_used;
> }
>
> if (!per_vq_vectors) {
> @@ -369,11 +369,15 @@ static int vp_request_msix_vectors(struct virtio_device
> *vdev, int nvectors,
> vp_vring_interrupt, 0, vp_dev->msix_names[v],
> vp_dev);
> if (err)
> - goto error;
> + goto error_msix_used;
> ++vp_dev->msix_used_vectors;
> }
> return 0;
> +error_msix_used:
> + v = --vp_dev->msix_used_vectors;
> + free_irq(vp_dev->msix_entries[v].vector, vp_dev);
> error:
> + vp_dev->msix_enabled = 0;
> vp_free_vectors(vdev);
> return err;
> }
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] virtio_pci: properly clean up MSI-X state when initialization fails
Hi Michael,
On Mon, Sep 15, 2014 at 1:32 AM, Michael S. Tsirkin wrote:
> On Sun, Sep 14, 2014 at 08:23:26PM -0700, Anthony Liguori wrote:
>> From: Anthony Liguori
>>
>> If MSI-X initialization fails after setting msix_enabled = 1, then
>> the device is left in an inconsistent state. This would normally
>> only happen if there was a bug in the device emulation but it still
>> should be handled correctly.
>
> This might happen if host runs out of resources when trying
> to map VQs to vectors, so doesn't have to be a bug.
>
> But I don't see what the problem is:
> msix_used_vectors reflects the number of used vectors
> and msix_enabled is set, thus vp_free_vectors
> will free all IRQs and then disable MSIX.
>
> Where is the inconsistency you speak about?
I missed the fact that vp_free_vectors() conditionally sets
msix_enabled=0. It seems a bit cludgy especially since it is called
both before and after setting msix_enabled=1.
I ran into a number of weird problems due to read/write reordering
that was causing this code path to fail. The impact was
non-deterministic. I'll go back and try to better understand what
code path is causing it.
>> Cc: Matt Wilson
>> Cc: Rusty Russell
>> Cc: Michael Tsirkin
>> Signed-off-by: Anthony Liguori
>> ---
>> drivers/virtio/virtio_pci.c |8 ++--
>> 1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
>> index 9cbac33..3d2c2a5 100644
>> --- a/drivers/virtio/virtio_pci.c
>> +++ b/drivers/virtio/virtio_pci.c
>> @@ -357,7 +357,7 @@ static int vp_request_msix_vectors(struct virtio_device
>> *vdev, int nvectors,
>> v = ioread16(vp_dev->ioaddr + VIRTIO_MSI_CONFIG_VECTOR);
>> if (v == VIRTIO_MSI_NO_VECTOR) {
>> err = -EBUSY;
>> - goto error;
>> + goto error_msix_used;
>> }
>>
>> if (!per_vq_vectors) {
>> @@ -369,11 +369,15 @@ static int vp_request_msix_vectors(struct
>> virtio_device *vdev, int nvectors,
>> vp_vring_interrupt, 0, vp_dev->msix_names[v],
>> vp_dev);
>> if (err)
>> - goto error;
>> + goto error_msix_used;
>> ++vp_dev->msix_used_vectors;
>> }
>> return 0;
>> +error_msix_used:
>> + v = --vp_dev->msix_used_vectors;
>> + free_irq(vp_dev->msix_entries[v].vector, vp_dev);
>> error:
>> + vp_dev->msix_enabled = 0;
>
> As far as I can see, if you do this, guest will not call
> pci_disable_msix thus leaving the device with MSIX enabled.
I don't understand this comment. How is the work done in this path
any different from what's done in vp_free_vectors()?
Regards,
Anthony Liguori
> I'm not sure this won't break drivers if they then
> try to use the device without MSIX, and it
> definitely seems less elegant than reverting the
> device to the original state.
>
>
>> vp_free_vectors(vdev);
>> return err;
>> }
>> --
>> 1.7.9.5
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] virtio-pci: also bind to Amazon PCI vendor ID
Hi Michael, On Mon, Sep 15, 2014 at 1:20 AM, Michael S. Tsirkin wrote: > On Sun, Sep 14, 2014 at 08:29:33PM -0700, Anthony Liguori wrote: >> From: Anthony Liguori >> >> See https://issues.oasis-open.org/browse/VIRTIO-16 although it >> was prematurely closed. > > The reason it was closed is described in the comments. and I quote: > " I think anyone can use a different subsystem vendor id and whql the > driver. virtio-102 will make this even easier, by making the subsystem > id flexible. Let's close this and re-open if someone tries to do this > and runs into a problem. " > > Look here for example: > https://github.com/YanVugenfirer/kvm-guest-drivers-windows/blob/master/NetKVM/NDIS5/wxp/netkvm.inf > Replace SUBSYS_00011AF4 with SUBSYS_00011D0F, and you will get > a virtio-net driver that (I think) you should be able to WHQL. The string you are referencing is the device description which is part of what forms the device instance id. You can read more at http://msdn.microsoft.com/en-us/library/windows/hardware/ff541327%28v=vs.85%29.aspx. Including a SUBSYS entry is mandatory in modern Windows drivers. But this is independent of WHQL certification. My understanding is that Microsoft will only allow the owner of the PCI Vendor ID to WHQL drivers. As best as I know, this is not a publicly documented process. Do you have any examples of anyone else successfuling WHQL'ing drivers by just changing the subsystem ID? Microsoft has specific rules about the usage of the subsystem ID. See http://msdn.microsoft.com/en-us/library/windows/hardware/dn653567%28v=vs.85%29.aspx. I don't think it is intended to enable a totally different implementation of the driver based on subsystem ID. Certainly, this is not expected with typical PCI devices. > On the host side, you will need a QEMU patch to allow libvirt control of > the subsystem vendor ID. > > All this while all Linux guests will keep working without changes, > which seems like a better approach. > > Looking on the web, I found: > http://download.microsoft.com/download/1/6/1/161ba512-40e2-4cc9-843a-923143f3456c/pciidspec-11.doc > "Test will read and properly concatenate PCI IDs and verify uniqueness" > this is likely what you are running into: IDs must be unique, > so if you want to put your driver in Microsoft's database, > it must match a different set of IDs. > But you should not need to change the vendor ID to make them unique, > changing subsystem vendor ID will do. > > Did you try this? You cannot submit a modified kvm-guest-drivers.git for WHQL certification as the licensing is constructed in such a way as to prevent that. >> Red Hat has non-redistributable Windows drivers and Microsoft >> will not allow anyone else to WHQL certify drivers using that >> vendor ID. > > Don't see what Red Hat's windows drivers have to do with Linux really. > Amazon.com can do whatever it wants with its vendor ID, and if there is a > hypervisor with a different vendor ID that can use the virtio drivers, this > patch is required. > The following would be a reasonable commit log in that case: > > "Amazon.com uses PV devices with vendor ID 0x1d0f that are otherwise > compatible with Linux virtio drivers. Add 0x1d0f ID to the list > to make Linux work with these devices." > Feel free to use :) > > > But I'd like to note that by doing this on the hypervisor side, > you lose the ability to run older Linux guests, > and create work for all distros who now need to update their > kernels to work with your ID, apparently for no good reason. > > So if this isn't out in the field yet, I would suggest examining > the alternative listed above. I am very happy to use any alternative mechanism that allows for virtio to be used with a wide variety of guests. Many other companies have also struggled with this and AFAIK no one has had much success. > OTOH if it *is* decided this is going to be out there in the field, please add > the new devices to the PCI IDs list. > http://pci-ids.ucw.cz/ > Otherwise there's no way to be sure someone won't try to > use these IDs for something else. PCI-SIG assigns vendor IDs and 0x1d0f is assigned to Amazon. See https://www.pcisig.com/membership/vid_search/ Vendors self-manage device IDs and we have allocated 0x1000-0x103f to virtio devices. >> That makes it impossible to use virtio drivers with >> a Windows guest without changing the vendor ID. > > Hardly impossible: virtio drivers are available from a > variety of sources. Examples? Regards, Anthony Liguori > > But this is IMO beside the point. > >> Cc: Matt Wilson >> Cc: Rusty Russell >> Cc: Michael Tsirkin >> Signed-off-by: Anthony Liguori > > I'd like to see the response/confirmation of the above, and/or the > commit log replaced before this patch is applied. > > Thanks! > >> --- >> drivers/virtio/virtio_pci.c |2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c >> index 101db3f..9cbac33 100644 >> --- a/drivers/v
Re: [PATCH] virtio_pci: properly clean up MSI-X state when initialization fails
On Mon, Sep 15, 2014 at 07:10:45AM -0700, Anthony Liguori wrote:
> Hi Michael,
>
> On Mon, Sep 15, 2014 at 1:32 AM, Michael S. Tsirkin wrote:
> > On Sun, Sep 14, 2014 at 08:23:26PM -0700, Anthony Liguori wrote:
> >> From: Anthony Liguori
> >>
> >> If MSI-X initialization fails after setting msix_enabled = 1, then
> >> the device is left in an inconsistent state. This would normally
> >> only happen if there was a bug in the device emulation but it still
> >> should be handled correctly.
> >
> > This might happen if host runs out of resources when trying
> > to map VQs to vectors, so doesn't have to be a bug.
> >
> > But I don't see what the problem is:
> > msix_used_vectors reflects the number of used vectors
> > and msix_enabled is set, thus vp_free_vectors
> > will free all IRQs and then disable MSIX.
> >
> > Where is the inconsistency you speak about?
>
> I missed the fact that vp_free_vectors() conditionally sets
> msix_enabled=0. It seems a bit cludgy especially since it is called
> both before and after setting msix_enabled=1.
It's the style of initialization that records the current initialization
stage, and then uses that to do all cleanup in a single place
(as compared to detailed separate goto labels for each initialization
stage).
I don't mind either keeping this style or changing to another style,
but if we change it we should change it everywhere I think.
> I ran into a number of weird problems due to read/write reordering
> that was causing this code path to fail. The impact was
> non-deterministic. I'll go back and try to better understand what
> code path is causing it.
>
> >> Cc: Matt Wilson
> >> Cc: Rusty Russell
> >> Cc: Michael Tsirkin
> >> Signed-off-by: Anthony Liguori
> >> ---
> >> drivers/virtio/virtio_pci.c |8 ++--
> >> 1 file changed, 6 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/virtio/virtio_pci.c b/drivers/virtio/virtio_pci.c
> >> index 9cbac33..3d2c2a5 100644
> >> --- a/drivers/virtio/virtio_pci.c
> >> +++ b/drivers/virtio/virtio_pci.c
> >> @@ -357,7 +357,7 @@ static int vp_request_msix_vectors(struct
> >> virtio_device *vdev, int nvectors,
> >> v = ioread16(vp_dev->ioaddr + VIRTIO_MSI_CONFIG_VECTOR);
> >> if (v == VIRTIO_MSI_NO_VECTOR) {
> >> err = -EBUSY;
> >> - goto error;
> >> + goto error_msix_used;
> >> }
> >>
> >> if (!per_vq_vectors) {
> >> @@ -369,11 +369,15 @@ static int vp_request_msix_vectors(struct
> >> virtio_device *vdev, int nvectors,
> >> vp_vring_interrupt, 0,
> >> vp_dev->msix_names[v],
> >> vp_dev);
> >> if (err)
> >> - goto error;
> >> + goto error_msix_used;
> >> ++vp_dev->msix_used_vectors;
> >> }
> >> return 0;
> >> +error_msix_used:
> >> + v = --vp_dev->msix_used_vectors;
> >> + free_irq(vp_dev->msix_entries[v].vector, vp_dev);
> >> error:
> >> + vp_dev->msix_enabled = 0;
> >
> > As far as I can see, if you do this, guest will not call
> > pci_disable_msix thus leaving the device with MSIX enabled.
>
> I don't understand this comment. How is the work done in this path
> any different from what's done in vp_free_vectors()?
>
> Regards,
>
> Anthony Liguori
>
> > I'm not sure this won't break drivers if they then
> > try to use the device without MSIX, and it
> > definitely seems less elegant than reverting the
> > device to the original state.
> >
> >
> >> vp_free_vectors(vdev);
> >> return err;
> >> }
> >> --
> >> 1.7.9.5
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH v2 1/3] virtio-rng cleanup: move some code out of mutex protection
It doesn't save too much cpu time as expected, just a cleanup.
Signed-off-by: Amos Kong
---
drivers/char/hw_random/core.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
index aa30a25..c591d7e 100644
--- a/drivers/char/hw_random/core.c
+++ b/drivers/char/hw_random/core.c
@@ -270,8 +270,8 @@ static ssize_t hwrng_attr_current_show(struct device *dev,
return -ERESTARTSYS;
if (current_rng)
name = current_rng->name;
- ret = snprintf(buf, PAGE_SIZE, "%s\n", name);
mutex_unlock(&rng_mutex);
+ ret = snprintf(buf, PAGE_SIZE, "%s\n", name);
return ret;
}
@@ -284,19 +284,19 @@ static ssize_t hwrng_attr_available_show(struct device
*dev,
ssize_t ret = 0;
struct hwrng *rng;
+ buf[0] = '\0';
err = mutex_lock_interruptible(&rng_mutex);
if (err)
return -ERESTARTSYS;
- buf[0] = '\0';
list_for_each_entry(rng, &rng_list, list) {
strncat(buf, rng->name, PAGE_SIZE - ret - 1);
ret += strlen(rng->name);
strncat(buf, " ", PAGE_SIZE - ret - 1);
ret++;
}
+ mutex_unlock(&rng_mutex);
strncat(buf, "\n", PAGE_SIZE - ret - 1);
ret++;
- mutex_unlock(&rng_mutex);
return ret;
}
--
1.9.3
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH v2 0/3] fix stuck in accessing hwrng attributes
If we read hwrng by long-running dd process, it takes too much cpu time and almost hold the mutex lock. When we check hwrng attributes from sysfs by cat, it gets stuck in waiting the lock releaseing. The problem can only be reproduced with non-smp guest with slow backend. This patchset resolves the issue by changing rng_dev_read() to always schedule 10 jiffies after release mutex lock, then cat process can have chance to get the lock and execute protected code without stuck. Thanks. V2: update commitlog to describe PATCH 2, split second patch. Amos Kong (3): virtio-rng cleanup: move some code out of mutex protection hw_random: fix stuck in catting hwrng attributes hw_random: increase schedule timeout in rng_dev_read() drivers/char/hw_random/core.c | 9 - 1 file changed, 4 insertions(+), 5 deletions(-) -- 1.9.3 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH v2 2/3] hw_random: fix stuck in catting hwrng attributes
I started a QEMU (non-smp) guest with one virtio-rng device, and read
random data from /dev/hwrng by dd:
# dd if=/dev/hwrng of=/dev/null &
In the same time, if I check hwrng attributes from sysfs by cat:
# cat /sys/class/misc/hw_random/rng_*
The cat process always gets stuck with slow backend (5 k/s), if we
use a quick backend (1.2 M/s), the cat process will cost 1 to 2
minutes. The stuck doesn't exist for smp guest.
Reading syscall enters kernel and call rng_dev_read(), it's user
context. We used need_resched() to check if other tasks need to
be run, but it almost always return false, and re-hold the mutex
lock. The attributes accessing process always fails to hold the
lock, so the cat gets stuck.
User context doesn't allow other user contexts run on that CPU,
unless the kernel code sleeps for some reason. This is why the
need_reshed() always return false here.
This patch removed need_resched() and always schedule other tasks
then other tasks can have chance to hold the lock and execute
protected code.
Signed-off-by: Amos Kong
---
drivers/char/hw_random/core.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
index c591d7e..263a370 100644
--- a/drivers/char/hw_random/core.c
+++ b/drivers/char/hw_random/core.c
@@ -195,8 +195,7 @@ static ssize_t rng_dev_read(struct file *filp, char __user
*buf,
mutex_unlock(&rng_mutex);
- if (need_resched())
- schedule_timeout_interruptible(1);
+ schedule_timeout_interruptible(1);
if (signal_pending(current)) {
err = -ERESTARTSYS;
--
1.9.3
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH v2 3/3] hw_random: increase schedule timeout in rng_dev_read()
This patch increases the schedule timeout to 10 jiffies, it's more
appropriate, then other takes can easy to hold the mutex lock.
Signed-off-by: Amos Kong
---
drivers/char/hw_random/core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
index 263a370..b5d1b6f 100644
--- a/drivers/char/hw_random/core.c
+++ b/drivers/char/hw_random/core.c
@@ -195,7 +195,7 @@ static ssize_t rng_dev_read(struct file *filp, char __user
*buf,
mutex_unlock(&rng_mutex);
- schedule_timeout_interruptible(1);
+ schedule_timeout_interruptible(10);
if (signal_pending(current)) {
err = -ERESTARTSYS;
--
1.9.3
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] virtio-pci: also bind to Amazon PCI vendor ID
On Mon, Sep 15, 2014 at 07:24:33AM -0700, Anthony Liguori wrote: > Hi Michael, > > On Mon, Sep 15, 2014 at 1:20 AM, Michael S. Tsirkin wrote: > > On Sun, Sep 14, 2014 at 08:29:33PM -0700, Anthony Liguori wrote: > >> From: Anthony Liguori > >> > >> See https://issues.oasis-open.org/browse/VIRTIO-16 although it > >> was prematurely closed. > > > > The reason it was closed is described in the comments. and I quote: > > " I think anyone can use a different subsystem vendor id and whql the > > driver. virtio-102 will make this even easier, by making the subsystem > > id flexible. Let's close this and re-open if someone tries to do this > > and runs into a problem. " > > > > Look here for example: > > https://github.com/YanVugenfirer/kvm-guest-drivers-windows/blob/master/NetKVM/NDIS5/wxp/netkvm.inf > > Replace SUBSYS_00011AF4 with SUBSYS_00011D0F, and you will get > > a virtio-net driver that (I think) you should be able to WHQL. > > The string you are referencing is the device description which is part > of what forms the device instance id. You can read more at > http://msdn.microsoft.com/en-us/library/windows/hardware/ff541327%28v=vs.85%29.aspx. > Including a SUBSYS entry is mandatory in modern Windows drivers. Good. So there won't be conflicts as long as you don't try to use the same SUBSYS as someone else. > But this is independent of WHQL certification. My understanding is > that Microsoft will only allow the owner of the PCI Vendor ID to WHQL > drivers. As best as I know, this is not a publicly documented > process. WHQL process seems to be documented: http://download.microsoft.com/download/4/D/D/4DD894CD-62C8-488F-944D-4E5F8BA40114/hardware-certification-policies-processes-hck2-1.docx > Do you have any examples of anyone else successfuling WHQL'ing drivers > by just changing the subsystem ID? I don't have such examples to hand, sorry. I do think it's worth trying as I don't see any problems this could cause anyone. In particular maybe this applies (from link above): If a reseller wants to control distribution for its product, take the following steps: 1. Revise the PnP ID of the product to include the subvendor ID. 2. Submit a Driver Update Submission that revises the information (.inf) file to reflect the new PnP ID. If a reseller wants to change the driver binary for a device, a new initial device submission must be made (with full testing). All reseller submissions are subject to audit. Seems to say you can do this. > Microsoft has specific rules about > the usage of the subsystem ID. See > http://msdn.microsoft.com/en-us/library/windows/hardware/dn653567%28v=vs.85%29.aspx. > I don't think it is intended to enable a totally different > implementation of the driver based on subsystem ID. > Certainly, this is not expected with typical PCI devices. Why not? Changing device ID doesn't seem to be very different. It's not uncommon for devices with specific subsystem ID to have different bugs, or be supported by different driver versions, or just be tested only with a specific driver. One of these from the list above might fit the bill: - Driver packages submitted by an OEM for logo (must be tested on the OEM hardware and must not contain a VID/DID entry) - Driver packages submitted by an independent hardware vendor providing customizations for an OEM (must be tested on the OEM hardware and must not contain a VID/DID entry) - Driver packages with a bug fix for a specific implementation for example, you can rip out all kid of compatibility code for old hosts. > > > On the host side, you will need a QEMU patch to allow libvirt control of > > the subsystem vendor ID. > > > > All this while all Linux guests will keep working without changes, > > which seems like a better approach. > > > > Looking on the web, I found: > > http://download.microsoft.com/download/1/6/1/161ba512-40e2-4cc9-843a-923143f3456c/pciidspec-11.doc > > "Test will read and properly concatenate PCI IDs and verify uniqueness" > > this is likely what you are running into: IDs must be unique, > > so if you want to put your driver in Microsoft's database, > > it must match a different set of IDs. > > But you should not need to change the vendor ID to make them unique, > > changing subsystem vendor ID will do. > > > > Did you try this? > > You cannot submit a modified kvm-guest-drivers.git for WHQL > certification Sorry about being unclear, that github link was just an example to give you the idea. The point is to change the subsystem ID *of the device*. > as the licensing is constructed in such a way as to > prevent that. > > >> Red Hat has non-redistributable Windows drivers and Microsoft > >> will not allow anyone else to WHQL certify drivers using that > >> vendor ID. > > > > Don't see what Red Hat's windows drivers have to do with Linux really. > > Amazon.com can do whatever it wa
Re: [PATCH v2 1/3] virtio-rng cleanup: move some code out of mutex protection
On Tue, 16 Sep 2014 00:02:27 +0800
Amos Kong wrote:
> It doesn't save too much cpu time as expected, just a cleanup.
>
> Signed-off-by: Amos Kong
> ---
> drivers/char/hw_random/core.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
> index aa30a25..c591d7e 100644
> --- a/drivers/char/hw_random/core.c
> +++ b/drivers/char/hw_random/core.c
> @@ -270,8 +270,8 @@ static ssize_t hwrng_attr_current_show(struct device *dev,
> return -ERESTARTSYS;
> if (current_rng)
> name = current_rng->name;
> - ret = snprintf(buf, PAGE_SIZE, "%s\n", name);
> mutex_unlock(&rng_mutex);
> + ret = snprintf(buf, PAGE_SIZE, "%s\n", name);
I'm not sure this is safe.
Name is just a pointer.
What if the hwrng gets unregistered after unlock and just before the snprintf?
> return ret;
> }
> @@ -284,19 +284,19 @@ static ssize_t hwrng_attr_available_show(struct device
> *dev,
> ssize_t ret = 0;
> struct hwrng *rng;
>
> + buf[0] = '\0';
> err = mutex_lock_interruptible(&rng_mutex);
> if (err)
> return -ERESTARTSYS;
> - buf[0] = '\0';
> list_for_each_entry(rng, &rng_list, list) {
> strncat(buf, rng->name, PAGE_SIZE - ret - 1);
> ret += strlen(rng->name);
> strncat(buf, " ", PAGE_SIZE - ret - 1);
> ret++;
> }
> + mutex_unlock(&rng_mutex);
> strncat(buf, "\n", PAGE_SIZE - ret - 1);
> ret++;
> - mutex_unlock(&rng_mutex);
>
> return ret;
> }
This looks ok.
--
Michael
signature.asc
Description: PGP signature
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 3/3] hw_random: increase schedule timeout in rng_dev_read()
On Tue, 16 Sep 2014 00:02:29 +0800
Amos Kong wrote:
> This patch increases the schedule timeout to 10 jiffies, it's more
> appropriate, then other takes can easy to hold the mutex lock.
>
> Signed-off-by: Amos Kong
> ---
> drivers/char/hw_random/core.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
> index 263a370..b5d1b6f 100644
> --- a/drivers/char/hw_random/core.c
> +++ b/drivers/char/hw_random/core.c
> @@ -195,7 +195,7 @@ static ssize_t rng_dev_read(struct file *filp, char
> __user *buf,
>
> mutex_unlock(&rng_mutex);
>
> - schedule_timeout_interruptible(1);
> + schedule_timeout_interruptible(10);
>
> if (signal_pending(current)) {
> err = -ERESTARTSYS;
Does a schedule of 1 ms or 10 ms decrease the throughput?
I think we need some benchmarks.
--
Michael
signature.asc
Description: PGP signature
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 2/2] virtio-rng: fix stuck in catting hwrng attributes
2014-09-14 10:25+0800, Amos Kong: > On Sun, Sep 14, 2014 at 09:12:08AM +0800, Amos Kong wrote: > > ... > > > > > diff --git a/drivers/char/hw_random/core.c > > > > > b/drivers/char/hw_random/core.c > > > > > index c591d7e..b5d1b6f 100644 > > > > > --- a/drivers/char/hw_random/core.c > > > > > +++ b/drivers/char/hw_random/core.c > > > > > @@ -195,8 +195,7 @@ static ssize_t rng_dev_read(struct file *filp, > > > > > char __user *buf, > > > > > > > > > > mutex_unlock(&rng_mutex); > > > > > > > > > > - if (need_resched()) > > > > > - schedule_timeout_interruptible(1); > > > > > + schedule_timeout_interruptible(10); If cond_resched() does not work, it is a bug elsewehere. > Problem only occurred in non-smp guest, we can improve it to: > > if(!is_smp()) > schedule_timeout_interruptible(10); > > is_smp() is only available for arm arch, we need a general one. (It is num_online_cpus() > 1.) ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH] virtio-pci: also bind to Amazon PCI vendor ID
On Mon, Sep 15, 2014 at 07:23:08PM +0300, Michael S. Tsirkin wrote: > On Mon, Sep 15, 2014 at 07:24:33AM -0700, Anthony Liguori wrote: > > Hi Michael, > > > > On Mon, Sep 15, 2014 at 1:20 AM, Michael S. Tsirkin wrote: > > > On Sun, Sep 14, 2014 at 08:29:33PM -0700, Anthony Liguori wrote: > > >> From: Anthony Liguori > > >> > > >> See https://issues.oasis-open.org/browse/VIRTIO-16 although it > > >> was prematurely closed. > > > > > > The reason it was closed is described in the comments. and I quote: > > > " I think anyone can use a different subsystem vendor id and whql the > > > driver. virtio-102 will make this even easier, by making the subsystem > > > id flexible. Let's close this and re-open if someone tries to do this > > > and runs into a problem. " > > > > > > Look here for example: > > > https://github.com/YanVugenfirer/kvm-guest-drivers-windows/blob/master/NetKVM/NDIS5/wxp/netkvm.inf > > > Replace SUBSYS_00011AF4 with SUBSYS_00011D0F, and you will get > > > a virtio-net driver that (I think) you should be able to WHQL. > > > > The string you are referencing is the device description which is part > > of what forms the device instance id. You can read more at > > http://msdn.microsoft.com/en-us/library/windows/hardware/ff541327%28v=vs.85%29.aspx. > > Including a SUBSYS entry is mandatory in modern Windows drivers. > > Good. So there won't be conflicts as long as you don't > try to use the same SUBSYS as someone else. > > > But this is independent of WHQL certification. My understanding is > > that Microsoft will only allow the owner of the PCI Vendor ID to WHQL > > drivers. As best as I know, this is not a publicly documented > > process. > > WHQL process seems to be documented: > http://download.microsoft.com/download/4/D/D/4DD894CD-62C8-488F-944D-4E5F8BA40114/hardware-certification-policies-processes-hck2-1.docx > > Do you have any examples of anyone else successfuling WHQL'ing drivers > > by just changing the subsystem ID? > > I don't have such examples to hand, sorry. > I do think it's worth trying as I don't see any problems > this could cause anyone. > > > In particular maybe this applies (from link above): > > If a reseller wants to control distribution for its product, take the > following steps: > 1. Revise the PnP ID of the product to include the subvendor ID. > 2. Submit a Driver Update Submission that revises the information (.inf) > file to reflect the new PnP ID. > If a reseller wants to change the driver binary for a device, a new > initial device submission must be made (with full testing). > All reseller submissions are subject to audit. > > Seems to say you can do this. This is part of the Reseller Submission Policy, and only applies if there is a "parent submission" that has gone through the WHQL process. > > Microsoft has specific rules about > > the usage of the subsystem ID. See > > http://msdn.microsoft.com/en-us/library/windows/hardware/dn653567%28v=vs.85%29.aspx. > > I don't think it is intended to enable a totally different > > implementation of the driver based on subsystem ID. > > Certainly, this is not expected with typical PCI devices. > > Why not? Changing device ID doesn't seem to be very different. > It's not uncommon for devices with specific subsystem ID > to have different bugs, or be supported by different driver versions, > or just be tested only with a specific driver. > > One of these from the list above might fit the bill: > > - Driver packages submitted by an OEM for logo (must be tested on > the OEM hardware and must not contain a VID/DID entry) > - Driver packages submitted by an independent hardware vendor providing > customizations for an OEM (must be tested on the OEM hardware and must > not contain a VID/DID entry) > - Driver packages with a bug fix for a specific implementation > for example, you can rip out all kid of > compatibility code for old hosts. I think that the best technical solution would be to use a different subsystem vendor ID, so long as both the WHQL process and Windows Update supports this approach. > > > On the host side, you will need a QEMU patch to allow libvirt control of > > > the subsystem vendor ID. > > > > > > All this while all Linux guests will keep working without changes, > > > which seems like a better approach. > > > > > > Looking on the web, I found: > > > http://download.microsoft.com/download/1/6/1/161ba512-40e2-4cc9-843a-923143f3456c/pciidspec-11.doc > > > "Test will read and properly concatenate PCI IDs and verify uniqueness" > > > this is likely what you are running into: IDs must be unique, > > > so if you want to put your driver in Microsoft's database, > > > it must match a different set of IDs. > > > But you should not need to change the vendor ID to make them unique, > > > changing subsystem vendor ID will do. > > > > > > Did you try this? > > > > You cannot submit a modified
CFP: IEEE/ACM Int. Symposium on Big Data Computing (BDC) 2014 -- 1 week deadline extension
Call for Papers IEEE/ACM International Symposium on Big Data Computing (BDC) 2014 December 8-11, 2014, London, UK http://www.cloudbus.org/bdc2014 In conjunction with: 7th IEEE/ACM International Conference on Utility and Cloud Computing (UCC 2014) Sponsored by: IEEE Computer Society and ACM (Association for Computing Machinery) Introduction === Rapid advances in digital sensors, networks, storage, and computation along with their availability at low cost is leading to the creation of huge collections of data -- dubbed as Big Data. This data has the potential for enabling new insights that can change the way business, science, and governments deliver services to their consumers and can impact society as a whole. This has led to the emergence of the Big Data Computing paradigm focusing on sensing, collection, storage, management and analysis of data from variety of sources to enable new value and insights. To realize the full potential of Big Data Computing, we need to address several challenges and develop suitable conceptual and technological solutions for dealing them. These include life-cycle management of data, large-scale storage, flexible processing infrastructure, data modelling, scalable machine learning and data analysis algorithms, techniques for sampling and making trade-off between data processing time and accuracy, and dealing with privacy and ethical issues involved in data sensing, storage, processing, and actions. The IEEE/ACM International Symposium on Big Data Computing (BDC) 2014 -- held in conjunction with 7th IEEE/ACM International Conference on Utility and Cloud Computing (UCC 2014), December 8-11, 2014, London, UK, aims at bringing together international researchers, developers, policy makers, and users and to provide an international forum to present leading research activities, technical solutions, and results on a broad range of topics related to Big Data Computing paradigms, platforms and their applications. The conference features keynotes, technical presentations, posters, workshops, tutorials, as well as competitions featuring live demonstrations. Topics === Topics of interest include, but are not limited to: I. Big Data Science * Analytics * Algorithms for Big Data * Energy-efficient Algorithms * Big Data Search * Big Data Acquisition, Integration, Cleaning, and Best Practices * Visualization of Big Data II. Big Data Infrastructures and Platforms * Programming Systems * Cyber-Infrastructure * Performance evaluation * Fault tolerance and reliability * I/O and Data management * Storage Systems (including file systems, NoSQL, and RDBMS) * Resource management * Many-Task Computing * Many-core computing and accelerators III. Big Data Security and Policy * Management Policies * Data Privacy * Data Security * Big Data Archival and Preservation * Big Data Provenance IV. Big Data Applications * Scientific application cases studies on Cloud infrastructure * Big Data Applications at Scale * Experience Papers with Big Data Application Deployments * Data streaming applications * Big Data in Social Networks * Healthcare Applications * Enterprise Applications IMPORTANT DATES === * Abstracts Due:September 15th, 2014 * Papers Due: September 22nd, 2014 * Notification of Acceptance: October 15th, 2014 * Camera Ready Papers Due: October 31st, 2014 Note: Those who submit an abstract by the deadline will be given 1 week to upload the final paper. PAPER SUBMISSION === Authors are invited to submit papers electronically. Submitted manuscripts should be structured as technical papers and may not exceed 10 letter size (8.5 x 11) pages including figures, tables and references using the IEEE format for conference proceedings (print area of 6-1/2 inches (16.51 cm) wide by 8-7/8 inches (22.51 cm) high, two-column format with columns 3-1/16 inches (7.85 cm) wide with a 3/8 inch (0.81 cm) space between them, single-spaced 10-point Times fully justified text). Submissions not conforming to these guidelines may be returned without review. Authors should submit the manuscript in PDF format and make sure that the file will print on a printer that uses letter size (8.5 x 11) paper. The official language of the meeting is English. All manuscripts will be reviewed and will be judged on correctness, originality, technical strength, significance, quality of presentation, and interest and relevance to the conference attendees. Papers conforming to the above guidelines can be submitted through the BDC 2014 paper submission system (https://www.easychair.org/conferences/?conf=bdc2014). Submitted papers must represent original unpublished research that is not currently
Re: [PATCH v2 3/3] hw_random: increase schedule timeout in rng_dev_read()
On Mon, Sep 15, 2014 at 06:13:31PM +0200, Michael Büsch wrote:
> On Tue, 16 Sep 2014 00:02:29 +0800
> Amos Kong wrote:
>
> > This patch increases the schedule timeout to 10 jiffies, it's more
> > appropriate, then other takes can easy to hold the mutex lock.
> >
> > Signed-off-by: Amos Kong
> > ---
> > drivers/char/hw_random/core.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
> > index 263a370..b5d1b6f 100644
> > --- a/drivers/char/hw_random/core.c
> > +++ b/drivers/char/hw_random/core.c
> > @@ -195,7 +195,7 @@ static ssize_t rng_dev_read(struct file *filp, char
> > __user *buf,
> >
> > mutex_unlock(&rng_mutex);
> >
> > - schedule_timeout_interruptible(1);
> > + schedule_timeout_interruptible(10);
> >
> > if (signal_pending(current)) {
> > err = -ERESTARTSYS;
>
> Does a schedule of 1 ms or 10 ms decrease the throughput?
In my test environment, 1 jiffe always works (100%), as suggested by
Amit 10 jiffes is more appropriate.
After applied current 3 patches, there is a throughput regression.
1.2 M/s -> 6 K/s
We can only schedule in the end of loop (size == 0), and only for
non-smp guest. So smp guest won't be effected.
| if (!size && num_online_cpus() == 1)
| schedule_timeout_interruptible(timeout);
Set timeout to 1:
non-smp guest with quick backend (1.2M/s) -> about 49K/s)
Set timeout to 10:
non-smp guest with quick backend (1.2M/s) -> about 490K/s)
We might need other benchmark to test the performance, but we can
see the bug clearly caused a regression.
As we discussed in other thread, need_resched() should work in this
case, so those patches might be wrong fixing.
> I think we need some benchmarks.
>
> --
> Michael
--
Amos.
pgpEioLyquXyk.pgp
Description: PGP signature
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 1/3] virtio-rng cleanup: move some code out of mutex protection
On Mon, Sep 15, 2014 at 06:13:20PM +0200, Michael Büsch wrote:
> On Tue, 16 Sep 2014 00:02:27 +0800
> Amos Kong wrote:
>
> > It doesn't save too much cpu time as expected, just a cleanup.
> >
> > Signed-off-by: Amos Kong
> > ---
> > drivers/char/hw_random/core.c | 6 +++---
> > 1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
> > index aa30a25..c591d7e 100644
> > --- a/drivers/char/hw_random/core.c
> > +++ b/drivers/char/hw_random/core.c
> > @@ -270,8 +270,8 @@ static ssize_t hwrng_attr_current_show(struct device
> > *dev,
> > return -ERESTARTSYS;
> > if (current_rng)
> > name = current_rng->name;
> > - ret = snprintf(buf, PAGE_SIZE, "%s\n", name);
> > mutex_unlock(&rng_mutex);
> > + ret = snprintf(buf, PAGE_SIZE, "%s\n", name);
>
> I'm not sure this is safe.
> Name is just a pointer.
> What if the hwrng gets unregistered after unlock and just before the snprintf?
Oh, it points to protected current_rng->name, I will drop this
cleanup. Thanks.
> > return ret;
> > }
> > @@ -284,19 +284,19 @@ static ssize_t hwrng_attr_available_show(struct
> > device *dev,
> > ssize_t ret = 0;
> > struct hwrng *rng;
> >
> > + buf[0] = '\0';
> > err = mutex_lock_interruptible(&rng_mutex);
> > if (err)
> > return -ERESTARTSYS;
> > - buf[0] = '\0';
> > list_for_each_entry(rng, &rng_list, list) {
> > strncat(buf, rng->name, PAGE_SIZE - ret - 1);
> > ret += strlen(rng->name);
> > strncat(buf, " ", PAGE_SIZE - ret - 1);
> > ret++;
> > }
> > + mutex_unlock(&rng_mutex);
> > strncat(buf, "\n", PAGE_SIZE - ret - 1);
> > ret++;
> > - mutex_unlock(&rng_mutex);
> >
> > return ret;
> > }
>
> This looks ok.
>
> --
> Michael
--
Amos.
pgpLebQhrkTiH.pgp
Description: PGP signature
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 2/2] virtio-rng: fix stuck in catting hwrng attributes
CC linux-kernel Original thread: http://comments.gmane.org/gmane.linux.kernel.virtualization/22775 On Mon, Sep 15, 2014 at 06:48:46PM +0200, Radim Krčmář wrote: > 2014-09-14 10:25+0800, Amos Kong: > > On Sun, Sep 14, 2014 at 09:12:08AM +0800, Amos Kong wrote: > > > > ... > > > > > > diff --git a/drivers/char/hw_random/core.c > > > > > > b/drivers/char/hw_random/core.c > > > > > > index c591d7e..b5d1b6f 100644 > > > > > > --- a/drivers/char/hw_random/core.c > > > > > > +++ b/drivers/char/hw_random/core.c > > > > > > @@ -195,8 +195,7 @@ static ssize_t rng_dev_read(struct file *filp, > > > > > > char __user *buf, > > > > > > > > > > > > mutex_unlock(&rng_mutex); > > > > > > > > > > > > - if (need_resched()) > > > > > > - schedule_timeout_interruptible(1); > > > > > > + schedule_timeout_interruptible(10); > > If cond_resched() does not work, it is a bug elsewehere. Thanks for your reply, Jason also told me the TIF_NEED_RESCHED should be set in this case, then need_resched() returns true. I will investigate the issue and reply you later. > > Problem only occurred in non-smp guest, we can improve it to: > > > > if(!is_smp()) > > schedule_timeout_interruptible(10); > > > > is_smp() is only available for arm arch, we need a general one. > > (It is num_online_cpus() > 1.) -- Amos. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
