* [PATCH v2 1/2] virtio_mmio: add cleanup for virtio_mmio_probe
From: weiping zhang @ 2017-12-05 11:57 UTC (permalink / raw)
To: cohuck, mst, jasowang; +Cc: virtualization
In-Reply-To: <cover.1512445595.git.zhangweiping@didichuxing.com>
As mentioned at drivers/base/core.c:
/*
* NOTE: _Never_ directly free @dev after calling this function, even
* if it returned an error! Always use put_device() to give up the
* reference initialized in this function instead.
*/
Normal we do cleanup for @vm_dev by contianer_of(@dev), but in this case
we need release @mem resource from @pdev and vm_dev->base. It make
@pdev->vm_dev.dev.release() too complicated, so put_device just put the
reference of register_virtio_device->device_register->device_initialize
and release all resource in virtio_mmio_probe.
Signed-off-by: weiping zhang <zhangweiping@didichuxing.com>
---
drivers/virtio/virtio_mmio.c | 36 ++++++++++++++++++++++++++++--------
1 file changed, 28 insertions(+), 8 deletions(-)
diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
index 74dc717..f984510 100644
--- a/drivers/virtio/virtio_mmio.c
+++ b/drivers/virtio/virtio_mmio.c
@@ -513,8 +513,10 @@ static int virtio_mmio_probe(struct platform_device *pdev)
return -EBUSY;
vm_dev = devm_kzalloc(&pdev->dev, sizeof(*vm_dev), GFP_KERNEL);
- if (!vm_dev)
- return -ENOMEM;
+ if (!vm_dev) {
+ rc = -ENOMEM;
+ goto free_mem;
+ }
vm_dev->vdev.dev.parent = &pdev->dev;
vm_dev->vdev.dev.release = virtio_mmio_release_dev_empty;
@@ -524,14 +526,17 @@ static int virtio_mmio_probe(struct platform_device *pdev)
spin_lock_init(&vm_dev->lock);
vm_dev->base = devm_ioremap(&pdev->dev, mem->start, resource_size(mem));
- if (vm_dev->base == NULL)
- return -EFAULT;
+ if (vm_dev->base == NULL) {
+ rc = -EFAULT;
+ goto free_vmdev;
+ }
/* Check magic value */
magic = readl(vm_dev->base + VIRTIO_MMIO_MAGIC_VALUE);
if (magic != ('v' | 'i' << 8 | 'r' << 16 | 't' << 24)) {
dev_warn(&pdev->dev, "Wrong magic value 0x%08lx!\n", magic);
- return -ENODEV;
+ rc = -ENODEV;
+ goto unmap;
}
/* Check device version */
@@ -539,7 +544,8 @@ static int virtio_mmio_probe(struct platform_device *pdev)
if (vm_dev->version < 1 || vm_dev->version > 2) {
dev_err(&pdev->dev, "Version %ld not supported!\n",
vm_dev->version);
- return -ENXIO;
+ rc = -ENXIO;
+ goto unmap;
}
vm_dev->vdev.id.device = readl(vm_dev->base + VIRTIO_MMIO_DEVICE_ID);
@@ -548,7 +554,8 @@ static int virtio_mmio_probe(struct platform_device *pdev)
* virtio-mmio device with an ID 0 is a (dummy) placeholder
* with no function. End probing now with no error reported.
*/
- return -ENODEV;
+ rc = -ENODEV;
+ goto unmap;
}
vm_dev->vdev.id.vendor = readl(vm_dev->base + VIRTIO_MMIO_VENDOR_ID);
@@ -573,7 +580,20 @@ static int virtio_mmio_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, vm_dev);
- return register_virtio_device(&vm_dev->vdev);
+ rc = register_virtio_device(&vm_dev->vdev);
+ if (rc)
+ goto put_dev;
+ return 0;
+put_dev:
+ put_device(&vm_dev->vdev.dev);
+unmap:
+ iounmap(vm_dev->base);
+free_mem:
+ devm_release_mem_region(&pdev->dev, mem->start,
+ resource_size(mem));
+free_vmdev:
+ devm_kfree(&pdev->dev, vm_dev);
+ return rc;
}
static int virtio_mmio_remove(struct platform_device *pdev)
--
2.9.4
^ permalink raw reply related
* [PATCH v2 0/2] Add cleanup for virtio_mmio driver
From: weiping zhang @ 2017-12-05 11:56 UTC (permalink / raw)
To: cohuck, mst, jasowang; +Cc: virtualization
this patchset try to add cleanup for virtio_mmio driver, include
virtio_mmio_probe and virtio_mmio_remove
weiping zhang (2):
virtio_mmio: add cleanup for virtio_mmio_probe
virtio_mmio: add cleanup for virtio_mmio_remove
drivers/virtio/virtio_mmio.c | 43 +++++++++++++++++++++++++++++++++++--------
1 file changed, 35 insertions(+), 8 deletions(-)
--
2.9.4
^ permalink raw reply
* Re: [RFC] virtio-net: help live migrate SR-IOV devices
From: achiad shochat @ 2017-12-05 9:59 UTC (permalink / raw)
To: Alexander Duyck
Cc: Jakub Kicinski, Hannes Frederic Sowa, Sridhar Samudrala,
Michael S. Tsirkin, virtualization, Shannon Nelson, Achiad,
Peter Waskiewicz Jr, netdev, Singhai, Anjali, Andy Gospodarek,
Or Gerlitz
In-Reply-To: <CAKgT0Uf5gta+CNM3ReybV3qNMJXqoT9BHvMjKRbd=PYOPQrQaA@mail.gmail.com>
On 4 December 2017 at 18:30, Alexander Duyck <alexander.duyck@gmail.com> wrote:
> On Mon, Dec 4, 2017 at 1:51 AM, achiad shochat
> <achiad.mellanox@gmail.com> wrote:
>> On 3 December 2017 at 19:35, Stephen Hemminger
>> <stephen@networkplumber.org> wrote:
>>> On Sun, 3 Dec 2017 11:14:37 +0200
>>> achiad shochat <achiad.mellanox@gmail.com> wrote:
>>>
>>>> On 3 December 2017 at 07:05, Michael S. Tsirkin <mst@redhat.com> wrote:
>>>> > On Fri, Dec 01, 2017 at 12:08:59PM -0800, Shannon Nelson wrote:
>>>> >> On 11/30/2017 6:11 AM, Michael S. Tsirkin wrote:
>>>> >> > On Thu, Nov 30, 2017 at 10:08:45AM +0200, achiad shochat wrote:
>>>> >> > > Re. problem #2:
>>>> >> > > Indeed the best way to address it seems to be to enslave the VF driver
>>>> >> > > netdev under a persistent anchor netdev.
>>>> >> > > And it's indeed desired to allow (but not enforce) PV netdev and VF
>>>> >> > > netdev to work in conjunction.
>>>> >> > > And it's indeed desired that this enslavement logic work out-of-the box.
>>>> >> > > But in case of PV+VF some configurable policies must be in place (and
>>>> >> > > they'd better be generic rather than differ per PV technology).
>>>> >> > > For example - based on which characteristics should the PV+VF coupling
>>>> >> > > be done? netvsc uses MAC address, but that might not always be the
>>>> >> > > desire.
>>>> >> >
>>>> >> > It's a policy but not guest userspace policy.
>>>> >> >
>>>> >> > The hypervisor certainly knows.
>>>> >> >
>>>> >> > Are you concerned that someone might want to create two devices with the
>>>> >> > same MAC for an unrelated reason? If so, hypervisor could easily set a
>>>> >> > flag in the virtio device to say "this is a backup, use MAC to find
>>>> >> > another device".
>>>> >>
>>>> >> This is something I was going to suggest: a flag or other configuration on
>>>> >> the virtio device to help control how this new feature is used. I can
>>>> >> imagine this might be useful to control from either the hypervisor side or
>>>> >> the VM side.
>>>> >>
>>>> >> The hypervisor might want to (1) disable it (force it off), (2) enable it
>>>> >> for VM choice, or (3) force it on for the VM. In case (2), the VM might be
>>>> >> able to chose whether it wants to make use of the feature, or stick with the
>>>> >> bonding solution.
>>>> >>
>>>> >> Either way, the kernel is making a feature available, and the user (VM or
>>>> >> hypervisor) is able to control it by selecting the feature based on the
>>>> >> policy desired.
>>>> >>
>>>> >> sln
>>>> >
>>>> > I'm not sure what's the feature that is available here.
>>>> >
>>>> > I saw this as a flag that says "this device shares backend with another
>>>> > network device which can be found using MAC, and that backend should be
>>>> > preferred". kernel then forces configuration which uses that other
>>>> > backend - as long as it exists.
>>>> >
>>>> > However, please Cc virtio-dev mailing list if we are doing this since
>>>> > this is a spec extension.
>>>> >
>>>> > --
>>>> > MST
>>>>
>>>>
>>>> Can someone please explain why assume a virtio device is there at all??
>>>> I specified a case where there isn't any.
>
> Migrating without any virtual device is going to be extremely
> challenging, especially in any kind of virtualization setup where the
> hosts are not homogeneous. By providing a virtio interface you can
> guarantee that at least 1 network interface is available on any given
> host, and then fail over to that as the least common denominator for
> any migration.
>
I am not sure why you think it is going to be so challenging.
Are you referring to preserving the pass-through device driver state
(RX/TX rings)?
I do not think we should preserve them, we can simply teardown the
whole VF netdev (since we have a parent netdev as application
interface).
The downtime impact will be negligible.
>>>> I second Jacob - having a netdev of one device driver enslave a netdev
>>>> of another device driver is an awkward a-symmetric model.
>>>> Regardless of whether they share the same backend device.
>>>> Only I am not sure the Linux Bond is the right choice.
>>>> e.g one may well want to use the virtio device also when the
>>>> pass-through device is available, e.g for multicasts, east-west
>>>> traffic, etc.
>>>> I'm not sure the Linux Bond fits that functionality.
>>>> And, as I hear in this thread, it is hard to make it work out of the box.
>>>> So I think the right thing would be to write a new dedicated module
>>>> for this purpose.
>
> This part I can sort of agree with. What if we were to look at
> providing a way to somehow advertise that the two devices were meant
> to be boded for virtualization purposes? For now lets call it a
> "virt-bond". Basically we could look at providing a means for virtio
> and VF drivers to advertise that they want this sort of bond. Then it
> would just be a matter of providing some sort of side channel to
> indicate where you want things like multicast/broadcast/east-west
> traffic to go.
>
I like this approach.
>>>> Re policy -
>>>> Indeed the HV can request a policy from the guest but that's not a
>>>> claim for the virtio device enslaving the pass-through device.
>>>> Any policy can be queried by the upper enslaving device.
>>>>
>>>> Bottom line - I do not see a single reason to have the virtio netdev
>>>> (nor netvsc or any other PV netdev) enslave another netdev by itself.
>>>> If we'd do it right with netvsc from the beginning we wouldn't need
>>>> this discussion at all...
>>>
>>> There are several issues with transparent migration.
>>> The first is that the SR-IOV device needs to be shut off for earlier
>>> in the migration process.
>>
>> That's not a given fact.
>> It's due to the DMA and it should be solve anyway.
>> Please read my first reply in this thread.
>
> For now it is a fact. We would need to do a drastic rewrite of the DMA
> API in the guest/host/QEMU/IOMMU in order to avoid it for now. So as a
> first step I would say we should look at using this bonding type
> solution. Being able to defer the VF eviction could be a next step for
> all this as it would allow for much better performance, but we still
> have too many cases where the VF might not be there after a migration.
>
Why would we need such a drastic rewrite?
Why would a simple Read-DontModify-Write (to mark the page as dirty)
by the VF driver not do the job?
Anyway, if you have a generic virtual parent netdev handling that can
be handled orthogonally.
>>> Next, the SR-IOV device in the migrated go guest environment maybe different.
>>> It might not exist at all, it might be at a different PCI address, or it
>>> could even be a different vendor/speed/model.
>>> Keeping a virtual network device around allows persisting the connectivity,
>>> during the process.
>>
>> Right, but that virtual device must not relate to any para-virt
>> specific technology (not netvsc, nor virtio).
>> Again, it seems you did not read my first reply.
>
> I would agree with the need to make this agnostic. Maybe we could look
> at the current netvsc solution and find a way to make it generic so it
> could be applied to any combination of paravirtual interface and PF.
Agree. That's should be the approach IMO.
Then we'll have a single solution for both netvsc and virtio (and any
other PV device).
And we could handle the VF DMA dirt issue agnostically.
^ permalink raw reply
* Re: [PATCH] virtio_mmio: add cleanup for virtio_mmio_probe
From: weiping zhang @ 2017-12-05 1:36 UTC (permalink / raw)
To: Cornelia Huck; +Cc: virtualization, mst
In-Reply-To: <20171204112455.33dad9c2.cohuck@redhat.com>
2017-12-04 18:24 GMT+08:00 Cornelia Huck <cohuck@redhat.com>:
> On Sat, 2 Dec 2017 01:51:40 +0800
> weiping zhang <zwp10758@gmail.com> wrote:
>
>> cleanup all resource allocated by virtio_mmio_probe.
>>
>> Signed-off-by: weiping zhang <zhangweiping@didichuxing.com>
>> ---
>> drivers/virtio/virtio_mmio.c | 34 ++++++++++++++++++++++++++--------
>> 1 file changed, 26 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
>> index 74dc717..3fd0e66 100644
>> --- a/drivers/virtio/virtio_mmio.c
>> +++ b/drivers/virtio/virtio_mmio.c
>> @@ -513,8 +513,10 @@ static int virtio_mmio_probe(struct platform_device *pdev)
>> return -EBUSY;
>>
>> vm_dev = devm_kzalloc(&pdev->dev, sizeof(*vm_dev), GFP_KERNEL);
>> - if (!vm_dev)
>> - return -ENOMEM;
>> + if (!vm_dev) {
>> + rc = -ENOMEM;
>
> Touching this would be a good time to remove the extra space in front
> of the -ENOMEM :)
Thanks, I fix this at V2.
>
>> + goto free_mem;
>> + }
>>
>> vm_dev->vdev.dev.parent = &pdev->dev;
>> vm_dev->vdev.dev.release = virtio_mmio_release_dev_empty;
>
> (...)
>
>> @@ -573,7 +580,18 @@ static int virtio_mmio_probe(struct platform_device *pdev)
>>
>> platform_set_drvdata(pdev, vm_dev);
>>
>> - return register_virtio_device(&vm_dev->vdev);
>> + rc = register_virtio_device(&vm_dev->vdev);
>> + if (rc)
>> + goto unmap;
>> + return 0;
>> +unmap:
>> + iounmap(vm_dev->base);
>> +free_mem:
>> + devm_release_mem_region(&pdev->dev, mem->start,
>> + resource_size(mem));
>> +free_vmdev:
>> + devm_kfree(&pdev->dev, vm_dev);
>
> I think this is problematic as vm_dev embeds a struct device (via
> embedding a struct virtio_device). I think the right way to do this is
> - call this only if register_virtio_device() has not been called
> - put the devm_kfree() into the ->release callback for the
> virtio_device (IOW, replace virtio_mmio_release_dev_empty() with a
> function that calls this)
> - do a put_device() if register_virtio_device() failed
>
> I might be missing some interaction between the usual driver model
> handling and devm resources, though.
>
if fail in virtio_mmio_probe, we need free @mem of @pdev, vdev->base,
so I prefer clean all this resource in virtio_mmio_probe itself, keep
vdev.dev.release do nothing.
also we need release these resources at virtio_mmio_remove.
I'll send V2.
--
thanks a ton
^ permalink raw reply
* Re: [PATCH] virtio: release virtio index when fail to device_register
From: weiping zhang @ 2017-12-05 1:30 UTC (permalink / raw)
To: Cornelia Huck; +Cc: virtualization, mst
In-Reply-To: <20171204103842.3d8709a8.cohuck@redhat.com>
2017-12-04 17:38 GMT+08:00 Cornelia Huck <cohuck@redhat.com>:
> On Sat, 2 Dec 2017 00:55:39 +0800
> weiping zhang <zwp10758@gmail.com> wrote:
>
>> On Wed, Nov 29, 2017 at 10:50:44AM +0100, Cornelia Huck wrote:
>
>> > We hold an extra reference to the struct device, even after a failed
>> > register, and it is the responsibility of the caller to give up that
>> > reference once no longer needed. As callers toregister_virtio_device()
>> > embed the struct virtio_device, it needs to be their responsibility.
>> > Looking at the existing callers,
>> >
>> > - ccw does a put_device
>> > - pci, mmio and remoteproc do nothing, causing a leak
>> > - vop does a free on the embedding structure, which is a big no-no
>> >
>> > Thoughts?
>> Sorry to relay late and thanks for your review.
>> Do you mean the "extra reference to the struct device" caused by the
>> following code?
>>
>> err = device_register(&dev->dev);
>> device_add(dev)
>> get_device(dev)
>> If I'm understand right, I think there is no extra reference if we fail
>> virtio_register_device, because if device_register we don't get a
>> reference.
>
> The device_initialize() already gives you a reference. If device_add()
> fails, it has cleaned up any additional reference it might have
> obtained, but the initial reference is still there and needs to be
> released by the caller.
Thanks your clarify, I also notice the comments at device_register,
device_initialize, device_add,
* NOTE: _Never_ directly free @dev after calling this function, even
* if it returned an error! Always use put_device() to give up the
* reference initialized in this function instead.
--
Thanks
weiping
^ permalink raw reply
* Re: [RFC] virtio-net: help live migrate SR-IOV devices
From: Alexander Duyck @ 2017-12-04 16:30 UTC (permalink / raw)
To: achiad shochat
Cc: Jakub Kicinski, Hannes Frederic Sowa, Sridhar Samudrala,
Michael S. Tsirkin, virtualization, Shannon Nelson, Achiad,
Peter Waskiewicz Jr, netdev, Singhai, Anjali, Andy Gospodarek,
Or Gerlitz
In-Reply-To: <CAEHy93LEr5h81KGfJBr5Z4ZKaeWEWwQL1LYfZNXoVF_HB6TQGw@mail.gmail.com>
On Mon, Dec 4, 2017 at 1:51 AM, achiad shochat
<achiad.mellanox@gmail.com> wrote:
> On 3 December 2017 at 19:35, Stephen Hemminger
> <stephen@networkplumber.org> wrote:
>> On Sun, 3 Dec 2017 11:14:37 +0200
>> achiad shochat <achiad.mellanox@gmail.com> wrote:
>>
>>> On 3 December 2017 at 07:05, Michael S. Tsirkin <mst@redhat.com> wrote:
>>> > On Fri, Dec 01, 2017 at 12:08:59PM -0800, Shannon Nelson wrote:
>>> >> On 11/30/2017 6:11 AM, Michael S. Tsirkin wrote:
>>> >> > On Thu, Nov 30, 2017 at 10:08:45AM +0200, achiad shochat wrote:
>>> >> > > Re. problem #2:
>>> >> > > Indeed the best way to address it seems to be to enslave the VF driver
>>> >> > > netdev under a persistent anchor netdev.
>>> >> > > And it's indeed desired to allow (but not enforce) PV netdev and VF
>>> >> > > netdev to work in conjunction.
>>> >> > > And it's indeed desired that this enslavement logic work out-of-the box.
>>> >> > > But in case of PV+VF some configurable policies must be in place (and
>>> >> > > they'd better be generic rather than differ per PV technology).
>>> >> > > For example - based on which characteristics should the PV+VF coupling
>>> >> > > be done? netvsc uses MAC address, but that might not always be the
>>> >> > > desire.
>>> >> >
>>> >> > It's a policy but not guest userspace policy.
>>> >> >
>>> >> > The hypervisor certainly knows.
>>> >> >
>>> >> > Are you concerned that someone might want to create two devices with the
>>> >> > same MAC for an unrelated reason? If so, hypervisor could easily set a
>>> >> > flag in the virtio device to say "this is a backup, use MAC to find
>>> >> > another device".
>>> >>
>>> >> This is something I was going to suggest: a flag or other configuration on
>>> >> the virtio device to help control how this new feature is used. I can
>>> >> imagine this might be useful to control from either the hypervisor side or
>>> >> the VM side.
>>> >>
>>> >> The hypervisor might want to (1) disable it (force it off), (2) enable it
>>> >> for VM choice, or (3) force it on for the VM. In case (2), the VM might be
>>> >> able to chose whether it wants to make use of the feature, or stick with the
>>> >> bonding solution.
>>> >>
>>> >> Either way, the kernel is making a feature available, and the user (VM or
>>> >> hypervisor) is able to control it by selecting the feature based on the
>>> >> policy desired.
>>> >>
>>> >> sln
>>> >
>>> > I'm not sure what's the feature that is available here.
>>> >
>>> > I saw this as a flag that says "this device shares backend with another
>>> > network device which can be found using MAC, and that backend should be
>>> > preferred". kernel then forces configuration which uses that other
>>> > backend - as long as it exists.
>>> >
>>> > However, please Cc virtio-dev mailing list if we are doing this since
>>> > this is a spec extension.
>>> >
>>> > --
>>> > MST
>>>
>>>
>>> Can someone please explain why assume a virtio device is there at all??
>>> I specified a case where there isn't any.
Migrating without any virtual device is going to be extremely
challenging, especially in any kind of virtualization setup where the
hosts are not homogeneous. By providing a virtio interface you can
guarantee that at least 1 network interface is available on any given
host, and then fail over to that as the least common denominator for
any migration.
>>> I second Jacob - having a netdev of one device driver enslave a netdev
>>> of another device driver is an awkward a-symmetric model.
>>> Regardless of whether they share the same backend device.
>>> Only I am not sure the Linux Bond is the right choice.
>>> e.g one may well want to use the virtio device also when the
>>> pass-through device is available, e.g for multicasts, east-west
>>> traffic, etc.
>>> I'm not sure the Linux Bond fits that functionality.
>>> And, as I hear in this thread, it is hard to make it work out of the box.
>>> So I think the right thing would be to write a new dedicated module
>>> for this purpose.
This part I can sort of agree with. What if we were to look at
providing a way to somehow advertise that the two devices were meant
to be boded for virtualization purposes? For now lets call it a
"virt-bond". Basically we could look at providing a means for virtio
and VF drivers to advertise that they want this sort of bond. Then it
would just be a matter of providing some sort of side channel to
indicate where you want things like multicast/broadcast/east-west
traffic to go.
>>> Re policy -
>>> Indeed the HV can request a policy from the guest but that's not a
>>> claim for the virtio device enslaving the pass-through device.
>>> Any policy can be queried by the upper enslaving device.
>>>
>>> Bottom line - I do not see a single reason to have the virtio netdev
>>> (nor netvsc or any other PV netdev) enslave another netdev by itself.
>>> If we'd do it right with netvsc from the beginning we wouldn't need
>>> this discussion at all...
>>
>> There are several issues with transparent migration.
>> The first is that the SR-IOV device needs to be shut off for earlier
>> in the migration process.
>
> That's not a given fact.
> It's due to the DMA and it should be solve anyway.
> Please read my first reply in this thread.
For now it is a fact. We would need to do a drastic rewrite of the DMA
API in the guest/host/QEMU/IOMMU in order to avoid it for now. So as a
first step I would say we should look at using this bonding type
solution. Being able to defer the VF eviction could be a next step for
all this as it would allow for much better performance, but we still
have too many cases where the VF might not be there after a migration.
>> Next, the SR-IOV device in the migrated go guest environment maybe different.
>> It might not exist at all, it might be at a different PCI address, or it
>> could even be a different vendor/speed/model.
>> Keeping a virtual network device around allows persisting the connectivity,
>> during the process.
>
> Right, but that virtual device must not relate to any para-virt
> specific technology (not netvsc, nor virtio).
> Again, it seems you did not read my first reply.
I would agree with the need to make this agnostic. Maybe we could look
at the current netvsc solution and find a way to make it generic so it
could be applied to any combination of paravirtual interface and PF.
^ permalink raw reply
* [PULL] vhost: cleanups and fixes
From: Michael S. Tsirkin @ 2017-12-04 13:25 UTC (permalink / raw)
To: Linus Torvalds
Cc: mhocko, kvm, mst, penguin-kernel, netdev, linux-kernel, stable,
virtualization, zhangweiping, marcandre.lureau, jstancek
The following changes since commit c1d0c3f623ada808904dec676da0126f5b800630:
fw_cfg: fix the command line module name (2017-11-14 23:57:40 +0200)
are available in the Git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git tags/for_linus
for you to fetch changes up to d9e427f6ab8142d6868eb719e6a7851aafea56b6:
virtio_balloon: fix increment of vb->num_pfns in fill_balloon() (2017-12-01 16:55:45 +0200)
----------------------------------------------------------------
virtio,qemu: bugfixes
A couple of bugfixes that just became ready.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
----------------------------------------------------------------
Jan Stancek (1):
virtio_balloon: fix increment of vb->num_pfns in fill_balloon()
Marc-André Lureau (1):
fw_cfg: fix driver remove
weiping zhang (1):
virtio: release virtio index when fail to device_register
drivers/firmware/qemu_fw_cfg.c | 3 ++-
drivers/virtio/virtio.c | 2 ++
drivers/virtio/virtio_balloon.c | 3 +--
3 files changed, 5 insertions(+), 3 deletions(-)
^ permalink raw reply
* Re: [PATCH] virtio_mmio: add cleanup for virtio_mmio_probe
From: Cornelia Huck @ 2017-12-04 10:24 UTC (permalink / raw)
To: weiping zhang; +Cc: virtualization, mst
In-Reply-To: <20171201175140.GA19140@localhost.didichuxing.com>
On Sat, 2 Dec 2017 01:51:40 +0800
weiping zhang <zwp10758@gmail.com> wrote:
> cleanup all resource allocated by virtio_mmio_probe.
>
> Signed-off-by: weiping zhang <zhangweiping@didichuxing.com>
> ---
> drivers/virtio/virtio_mmio.c | 34 ++++++++++++++++++++++++++--------
> 1 file changed, 26 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
> index 74dc717..3fd0e66 100644
> --- a/drivers/virtio/virtio_mmio.c
> +++ b/drivers/virtio/virtio_mmio.c
> @@ -513,8 +513,10 @@ static int virtio_mmio_probe(struct platform_device *pdev)
> return -EBUSY;
>
> vm_dev = devm_kzalloc(&pdev->dev, sizeof(*vm_dev), GFP_KERNEL);
> - if (!vm_dev)
> - return -ENOMEM;
> + if (!vm_dev) {
> + rc = -ENOMEM;
Touching this would be a good time to remove the extra space in front
of the -ENOMEM :)
> + goto free_mem;
> + }
>
> vm_dev->vdev.dev.parent = &pdev->dev;
> vm_dev->vdev.dev.release = virtio_mmio_release_dev_empty;
(...)
> @@ -573,7 +580,18 @@ static int virtio_mmio_probe(struct platform_device *pdev)
>
> platform_set_drvdata(pdev, vm_dev);
>
> - return register_virtio_device(&vm_dev->vdev);
> + rc = register_virtio_device(&vm_dev->vdev);
> + if (rc)
> + goto unmap;
> + return 0;
> +unmap:
> + iounmap(vm_dev->base);
> +free_mem:
> + devm_release_mem_region(&pdev->dev, mem->start,
> + resource_size(mem));
> +free_vmdev:
> + devm_kfree(&pdev->dev, vm_dev);
I think this is problematic as vm_dev embeds a struct device (via
embedding a struct virtio_device). I think the right way to do this is
- call this only if register_virtio_device() has not been called
- put the devm_kfree() into the ->release callback for the
virtio_device (IOW, replace virtio_mmio_release_dev_empty() with a
function that calls this)
- do a put_device() if register_virtio_device() failed
I might be missing some interaction between the usual driver model
handling and devm resources, though.
> + return rc;
> }
>
> static int virtio_mmio_remove(struct platform_device *pdev)
^ permalink raw reply
* Re: [RFC] virtio-net: help live migrate SR-IOV devices
From: achiad shochat @ 2017-12-04 9:51 UTC (permalink / raw)
To: Stephen Hemminger
Cc: Jakub Kicinski, Hannes Frederic Sowa, Sridhar Samudrala,
Michael S. Tsirkin, virtualization, Shannon Nelson, Achiad,
Peter Waskiewicz Jr, netdev, Singhai, Anjali, Andy Gospodarek,
Or Gerlitz
In-Reply-To: <20171203093524.26491cd0@xeon-e3>
On 3 December 2017 at 19:35, Stephen Hemminger
<stephen@networkplumber.org> wrote:
> On Sun, 3 Dec 2017 11:14:37 +0200
> achiad shochat <achiad.mellanox@gmail.com> wrote:
>
>> On 3 December 2017 at 07:05, Michael S. Tsirkin <mst@redhat.com> wrote:
>> > On Fri, Dec 01, 2017 at 12:08:59PM -0800, Shannon Nelson wrote:
>> >> On 11/30/2017 6:11 AM, Michael S. Tsirkin wrote:
>> >> > On Thu, Nov 30, 2017 at 10:08:45AM +0200, achiad shochat wrote:
>> >> > > Re. problem #2:
>> >> > > Indeed the best way to address it seems to be to enslave the VF driver
>> >> > > netdev under a persistent anchor netdev.
>> >> > > And it's indeed desired to allow (but not enforce) PV netdev and VF
>> >> > > netdev to work in conjunction.
>> >> > > And it's indeed desired that this enslavement logic work out-of-the box.
>> >> > > But in case of PV+VF some configurable policies must be in place (and
>> >> > > they'd better be generic rather than differ per PV technology).
>> >> > > For example - based on which characteristics should the PV+VF coupling
>> >> > > be done? netvsc uses MAC address, but that might not always be the
>> >> > > desire.
>> >> >
>> >> > It's a policy but not guest userspace policy.
>> >> >
>> >> > The hypervisor certainly knows.
>> >> >
>> >> > Are you concerned that someone might want to create two devices with the
>> >> > same MAC for an unrelated reason? If so, hypervisor could easily set a
>> >> > flag in the virtio device to say "this is a backup, use MAC to find
>> >> > another device".
>> >>
>> >> This is something I was going to suggest: a flag or other configuration on
>> >> the virtio device to help control how this new feature is used. I can
>> >> imagine this might be useful to control from either the hypervisor side or
>> >> the VM side.
>> >>
>> >> The hypervisor might want to (1) disable it (force it off), (2) enable it
>> >> for VM choice, or (3) force it on for the VM. In case (2), the VM might be
>> >> able to chose whether it wants to make use of the feature, or stick with the
>> >> bonding solution.
>> >>
>> >> Either way, the kernel is making a feature available, and the user (VM or
>> >> hypervisor) is able to control it by selecting the feature based on the
>> >> policy desired.
>> >>
>> >> sln
>> >
>> > I'm not sure what's the feature that is available here.
>> >
>> > I saw this as a flag that says "this device shares backend with another
>> > network device which can be found using MAC, and that backend should be
>> > preferred". kernel then forces configuration which uses that other
>> > backend - as long as it exists.
>> >
>> > However, please Cc virtio-dev mailing list if we are doing this since
>> > this is a spec extension.
>> >
>> > --
>> > MST
>>
>>
>> Can someone please explain why assume a virtio device is there at all??
>> I specified a case where there isn't any.
>>
>> I second Jacob - having a netdev of one device driver enslave a netdev
>> of another device driver is an awkward a-symmetric model.
>> Regardless of whether they share the same backend device.
>> Only I am not sure the Linux Bond is the right choice.
>> e.g one may well want to use the virtio device also when the
>> pass-through device is available, e.g for multicasts, east-west
>> traffic, etc.
>> I'm not sure the Linux Bond fits that functionality.
>> And, as I hear in this thread, it is hard to make it work out of the box.
>> So I think the right thing would be to write a new dedicated module
>> for this purpose.
>>
>> Re policy -
>> Indeed the HV can request a policy from the guest but that's not a
>> claim for the virtio device enslaving the pass-through device.
>> Any policy can be queried by the upper enslaving device.
>>
>> Bottom line - I do not see a single reason to have the virtio netdev
>> (nor netvsc or any other PV netdev) enslave another netdev by itself.
>> If we'd do it right with netvsc from the beginning we wouldn't need
>> this discussion at all...
>
> There are several issues with transparent migration.
> The first is that the SR-IOV device needs to be shut off for earlier
> in the migration process.
That's not a given fact.
It's due to the DMA and it should be solve anyway.
Please read my first reply in this thread.
> Next, the SR-IOV device in the migrated go guest environment maybe different.
> It might not exist at all, it might be at a different PCI address, or it
> could even be a different vendor/speed/model.
> Keeping a virtual network device around allows persisting the connectivity,
> during the process.
Right, but that virtual device must not relate to any para-virt
specific technology (not netvsc, nor virtio).
Again, it seems you did not read my first reply.
^ permalink raw reply
* Re: [PATCH] virtio: release virtio index when fail to device_register
From: Cornelia Huck @ 2017-12-04 9:38 UTC (permalink / raw)
To: weiping zhang; +Cc: virtualization, mst
In-Reply-To: <20171201165539.GA11865@localhost.didichuxing.com>
On Sat, 2 Dec 2017 00:55:39 +0800
weiping zhang <zwp10758@gmail.com> wrote:
> On Wed, Nov 29, 2017 at 10:50:44AM +0100, Cornelia Huck wrote:
> > We hold an extra reference to the struct device, even after a failed
> > register, and it is the responsibility of the caller to give up that
> > reference once no longer needed. As callers toregister_virtio_device()
> > embed the struct virtio_device, it needs to be their responsibility.
> > Looking at the existing callers,
> >
> > - ccw does a put_device
> > - pci, mmio and remoteproc do nothing, causing a leak
> > - vop does a free on the embedding structure, which is a big no-no
> >
> > Thoughts?
> Sorry to relay late and thanks for your review.
> Do you mean the "extra reference to the struct device" caused by the
> following code?
>
> err = device_register(&dev->dev);
> device_add(dev)
> get_device(dev)
> If I'm understand right, I think there is no extra reference if we fail
> virtio_register_device, because if device_register we don't get a
> reference.
The device_initialize() already gives you a reference. If device_add()
fails, it has cleaned up any additional reference it might have
obtained, but the initial reference is still there and needs to be
released by the caller.
^ permalink raw reply
* Re: [PATCH 1/3] vhost: fix skb leak in handle_rx()
From: Jason Wang @ 2017-12-04 7:18 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: mjrosato, netdev, wexu, linux-kernel, virtualization
In-Reply-To: <20171201163728-mutt-send-email-mst@kernel.org>
On 2017年12月01日 22:37, Michael S. Tsirkin wrote:
> On Fri, Dec 01, 2017 at 03:11:05PM +0800, Jason Wang wrote:
>>
>> On 2017年12月01日 13:54, wexu@redhat.com wrote:
>>> From: Wei Xu <wexu@redhat.com>
>>>
>>> Matthew found a roughly 40% tcp throughput regression with commit
>>> c67df11f(vhost_net: try batch dequing from skb array) as discussed
>>> in the following thread:
>>> https://www.mail-archive.com/netdev@vger.kernel.org/msg187936.html
>>>
>>> Eventually we figured out that it was a skb leak in handle_rx()
>>> when sending packets to the VM. This usually happens when a guest
>>> can not drain out vq as fast as vhost fills in, afterwards it sets
>>> off the traffic jam and leaks skb(s) which occurs as no headcount
>>> to send on the vq from vhost side.
>>>
>>> This can be avoided by making sure we have got enough headcount
>>> before actually consuming a skb from the batched rx array while
>>> transmitting, which is simply done by moving checking the zero
>>> headcount a bit ahead.
>>>
>>> Signed-off-by: Wei Xu <wexu@redhat.com>
>>> Reported-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com>
>>> ---
>>> drivers/vhost/net.c | 20 ++++++++++----------
>>> 1 file changed, 10 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
>>> index 8d626d7..c7bdeb6 100644
>>> --- a/drivers/vhost/net.c
>>> +++ b/drivers/vhost/net.c
>>> @@ -778,16 +778,6 @@ static void handle_rx(struct vhost_net *net)
>>> /* On error, stop handling until the next kick. */
>>> if (unlikely(headcount < 0))
>>> goto out;
>>> - if (nvq->rx_array)
>>> - msg.msg_control = vhost_net_buf_consume(&nvq->rxq);
>>> - /* On overrun, truncate and discard */
>>> - if (unlikely(headcount > UIO_MAXIOV)) {
>>> - iov_iter_init(&msg.msg_iter, READ, vq->iov, 1, 1);
>>> - err = sock->ops->recvmsg(sock, &msg,
>>> - 1, MSG_DONTWAIT | MSG_TRUNC);
>>> - pr_debug("Discarded rx packet: len %zd\n", sock_len);
>>> - continue;
>>> - }
>>> /* OK, now we need to know about added descriptors. */
>>> if (!headcount) {
>>> if (unlikely(vhost_enable_notify(&net->dev, vq))) {
>>> @@ -800,6 +790,16 @@ static void handle_rx(struct vhost_net *net)
>>> * they refilled. */
>>> goto out;
>>> }
>>> + if (nvq->rx_array)
>>> + msg.msg_control = vhost_net_buf_consume(&nvq->rxq);
>>> + /* On overrun, truncate and discard */
>>> + if (unlikely(headcount > UIO_MAXIOV)) {
>>> + iov_iter_init(&msg.msg_iter, READ, vq->iov, 1, 1);
>>> + err = sock->ops->recvmsg(sock, &msg,
>>> + 1, MSG_DONTWAIT | MSG_TRUNC);
>>> + pr_debug("Discarded rx packet: len %zd\n", sock_len);
>>> + continue;
>>> + }
>>> /* We don't need to be notified again. */
>>> iov_iter_init(&msg.msg_iter, READ, vq->iov, in, vhost_len);
>>> fixup = msg.msg_iter;
>> I suggest to reorder this patch to 3/3.
>>
>> Thanks
> Why? This doesn't cause any new leaks, does it?
>
It doesn't, just think it can ease the downstream back porting in case
patch 2-3 were missed if somebody did a bisect and just backport patch 1.
Thanks
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH v18 10/10] virtio-balloon: don't report free pages when page poisoning is enabled
From: Wei Wang @ 2017-12-04 5:39 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: yang.zhang.wz, kvm, penguin-kernel, liliang.opensource,
qemu-devel, virtualization, linux-mm, aarcange, virtio-dev,
mawilcox, willy, quan.xu, nilal, riel, cornelia.huck, mhocko,
linux-kernel, amit.shah, pbonzini, akpm, mgorman
In-Reply-To: <20171201173951-mutt-send-email-mst@kernel.org>
On 12/01/2017 11:49 PM, Michael S. Tsirkin wrote:
> On Wed, Nov 29, 2017 at 09:55:26PM +0800, Wei Wang wrote:
>> The guest free pages should not be discarded by the live migration thread
>> when page poisoning is enabled with PAGE_POISONING_NO_SANITY=n, because
>> skipping the transfer of such poisoned free pages will trigger false
>> positive when new pages are allocated and checked on the destination.
>> This patch skips the reporting of free pages in the above case.
>>
>> Reported-by: Michael S. Tsirkin <mst@redhat.com>
>> Signed-off-by: Wei Wang <wei.w.wang@intel.com>
>> Cc: Michal Hocko <mhocko@suse.com>
>> ---
>> drivers/virtio/virtio_balloon.c | 4 +++-
>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
>> index 035bd3a..6ac4cff 100644
>> --- a/drivers/virtio/virtio_balloon.c
>> +++ b/drivers/virtio/virtio_balloon.c
>> @@ -652,7 +652,9 @@ static void report_free_page(struct work_struct *work)
>> /* Start by sending the obtained cmd id to the host with an outbuf */
>> send_one_desc(vb, vb->free_page_vq, virt_to_phys(&vb->start_cmd_id),
>> sizeof(uint32_t), false, true, false);
>> - walk_free_mem_block(vb, 0, &virtio_balloon_send_free_pages);
>> + if (!(page_poisoning_enabled() &&
>> + !IS_ENABLED(CONFIG_PAGE_POISONING_NO_SANITY)))
>> + walk_free_mem_block(vb, 0, &virtio_balloon_send_free_pages);
>> /*
>> * End by sending the stop id to the host with an outbuf. Use the
>> * non-batching mode here to trigger a kick after adding the stop id.
> PAGE_POISONING_ZERO is actually OK.
I think the 0-filled pages still need to be sent. If the host on the
destination doesn't use PAGE_POISONING_ZERO, then the pages it offers to
the guest on the destination may have non-0 values.
>
> But I really would prefer it that we still send pages to host,
> otherwise debugging becomes much harder.
>
> And it does not have to be completely useless, even though
> you can not discard them as they would be zero-filled then.
>
> How about a config field telling host what should be there in the free
> pages? This way even though host can not discard them, host can send
> them out without reading them, still a win.
>
>
OK, but I think we would need two 32-bit config registers:
__u32 page_poison_val; // stores the PAGE_POISON VALUE, but it couldn't
indicate if page poisoning is in use
__u32 special_features; // set bit 0 to indicate page poisoning is in use
#define VIRTIO_BALLOON_SF_PAGE_POISON (1 << 0)
Best,
Wei
^ permalink raw reply
* Re: [PATCH v18 07/10] virtio-balloon: VIRTIO_BALLOON_F_SG
From: Wei Wang @ 2017-12-04 3:46 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: yang.zhang.wz, kvm, penguin-kernel, liliang.opensource,
qemu-devel, virtualization, linux-mm, aarcange, virtio-dev,
mawilcox, willy, quan.xu, nilal, riel, cornelia.huck, mhocko,
linux-kernel, amit.shah, pbonzini, akpm, mgorman
In-Reply-To: <20171201171746-mutt-send-email-mst@kernel.org>
On 12/01/2017 11:38 PM, Michael S. Tsirkin wrote:
> On Wed, Nov 29, 2017 at 09:55:23PM +0800, Wei Wang wrote:
>> +static void send_one_desc(struct virtio_balloon *vb,
>> + struct virtqueue *vq,
>> + uint64_t addr,
>> + uint32_t len,
>> + bool inbuf,
>> + bool batch)
>> +{
>> + int err;
>> + unsigned int size;
>> +
>> + /* Detach all the used buffers from the vq */
>> + while (virtqueue_get_buf(vq, &size))
>> + ;
>> +
>> + err = virtqueue_add_one_desc(vq, addr, len, inbuf, vq);
>> + /*
>> + * This is expected to never fail: there is always at least 1 entry
>> + * available on the vq, because when the vq is full the worker thread
>> + * that adds the desc will be put into sleep until at least 1 entry is
>> + * available to use.
>> + */
>> + BUG_ON(err);
>> +
>> + /* If batching is requested, we batch till the vq is full */
>> + if (!batch || !vq->num_free)
>> + kick_and_wait(vq, vb->acked);
>> +}
>> +
> This internal kick complicates callers. I suggest that instead,
> you move this to callers, just return a "kick required" boolean.
> This way callers do not need to play with num_free at all.
Then in what situation would the function return true of "kick required"?
I think this wouldn't make a difference fundamentally. For example, we
have 257 sgs (batching size=256) to send to host:
while (i < 257) {
kick_required = send_sgs();
if (kick_required)
kick(); // After the 256 sgs have been added, the caller
performs a kick().
}
Do we still need a kick here for the 257th sg as before? Only the caller
knows if the last added sgs need a kick (when the send_sgs receives one
sg, it doesn't know if there are more to come).
There is another approach to checking if the last added sgs haven't been
sync-ed to the host: expose "vring_virtqueue->num_added" to the caller
via a virtio_ring API:
unsigned int virtqueue_num_added(struct virtqueue *_vq)
{
struct vring_virtqueue *vq = to_vvq(_vq);
return vq->num_added;
}
>> +/*
>> + * Send balloon pages in sgs to host. The balloon pages are recorded in the
>> + * page xbitmap. Each bit in the bitmap corresponds to a page of PAGE_SIZE.
>> + * The page xbitmap is searched for continuous "1" bits, which correspond
>> + * to continuous pages, to chunk into sgs.
>> + *
>> + * @page_xb_start and @page_xb_end form the range of bits in the xbitmap that
>> + * need to be searched.
>> + */
>> +static void tell_host_sgs(struct virtio_balloon *vb,
>> + struct virtqueue *vq,
>> + unsigned long page_xb_start,
>> + unsigned long page_xb_end)
>> +{
>> + unsigned long pfn_start, pfn_end;
>> + uint64_t addr;
>> + uint32_t len, max_len = round_down(UINT_MAX, PAGE_SIZE);
>> +
>> + pfn_start = page_xb_start;
>> + while (pfn_start < page_xb_end) {
>> + pfn_start = xb_find_next_set_bit(&vb->page_xb, pfn_start,
>> + page_xb_end);
>> + if (pfn_start == page_xb_end + 1)
>> + break;
>> + pfn_end = xb_find_next_zero_bit(&vb->page_xb,
>> + pfn_start + 1,
>> + page_xb_end);
>> + addr = pfn_start << PAGE_SHIFT;
>> + len = (pfn_end - pfn_start) << PAGE_SHIFT;
> This assugnment can overflow. Next line compares with UINT_MAX but by
> that time it is too late. I think you should do all math in 64 bit to
> avoid surprises, then truncate to max_len and then it's safe to assign
> to sg.
Sounds reasonable, thanks.
>> +
>> + xb_clear_bit_range(&vb->page_xb, page_xb_start, page_xb_end);
>> +}
>> +
>> +static inline int xb_set_page(struct virtio_balloon *vb,
>> + struct page *page,
>> + unsigned long *pfn_min,
>> + unsigned long *pfn_max)
>> +{
>> + unsigned long pfn = page_to_pfn(page);
>> + int ret;
>> +
>> + *pfn_min = min(pfn, *pfn_min);
>> + *pfn_max = max(pfn, *pfn_max);
>> +
>> + do {
>> + ret = xb_preload_and_set_bit(&vb->page_xb, pfn,
>> + GFP_NOWAIT | __GFP_NOWARN);
>> + } while (unlikely(ret == -EAGAIN));
> what exactly does this loop do? Does this wait
> forever until there is some free memory? why GFP_NOWAIT?
Basically, "-EAGAIN" is returned from xb_set_bit() in the case when the
pre-allocated per-cpu ida_bitmap is NULL. In that case, the caller
re-invokes xb_preload_and_set_bit(), which re-invokes xb_preload to
allocate ida_bitmap. So "-EAGAIN" actually does not indicate a status
about memory allocation. "-ENOMEM" is the one to indicate the failure of
memory allocation, but the loop doesn't re-try on "-ENOMEM".
GFP_NOWAIT is used to avoid memory reclaiming, which could cause the
deadlock issue we discussed before.
> return num_freed_pages;
> }
>
> +/*
> + * The regular leak_balloon() with VIRTIO_BALLOON_F_SG needs memory allocation
> + * for xbitmap, which is not suitable for the oom case. This function does not
> + * use xbitmap to chunk pages, so it can be used by oom notifier to deflate
> + * pages when VIRTIO_BALLOON_F_SG is negotiated.
> + */
> I guess we can live with this for now.
Agree, the patchset has been big. We can get the basic implementation in
first, and leave the following as future work. I can add it in the
commit log.
> Two things to consider
> - adding support for pre-allocating indirect buffers
> - sorting the internal page queue (how?)
Best,
Wei
^ permalink raw reply
* Re: [RFC] virtio-net: help live migrate SR-IOV devices
From: Stephen Hemminger @ 2017-12-03 17:35 UTC (permalink / raw)
To: achiad shochat
Cc: Jakub Kicinski, Hannes Frederic Sowa, Sridhar Samudrala,
Michael S. Tsirkin, virtualization, Shannon Nelson, Achiad,
Peter Waskiewicz Jr, netdev, Singhai, Anjali, Andy Gospodarek,
Or Gerlitz
In-Reply-To: <CAEHy93+yhRtTQub0xk5W1jKRNisrcRBtNexcwRfWdSKPm5a4Gw@mail.gmail.com>
On Sun, 3 Dec 2017 11:14:37 +0200
achiad shochat <achiad.mellanox@gmail.com> wrote:
> On 3 December 2017 at 07:05, Michael S. Tsirkin <mst@redhat.com> wrote:
> > On Fri, Dec 01, 2017 at 12:08:59PM -0800, Shannon Nelson wrote:
> >> On 11/30/2017 6:11 AM, Michael S. Tsirkin wrote:
> >> > On Thu, Nov 30, 2017 at 10:08:45AM +0200, achiad shochat wrote:
> >> > > Re. problem #2:
> >> > > Indeed the best way to address it seems to be to enslave the VF driver
> >> > > netdev under a persistent anchor netdev.
> >> > > And it's indeed desired to allow (but not enforce) PV netdev and VF
> >> > > netdev to work in conjunction.
> >> > > And it's indeed desired that this enslavement logic work out-of-the box.
> >> > > But in case of PV+VF some configurable policies must be in place (and
> >> > > they'd better be generic rather than differ per PV technology).
> >> > > For example - based on which characteristics should the PV+VF coupling
> >> > > be done? netvsc uses MAC address, but that might not always be the
> >> > > desire.
> >> >
> >> > It's a policy but not guest userspace policy.
> >> >
> >> > The hypervisor certainly knows.
> >> >
> >> > Are you concerned that someone might want to create two devices with the
> >> > same MAC for an unrelated reason? If so, hypervisor could easily set a
> >> > flag in the virtio device to say "this is a backup, use MAC to find
> >> > another device".
> >>
> >> This is something I was going to suggest: a flag or other configuration on
> >> the virtio device to help control how this new feature is used. I can
> >> imagine this might be useful to control from either the hypervisor side or
> >> the VM side.
> >>
> >> The hypervisor might want to (1) disable it (force it off), (2) enable it
> >> for VM choice, or (3) force it on for the VM. In case (2), the VM might be
> >> able to chose whether it wants to make use of the feature, or stick with the
> >> bonding solution.
> >>
> >> Either way, the kernel is making a feature available, and the user (VM or
> >> hypervisor) is able to control it by selecting the feature based on the
> >> policy desired.
> >>
> >> sln
> >
> > I'm not sure what's the feature that is available here.
> >
> > I saw this as a flag that says "this device shares backend with another
> > network device which can be found using MAC, and that backend should be
> > preferred". kernel then forces configuration which uses that other
> > backend - as long as it exists.
> >
> > However, please Cc virtio-dev mailing list if we are doing this since
> > this is a spec extension.
> >
> > --
> > MST
>
>
> Can someone please explain why assume a virtio device is there at all??
> I specified a case where there isn't any.
>
> I second Jacob - having a netdev of one device driver enslave a netdev
> of another device driver is an awkward a-symmetric model.
> Regardless of whether they share the same backend device.
> Only I am not sure the Linux Bond is the right choice.
> e.g one may well want to use the virtio device also when the
> pass-through device is available, e.g for multicasts, east-west
> traffic, etc.
> I'm not sure the Linux Bond fits that functionality.
> And, as I hear in this thread, it is hard to make it work out of the box.
> So I think the right thing would be to write a new dedicated module
> for this purpose.
>
> Re policy -
> Indeed the HV can request a policy from the guest but that's not a
> claim for the virtio device enslaving the pass-through device.
> Any policy can be queried by the upper enslaving device.
>
> Bottom line - I do not see a single reason to have the virtio netdev
> (nor netvsc or any other PV netdev) enslave another netdev by itself.
> If we'd do it right with netvsc from the beginning we wouldn't need
> this discussion at all...
There are several issues with transparent migration.
The first is that the SR-IOV device needs to be shut off for earlier
in the migration process.
Next, the SR-IOV device in the migrated go guest environment maybe different.
It might not exist at all, it might be at a different PCI address, or it
could even be a different vendor/speed/model.
Keeping a virtual network device around allows persisting the connectivity,
during the process.
^ permalink raw reply
* Re: Memory corruption in powerpc guests with virtio_balloon (was Re: [PATCH v3] virtio_balloon: fix deadlock on OOM)
From: Michael Ellerman @ 2017-12-03 10:04 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Michal Hocko, Tetsuo Handa, linux-kernel, virtualization,
linux-mm, linuxppc-dev@lists.ozlabs.org
In-Reply-To: <20171201164129-mutt-send-email-mst@kernel.org>
"Michael S. Tsirkin" <mst@redhat.com> writes:
> On Fri, Dec 01, 2017 at 11:31:08PM +1100, Michael Ellerman wrote:
>> "Michael S. Tsirkin" <mst@redhat.com> writes:
>>
>> > fill_balloon doing memory allocations under balloon_lock
>> > can cause a deadlock when leak_balloon is called from
>> > virtballoon_oom_notify and tries to take same lock.
...
>>
>>
>> Somehow this commit seems to be killing powerpc guests.
...
>
> Thanks for the report!
> A fix was just posted:
> virtio_balloon: fix increment of vb->num_pfns in fill_balloon()
>
> Would appreciate testing.
Yep that fixes it. Thanks.
Tested-by: Michael Ellerman <mpe@ellerman.id.au>
cheers
^ permalink raw reply
* Re: [RFC] virtio-net: help live migrate SR-IOV devices
From: achiad shochat @ 2017-12-03 9:14 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Jakub Kicinski, Hannes Frederic Sowa, Sridhar Samudrala, netdev,
virtualization, Achiad, Peter Waskiewicz Jr, Singhai, Anjali,
Shannon Nelson, Andy Gospodarek, Or Gerlitz
In-Reply-To: <20171203065759-mutt-send-email-mst@kernel.org>
On 3 December 2017 at 07:05, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Fri, Dec 01, 2017 at 12:08:59PM -0800, Shannon Nelson wrote:
>> On 11/30/2017 6:11 AM, Michael S. Tsirkin wrote:
>> > On Thu, Nov 30, 2017 at 10:08:45AM +0200, achiad shochat wrote:
>> > > Re. problem #2:
>> > > Indeed the best way to address it seems to be to enslave the VF driver
>> > > netdev under a persistent anchor netdev.
>> > > And it's indeed desired to allow (but not enforce) PV netdev and VF
>> > > netdev to work in conjunction.
>> > > And it's indeed desired that this enslavement logic work out-of-the box.
>> > > But in case of PV+VF some configurable policies must be in place (and
>> > > they'd better be generic rather than differ per PV technology).
>> > > For example - based on which characteristics should the PV+VF coupling
>> > > be done? netvsc uses MAC address, but that might not always be the
>> > > desire.
>> >
>> > It's a policy but not guest userspace policy.
>> >
>> > The hypervisor certainly knows.
>> >
>> > Are you concerned that someone might want to create two devices with the
>> > same MAC for an unrelated reason? If so, hypervisor could easily set a
>> > flag in the virtio device to say "this is a backup, use MAC to find
>> > another device".
>>
>> This is something I was going to suggest: a flag or other configuration on
>> the virtio device to help control how this new feature is used. I can
>> imagine this might be useful to control from either the hypervisor side or
>> the VM side.
>>
>> The hypervisor might want to (1) disable it (force it off), (2) enable it
>> for VM choice, or (3) force it on for the VM. In case (2), the VM might be
>> able to chose whether it wants to make use of the feature, or stick with the
>> bonding solution.
>>
>> Either way, the kernel is making a feature available, and the user (VM or
>> hypervisor) is able to control it by selecting the feature based on the
>> policy desired.
>>
>> sln
>
> I'm not sure what's the feature that is available here.
>
> I saw this as a flag that says "this device shares backend with another
> network device which can be found using MAC, and that backend should be
> preferred". kernel then forces configuration which uses that other
> backend - as long as it exists.
>
> However, please Cc virtio-dev mailing list if we are doing this since
> this is a spec extension.
>
> --
> MST
Can someone please explain why assume a virtio device is there at all??
I specified a case where there isn't any.
I second Jacob - having a netdev of one device driver enslave a netdev
of another device driver is an awkward a-symmetric model.
Regardless of whether they share the same backend device.
Only I am not sure the Linux Bond is the right choice.
e.g one may well want to use the virtio device also when the
pass-through device is available, e.g for multicasts, east-west
traffic, etc.
I'm not sure the Linux Bond fits that functionality.
And, as I hear in this thread, it is hard to make it work out of the box.
So I think the right thing would be to write a new dedicated module
for this purpose.
Re policy -
Indeed the HV can request a policy from the guest but that's not a
claim for the virtio device enslaving the pass-through device.
Any policy can be queried by the upper enslaving device.
Bottom line - I do not see a single reason to have the virtio netdev
(nor netvsc or any other PV netdev) enslave another netdev by itself.
If we'd do it right with netvsc from the beginning we wouldn't need
this discussion at all...
^ permalink raw reply
* Re: [RFC] virtio-net: help live migrate SR-IOV devices
From: Michael S. Tsirkin @ 2017-12-03 5:05 UTC (permalink / raw)
To: Shannon Nelson
Cc: Jakub Kicinski, Hannes Frederic Sowa, achiad shochat,
Sridhar Samudrala, netdev, virtualization, Achiad,
Peter Waskiewicz Jr, Singhai, Anjali, Andy Gospodarek, Or Gerlitz
In-Reply-To: <780cf854-83e1-b2a4-0f07-74372a972ca6@oracle.com>
On Fri, Dec 01, 2017 at 12:08:59PM -0800, Shannon Nelson wrote:
> On 11/30/2017 6:11 AM, Michael S. Tsirkin wrote:
> > On Thu, Nov 30, 2017 at 10:08:45AM +0200, achiad shochat wrote:
> > > Re. problem #2:
> > > Indeed the best way to address it seems to be to enslave the VF driver
> > > netdev under a persistent anchor netdev.
> > > And it's indeed desired to allow (but not enforce) PV netdev and VF
> > > netdev to work in conjunction.
> > > And it's indeed desired that this enslavement logic work out-of-the box.
> > > But in case of PV+VF some configurable policies must be in place (and
> > > they'd better be generic rather than differ per PV technology).
> > > For example - based on which characteristics should the PV+VF coupling
> > > be done? netvsc uses MAC address, but that might not always be the
> > > desire.
> >
> > It's a policy but not guest userspace policy.
> >
> > The hypervisor certainly knows.
> >
> > Are you concerned that someone might want to create two devices with the
> > same MAC for an unrelated reason? If so, hypervisor could easily set a
> > flag in the virtio device to say "this is a backup, use MAC to find
> > another device".
>
> This is something I was going to suggest: a flag or other configuration on
> the virtio device to help control how this new feature is used. I can
> imagine this might be useful to control from either the hypervisor side or
> the VM side.
>
> The hypervisor might want to (1) disable it (force it off), (2) enable it
> for VM choice, or (3) force it on for the VM. In case (2), the VM might be
> able to chose whether it wants to make use of the feature, or stick with the
> bonding solution.
>
> Either way, the kernel is making a feature available, and the user (VM or
> hypervisor) is able to control it by selecting the feature based on the
> policy desired.
>
> sln
I'm not sure what's the feature that is available here.
I saw this as a flag that says "this device shares backend with another
network device which can be found using MAC, and that backend should be
preferred". kernel then forces configuration which uses that other
backend - as long as it exists.
However, please Cc virtio-dev mailing list if we are doing this since
this is a spec extension.
--
MST
^ permalink raw reply
* Re: [PATCH net,stable v4 0/3] vhost: fix a few skb leaks
From: David Miller @ 2017-12-03 2:32 UTC (permalink / raw)
To: wexu; +Cc: mjrosato, mst, netdev, linux-kernel, virtualization
In-Reply-To: <1512123038-15773-1-git-send-email-wexu@redhat.com>
From: wexu@redhat.com
Date: Fri, 1 Dec 2017 05:10:35 -0500
> Matthew found a roughly 40% tcp throughput regression with commit
> c67df11f(vhost_net: try batch dequing from skb array) as discussed
> in the following thread:
> https://www.mail-archive.com/netdev@vger.kernel.org/msg187936.html
Series applied and queued up for -stable.
^ permalink raw reply
* Re: [RFC] virtio-net: help live migrate SR-IOV devices
From: Shannon Nelson @ 2017-12-01 20:08 UTC (permalink / raw)
To: Michael S. Tsirkin, achiad shochat
Cc: Jakub Kicinski, Hannes Frederic Sowa, Sridhar Samudrala, netdev,
virtualization, Achiad, Peter Waskiewicz Jr, Singhai, Anjali,
Andy Gospodarek, Or Gerlitz
In-Reply-To: <20171130155612-mutt-send-email-mst@kernel.org>
On 11/30/2017 6:11 AM, Michael S. Tsirkin wrote:
> On Thu, Nov 30, 2017 at 10:08:45AM +0200, achiad shochat wrote:
>> Re. problem #2:
>> Indeed the best way to address it seems to be to enslave the VF driver
>> netdev under a persistent anchor netdev.
>> And it's indeed desired to allow (but not enforce) PV netdev and VF
>> netdev to work in conjunction.
>> And it's indeed desired that this enslavement logic work out-of-the box.
>> But in case of PV+VF some configurable policies must be in place (and
>> they'd better be generic rather than differ per PV technology).
>> For example - based on which characteristics should the PV+VF coupling
>> be done? netvsc uses MAC address, but that might not always be the
>> desire.
>
> It's a policy but not guest userspace policy.
>
> The hypervisor certainly knows.
>
> Are you concerned that someone might want to create two devices with the
> same MAC for an unrelated reason? If so, hypervisor could easily set a
> flag in the virtio device to say "this is a backup, use MAC to find
> another device".
This is something I was going to suggest: a flag or other configuration
on the virtio device to help control how this new feature is used. I
can imagine this might be useful to control from either the hypervisor
side or the VM side.
The hypervisor might want to (1) disable it (force it off), (2) enable
it for VM choice, or (3) force it on for the VM. In case (2), the VM
might be able to chose whether it wants to make use of the feature, or
stick with the bonding solution.
Either way, the kernel is making a feature available, and the user (VM
or hypervisor) is able to control it by selecting the feature based on
the policy desired.
sln
^ permalink raw reply
* [PATCH tip/core/rcu 21/21] drivers/vhost: Remove now-redundant read_barrier_depends()
From: Paul E. McKenney @ 2017-12-01 19:51 UTC (permalink / raw)
To: linux-kernel
Cc: tglx, kvm, Michael S. Tsirkin, peterz, fweisbec, jiangshanlai,
josh, rostedt, oleg, dhowells, edumazet, netdev,
mathieu.desnoyers, dipankar, akpm, Paul E. McKenney,
virtualization, mingo
In-Reply-To: <20171201195053.GA23494@linux.vnet.ibm.com>
Because READ_ONCE() now implies read_barrier_depends(), the
read_barrier_depends() in next_desc() is now redundant. This commit
therefore removes it and the related comments.
Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Jason Wang <jasowang@redhat.com>
Cc: <kvm@vger.kernel.org>
Cc: <virtualization@lists.linux-foundation.org>
Cc: <netdev@vger.kernel.org>
---
drivers/vhost/vhost.c | 7 +------
1 file changed, 1 insertion(+), 6 deletions(-)
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index 33ac2b186b85..78b5940a415a 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -1877,12 +1877,7 @@ static unsigned next_desc(struct vhost_virtqueue *vq, struct vring_desc *desc)
return -1U;
/* Check they're not leading us off end of descriptors. */
- next = vhost16_to_cpu(vq, desc->next);
- /* Make sure compiler knows to grab that: we don't want it changing! */
- /* We will use the result as an index in an array, so most
- * architectures only need a compiler barrier here. */
- read_barrier_depends();
-
+ next = vhost16_to_cpu(vq, READ_ONCE(desc->next));
return next;
}
--
2.5.2
^ permalink raw reply related
* [PATCH] virtio_mmio: add cleanup for virtio_mmio_probe
From: weiping zhang @ 2017-12-01 17:51 UTC (permalink / raw)
To: mst, jasowang, cohuck; +Cc: virtualization
cleanup all resource allocated by virtio_mmio_probe.
Signed-off-by: weiping zhang <zhangweiping@didichuxing.com>
---
drivers/virtio/virtio_mmio.c | 34 ++++++++++++++++++++++++++--------
1 file changed, 26 insertions(+), 8 deletions(-)
diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
index 74dc717..3fd0e66 100644
--- a/drivers/virtio/virtio_mmio.c
+++ b/drivers/virtio/virtio_mmio.c
@@ -513,8 +513,10 @@ static int virtio_mmio_probe(struct platform_device *pdev)
return -EBUSY;
vm_dev = devm_kzalloc(&pdev->dev, sizeof(*vm_dev), GFP_KERNEL);
- if (!vm_dev)
- return -ENOMEM;
+ if (!vm_dev) {
+ rc = -ENOMEM;
+ goto free_mem;
+ }
vm_dev->vdev.dev.parent = &pdev->dev;
vm_dev->vdev.dev.release = virtio_mmio_release_dev_empty;
@@ -524,14 +526,17 @@ static int virtio_mmio_probe(struct platform_device *pdev)
spin_lock_init(&vm_dev->lock);
vm_dev->base = devm_ioremap(&pdev->dev, mem->start, resource_size(mem));
- if (vm_dev->base == NULL)
- return -EFAULT;
+ if (vm_dev->base == NULL) {
+ rc = -EFAULT;
+ goto free_vmdev;
+ }
/* Check magic value */
magic = readl(vm_dev->base + VIRTIO_MMIO_MAGIC_VALUE);
if (magic != ('v' | 'i' << 8 | 'r' << 16 | 't' << 24)) {
dev_warn(&pdev->dev, "Wrong magic value 0x%08lx!\n", magic);
- return -ENODEV;
+ rc = -ENODEV;
+ goto unmap;
}
/* Check device version */
@@ -539,7 +544,8 @@ static int virtio_mmio_probe(struct platform_device *pdev)
if (vm_dev->version < 1 || vm_dev->version > 2) {
dev_err(&pdev->dev, "Version %ld not supported!\n",
vm_dev->version);
- return -ENXIO;
+ rc = -ENXIO;
+ goto unmap;
}
vm_dev->vdev.id.device = readl(vm_dev->base + VIRTIO_MMIO_DEVICE_ID);
@@ -548,7 +554,8 @@ static int virtio_mmio_probe(struct platform_device *pdev)
* virtio-mmio device with an ID 0 is a (dummy) placeholder
* with no function. End probing now with no error reported.
*/
- return -ENODEV;
+ rc = -ENODEV;
+ goto unmap;
}
vm_dev->vdev.id.vendor = readl(vm_dev->base + VIRTIO_MMIO_VENDOR_ID);
@@ -573,7 +580,18 @@ static int virtio_mmio_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, vm_dev);
- return register_virtio_device(&vm_dev->vdev);
+ rc = register_virtio_device(&vm_dev->vdev);
+ if (rc)
+ goto unmap;
+ return 0;
+unmap:
+ iounmap(vm_dev->base);
+free_mem:
+ devm_release_mem_region(&pdev->dev, mem->start,
+ resource_size(mem));
+free_vmdev:
+ devm_kfree(&pdev->dev, vm_dev);
+ return rc;
}
static int virtio_mmio_remove(struct platform_device *pdev)
--
2.9.4
^ permalink raw reply related
* Re: [PATCH] virtio: release virtio index when fail to device_register
From: weiping zhang @ 2017-12-01 17:30 UTC (permalink / raw)
To: Cornelia Huck; +Cc: virtualization, mst
In-Reply-To: <20171201165539.GA11865@localhost.didichuxing.com>
2017-12-02 0:55 GMT+08:00 weiping zhang <zwp10758@gmail.com>:
> On Wed, Nov 29, 2017 at 10:50:44AM +0100, Cornelia Huck wrote:
>> On Wed, 29 Nov 2017 09:23:01 +0800
>> weiping zhang <zwp10758@gmail.com> wrote:
>>
>> > index can be reused by other virtio device.
>> >
>> > Signed-off-by: weiping zhang <zhangweiping@didichuxing.com>
>>
>> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
>>
>> > ---
>> > drivers/virtio/virtio.c | 2 ++
>> > 1 file changed, 2 insertions(+)
>> >
>> > diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
>> > index 48230a5..bf7ff39 100644
>> > --- a/drivers/virtio/virtio.c
>> > +++ b/drivers/virtio/virtio.c
>> > @@ -333,6 +333,8 @@ int register_virtio_device(struct virtio_device *dev)
>> > /* device_register() causes the bus infrastructure to look for a
>> > * matching driver. */
>> > err = device_register(&dev->dev);
>> > + if (err)
>> > + ida_simple_remove(&virtio_index_ida, dev->index);
>> > out:
>> > if (err)
>> > virtio_add_status(dev, VIRTIO_CONFIG_S_FAILED);
>>
>> I think your patch is correct (giving up the index if we failed to
>> register), but this made me look at our error handling if a virtio
>> device failed to register.
>>
>> We hold an extra reference to the struct device, even after a failed
>> register, and it is the responsibility of the caller to give up that
>> reference once no longer needed. As callers toregister_virtio_device()
>> embed the struct virtio_device, it needs to be their responsibility.
>> Looking at the existing callers,
>>
>> - ccw does a put_device
>> - pci, mmio and remoteproc do nothing, causing a leak
>> - vop does a free on the embedding structure, which is a big no-no
>>
>> Thoughts?
> Sorry to relay late and thanks for your review.
> Do you mean the "extra reference to the struct device" caused by the
> following code?
>
> err = device_register(&dev->dev);
> device_add(dev)
> get_device(dev)
> If I'm understand right, I think there is no extra reference if we fail
> virtio_register_device, because if device_register we don't get a
> reference.
I go through these callers, virtio_mmio_probe should do more cleanup.
I'll sent a patch fix that.
> --
> Thanks
> weiping
^ permalink raw reply
* Re: [PATCH v18 05/10] xbitmap: add more operations
From: Matthew Wilcox @ 2017-12-01 17:25 UTC (permalink / raw)
To: Wang, Wei W
Cc: yang.zhang.wz@gmail.com, kvm@vger.kernel.org, mst@redhat.com,
Tetsuo Handa, liliang.opensource@gmail.com, qemu-devel@nongnu.org,
virtualization@lists.linux-foundation.org, linux-mm@kvack.org,
aarcange@redhat.com, virtio-dev@lists.oasis-open.org,
mawilcox@microsoft.com, quan.xu@aliyun.com, nilal@redhat.com,
riel@redhat.com, cornelia.huck@de.ibm.com, mhocko@kernel.org,
linux-kernel@vger.kernel.org, amit.sh
In-Reply-To: <286AC319A985734F985F78AFA26841F739376DA1@shsmsx102.ccr.corp.intel.com>
On Fri, Dec 01, 2017 at 03:09:08PM +0000, Wang, Wei W wrote:
> On Friday, December 1, 2017 9:02 PM, Tetsuo Handa wrote:
> > If start == end is legal,
> >
> > for (; start < end; start = (start | (IDA_BITMAP_BITS - 1)) + 1) {
> >
> > makes this loop do nothing because 10 < 10 is false.
>
> How about "start <= end "?
Don't ask Tetsuo for his opinion, write some userspace code that uses it.
^ permalink raw reply
* Re: [PATCH] virtio: release virtio index when fail to device_register
From: weiping zhang @ 2017-12-01 16:55 UTC (permalink / raw)
To: Cornelia Huck; +Cc: virtualization, mst
In-Reply-To: <20171129105044.3a4b0ab3.cohuck@redhat.com>
On Wed, Nov 29, 2017 at 10:50:44AM +0100, Cornelia Huck wrote:
> On Wed, 29 Nov 2017 09:23:01 +0800
> weiping zhang <zwp10758@gmail.com> wrote:
>
> > index can be reused by other virtio device.
> >
> > Signed-off-by: weiping zhang <zhangweiping@didichuxing.com>
>
> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
>
> > ---
> > drivers/virtio/virtio.c | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> > index 48230a5..bf7ff39 100644
> > --- a/drivers/virtio/virtio.c
> > +++ b/drivers/virtio/virtio.c
> > @@ -333,6 +333,8 @@ int register_virtio_device(struct virtio_device *dev)
> > /* device_register() causes the bus infrastructure to look for a
> > * matching driver. */
> > err = device_register(&dev->dev);
> > + if (err)
> > + ida_simple_remove(&virtio_index_ida, dev->index);
> > out:
> > if (err)
> > virtio_add_status(dev, VIRTIO_CONFIG_S_FAILED);
>
> I think your patch is correct (giving up the index if we failed to
> register), but this made me look at our error handling if a virtio
> device failed to register.
>
> We hold an extra reference to the struct device, even after a failed
> register, and it is the responsibility of the caller to give up that
> reference once no longer needed. As callers toregister_virtio_device()
> embed the struct virtio_device, it needs to be their responsibility.
> Looking at the existing callers,
>
> - ccw does a put_device
> - pci, mmio and remoteproc do nothing, causing a leak
> - vop does a free on the embedding structure, which is a big no-no
>
> Thoughts?
Sorry to relay late and thanks for your review.
Do you mean the "extra reference to the struct device" caused by the
following code?
err = device_register(&dev->dev);
device_add(dev)
get_device(dev)
If I'm understand right, I think there is no extra reference if we fail
virtio_register_device, because if device_register we don't get a
reference.
--
Thanks
weiping
^ permalink raw reply
* Re: [PATCH v18 10/10] virtio-balloon: don't report free pages when page poisoning is enabled
From: Michael S. Tsirkin @ 2017-12-01 15:49 UTC (permalink / raw)
To: Wei Wang
Cc: yang.zhang.wz, kvm, penguin-kernel, liliang.opensource,
qemu-devel, virtualization, linux-mm, aarcange, virtio-dev,
mawilcox, willy, quan.xu, nilal, riel, cornelia.huck, mhocko,
linux-kernel, amit.shah, pbonzini, akpm, mgorman
In-Reply-To: <1511963726-34070-11-git-send-email-wei.w.wang@intel.com>
On Wed, Nov 29, 2017 at 09:55:26PM +0800, Wei Wang wrote:
> The guest free pages should not be discarded by the live migration thread
> when page poisoning is enabled with PAGE_POISONING_NO_SANITY=n, because
> skipping the transfer of such poisoned free pages will trigger false
> positive when new pages are allocated and checked on the destination.
> This patch skips the reporting of free pages in the above case.
>
> Reported-by: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Wei Wang <wei.w.wang@intel.com>
> Cc: Michal Hocko <mhocko@suse.com>
> ---
> drivers/virtio/virtio_balloon.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> index 035bd3a..6ac4cff 100644
> --- a/drivers/virtio/virtio_balloon.c
> +++ b/drivers/virtio/virtio_balloon.c
> @@ -652,7 +652,9 @@ static void report_free_page(struct work_struct *work)
> /* Start by sending the obtained cmd id to the host with an outbuf */
> send_one_desc(vb, vb->free_page_vq, virt_to_phys(&vb->start_cmd_id),
> sizeof(uint32_t), false, true, false);
> - walk_free_mem_block(vb, 0, &virtio_balloon_send_free_pages);
> + if (!(page_poisoning_enabled() &&
> + !IS_ENABLED(CONFIG_PAGE_POISONING_NO_SANITY)))
> + walk_free_mem_block(vb, 0, &virtio_balloon_send_free_pages);
> /*
> * End by sending the stop id to the host with an outbuf. Use the
> * non-batching mode here to trigger a kick after adding the stop id.
PAGE_POISONING_ZERO is actually OK.
But I really would prefer it that we still send pages to host,
otherwise debugging becomes much harder.
And it does not have to be completely useless, even though
you can not discard them as they would be zero-filled then.
How about a config field telling host what should be there in the free
pages? This way even though host can not discard them, host can send
them out without reading them, still a win.
> --
> 2.7.4
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox