* 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
* 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: [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: [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
* [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
* [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 2/2] virtio_mmio: add cleanup for virtio_mmio_remove
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>
cleanup all resource allocated by virtio_mmio_probe.
Signed-off-by: weiping zhang <zhangweiping@didichuxing.com>
---
drivers/virtio/virtio_mmio.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
index f984510..5e2ca34 100644
--- a/drivers/virtio/virtio_mmio.c
+++ b/drivers/virtio/virtio_mmio.c
@@ -599,8 +599,15 @@ static int virtio_mmio_probe(struct platform_device *pdev)
static int virtio_mmio_remove(struct platform_device *pdev)
{
struct virtio_mmio_device *vm_dev = platform_get_drvdata(pdev);
+ struct resource *mem;
unregister_virtio_device(&vm_dev->vdev);
+ iounmap(vm_dev->base);
+ mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ if (mem)
+ devm_release_mem_region(&pdev->dev, mem->start,
+ resource_size(mem));
+ devm_kfree(&pdev->dev, vm_dev);
return 0;
}
--
2.9.4
^ permalink raw reply related
* Re: [PATCH tip/core/rcu 21/21] drivers/vhost: Remove now-redundant read_barrier_depends()
From: Michael S. Tsirkin @ 2017-12-05 18:31 UTC (permalink / raw)
To: Paul E. McKenney
Cc: tglx, kvm, peterz, fweisbec, jiangshanlai, linux-kernel, rostedt,
josh, dhowells, edumazet, netdev, mathieu.desnoyers, oleg,
dipankar, akpm, virtualization, mingo
In-Reply-To: <1512157876-24665-21-git-send-email-paulmck@linux.vnet.ibm.com>
On Fri, Dec 01, 2017 at 11:51:16AM -0800, Paul E. McKenney wrote:
> 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>
Apropos, READ_ONCE is now asymmetrical with WRITE_ONCE.
I can read a pointer with READ_ONCE and be sure the value
is sane, but only if I also remember to put in smp_wmb before
WRITE_ONCE. Otherwise the pointer is ok but no guarantees
about the data pointed to.
It would be better if the API reflected he assymetry somehow.
> ---
> 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
* Re: [PATCH tip/core/rcu 21/21] drivers/vhost: Remove now-redundant read_barrier_depends()
From: Peter Zijlstra @ 2017-12-05 18:39 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: tglx, kvm, fweisbec, jiangshanlai, linux-kernel, rostedt, josh,
dhowells, edumazet, netdev, mathieu.desnoyers, oleg, dipankar,
akpm, Paul E. McKenney, virtualization, mingo
In-Reply-To: <20171205202928-mutt-send-email-mst@kernel.org>
On Tue, Dec 05, 2017 at 08:31:20PM +0200, Michael S. Tsirkin wrote:
> Apropos, READ_ONCE is now asymmetrical with WRITE_ONCE.
>
> I can read a pointer with READ_ONCE and be sure the value
> is sane, but only if I also remember to put in smp_wmb before
> WRITE_ONCE. Otherwise the pointer is ok but no guarantees
> about the data pointed to.
That was already the case on everything except Alpha. And the canonical
match do the data dependency is store_release, not wmb.
^ permalink raw reply
* Re: [PATCH tip/core/rcu 21/21] drivers/vhost: Remove now-redundant read_barrier_depends()
From: Michael S. Tsirkin @ 2017-12-05 18:57 UTC (permalink / raw)
To: Peter Zijlstra
Cc: tglx, kvm, fweisbec, jiangshanlai, linux-kernel, rostedt, josh,
dhowells, edumazet, netdev, mathieu.desnoyers, oleg, dipankar,
akpm, Paul E. McKenney, virtualization, mingo
In-Reply-To: <20171205183946.GP3165@worktop.lehotels.local>
On Tue, Dec 05, 2017 at 07:39:46PM +0100, Peter Zijlstra wrote:
> On Tue, Dec 05, 2017 at 08:31:20PM +0200, Michael S. Tsirkin wrote:
>
> > Apropos, READ_ONCE is now asymmetrical with WRITE_ONCE.
> >
> > I can read a pointer with READ_ONCE and be sure the value
> > is sane, but only if I also remember to put in smp_wmb before
> > WRITE_ONCE. Otherwise the pointer is ok but no guarantees
> > about the data pointed to.
>
> That was already the case on everything except Alpha. And the canonical
> match do the data dependency is store_release, not wmb.
Oh, interesting
static __always_inline void __write_once_size(volatile void *p, void *res, int size)
{
switch (size) {
case 1: *(volatile __u8 *)p = *(__u8 *)res; break;
case 2: *(volatile __u16 *)p = *(__u16 *)res; break;
case 4: *(volatile __u32 *)p = *(__u32 *)res; break;
case 8: *(volatile __u64 *)p = *(__u64 *)res; break;
default:
barrier();
__builtin_memcpy((void *)p, (const void *)res, size);
barrier();
}
}
#define WRITE_ONCE(x, val) \
({ \
union { typeof(x) __val; char __c[1]; } __u = \
{ .__val = (__force typeof(x)) (val) }; \
__write_once_size(&(x), __u.__c, sizeof(x)); \
__u.__val; \
})
I don't see WRITE_ONCE inserting any barriers, release or
write.
So it seems that on an architecture where writes can be reordered,
if I do
*pointer = 0xa;
WRITE_ONCE(array[x], pointer);
array write might bypass the pointer write,
and readers will read a stale value.
--
MST
^ permalink raw reply
* Re: [PATCH tip/core/rcu 21/21] drivers/vhost: Remove now-redundant read_barrier_depends()
From: Peter Zijlstra @ 2017-12-05 19:17 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: tglx, kvm, fweisbec, jiangshanlai, linux-kernel, rostedt, josh,
dhowells, edumazet, netdev, mathieu.desnoyers, oleg, dipankar,
akpm, Paul E. McKenney, virtualization, mingo
In-Reply-To: <20171205204928-mutt-send-email-mst@kernel.org>
On Tue, Dec 05, 2017 at 08:57:46PM +0200, Michael S. Tsirkin wrote:
> I don't see WRITE_ONCE inserting any barriers, release or
> write.
Correct, never claimed there was.
Just saying that:
obj = READ_ONCE(*foo);
val = READ_ONCE(obj->val);
Never needs a barrier (except on Alpha and we want to make that go
away). Simply because a CPU needs to complete the load of @obj before it
can compute the address &obj->val. Thus the second load _must_ come
after the first load and we get LOAD-LOAD ordering.
Alpha messing that up is a royal pain, and Alpha not being an
active/living architecture is just not worth the pain of keeping this in
the generic model.
^ permalink raw reply
* Re: [RFC] virtio-net: help live migrate SR-IOV devices
From: Michael S. Tsirkin @ 2017-12-05 19:20 UTC (permalink / raw)
To: achiad shochat
Cc: Jakub Kicinski, Hannes Frederic Sowa, Sridhar Samudrala,
Alexander Duyck, virtualization, Shannon Nelson, Achiad,
Peter Waskiewicz Jr, netdev, Singhai, Anjali, Andy Gospodarek,
Or Gerlitz
In-Reply-To: <CAEHy93Kw-GePsraaAkVZNbOwt=rDPMVjmwuU22004LHCAoSSFA@mail.gmail.com>
On Tue, Dec 05, 2017 at 11:59:17AM +0200, achiad shochat wrote:
> 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.
For the record, I won't block patches adding this kist to virtio
on the basis that they must be generic. It's not a lot
of code, implementation can come first, prettify later.
But we do need to have a discussion about how devices are paired.
I am not sure using just MAC works. E.g. some passthrough
devices don't give host ability to set the MAC.
Are these worth worrying about?
--
MST
^ permalink raw reply
* Re: [PATCH tip/core/rcu 21/21] drivers/vhost: Remove now-redundant read_barrier_depends()
From: Michael S. Tsirkin @ 2017-12-05 19:24 UTC (permalink / raw)
To: Peter Zijlstra
Cc: tglx, kvm, fweisbec, jiangshanlai, linux-kernel, rostedt, josh,
dhowells, edumazet, netdev, mathieu.desnoyers, oleg, dipankar,
akpm, Paul E. McKenney, virtualization, mingo
In-Reply-To: <20171205191733.GQ3165@worktop.lehotels.local>
On Tue, Dec 05, 2017 at 08:17:33PM +0100, Peter Zijlstra wrote:
> On Tue, Dec 05, 2017 at 08:57:46PM +0200, Michael S. Tsirkin wrote:
>
> > I don't see WRITE_ONCE inserting any barriers, release or
> > write.
>
> Correct, never claimed there was.
>
> Just saying that:
>
> obj = READ_ONCE(*foo);
> val = READ_ONCE(obj->val);
>
> Never needs a barrier (except on Alpha and we want to make that go
> away). Simply because a CPU needs to complete the load of @obj before it
> can compute the address &obj->val. Thus the second load _must_ come
> after the first load and we get LOAD-LOAD ordering.
>
> Alpha messing that up is a royal pain, and Alpha not being an
> active/living architecture is just not worth the pain of keeping this in
> the generic model.
>
Right. What I am saying is that for writes you need
WRITE_ONCE(obj->val, 1);
smp_wmb();
WRITE_ONCE(*foo, obj);
and this barrier is no longer paired with anything until
you realize there's a dependency barrier within READ_ONCE.
Barrier pairing was a useful tool to check code validity,
maybe there are other, better tools now.
--
MST
^ permalink raw reply
* [PATCH] ptr_ring: add barriers
From: Michael S. Tsirkin @ 2017-12-05 19:29 UTC (permalink / raw)
To: linux-kernel; +Cc: George Cherian, netdev, virtualization, edumazet, davem
Users of ptr_ring expect that it's safe to give the
data structure a pointer and have it be available
to consumers, but that actually requires an smb_wmb
or a stronger barrier.
In absence of such barriers and on architectures that reorder writes,
consumer might read an un=initialized value from an skb pointer stored
in the skb array. This was observed causing crashes.
To fix, add memory barriers. The barrier we use is a wmb, the
assumption being that producers do not need to read the value so we do
not need to order these reads.
Reported-by: George Cherian <george.cherian@cavium.com>
Suggested-by: Jason Wang <jasowang@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
George, could you pls report whether this patch fixes
the issue for you?
This seems to be needed in stable as well.
include/linux/ptr_ring.h | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/include/linux/ptr_ring.h b/include/linux/ptr_ring.h
index 37b4bb2..6866df4 100644
--- a/include/linux/ptr_ring.h
+++ b/include/linux/ptr_ring.h
@@ -101,12 +101,18 @@ static inline bool ptr_ring_full_bh(struct ptr_ring *r)
/* Note: callers invoking this in a loop must use a compiler barrier,
* for example cpu_relax(). Callers must hold producer_lock.
+ * Callers are responsible for making sure pointer that is being queued
+ * points to a valid data.
*/
static inline int __ptr_ring_produce(struct ptr_ring *r, void *ptr)
{
if (unlikely(!r->size) || r->queue[r->producer])
return -ENOSPC;
+ /* Make sure the pointer we are storing points to a valid data. */
+ /* Pairs with smp_read_barrier_depends in __ptr_ring_consume. */
+ smp_wmb();
+
r->queue[r->producer++] = ptr;
if (unlikely(r->producer >= r->size))
r->producer = 0;
@@ -275,6 +281,9 @@ static inline void *__ptr_ring_consume(struct ptr_ring *r)
if (ptr)
__ptr_ring_discard_one(r);
+ /* Make sure anyone accessing data through the pointer is up to date. */
+ /* Pairs with smp_wmb in __ptr_ring_produce. */
+ smp_read_barrier_depends();
return ptr;
}
--
MST
^ permalink raw reply related
* Re: [PATCH tip/core/rcu 21/21] drivers/vhost: Remove now-redundant read_barrier_depends()
From: Paul E. McKenney @ 2017-12-05 19:33 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: tglx, kvm, Peter Zijlstra, fweisbec, jiangshanlai, linux-kernel,
rostedt, josh, dhowells, edumazet, netdev, mathieu.desnoyers,
oleg, dipankar, akpm, virtualization, mingo
In-Reply-To: <20171205212053-mutt-send-email-mst@kernel.org>
On Tue, Dec 05, 2017 at 09:24:21PM +0200, Michael S. Tsirkin wrote:
> On Tue, Dec 05, 2017 at 08:17:33PM +0100, Peter Zijlstra wrote:
> > On Tue, Dec 05, 2017 at 08:57:46PM +0200, Michael S. Tsirkin wrote:
> >
> > > I don't see WRITE_ONCE inserting any barriers, release or
> > > write.
> >
> > Correct, never claimed there was.
> >
> > Just saying that:
> >
> > obj = READ_ONCE(*foo);
> > val = READ_ONCE(obj->val);
> >
> > Never needs a barrier (except on Alpha and we want to make that go
> > away). Simply because a CPU needs to complete the load of @obj before it
> > can compute the address &obj->val. Thus the second load _must_ come
> > after the first load and we get LOAD-LOAD ordering.
> >
> > Alpha messing that up is a royal pain, and Alpha not being an
> > active/living architecture is just not worth the pain of keeping this in
> > the generic model.
> >
>
> Right. What I am saying is that for writes you need
>
> WRITE_ONCE(obj->val, 1);
> smp_wmb();
> WRITE_ONCE(*foo, obj);
I believe Peter was instead suggesting:
WRITE_ONCE(obj->val, 1);
smp_store_release(foo, obj);
> and this barrier is no longer paired with anything until
> you realize there's a dependency barrier within READ_ONCE.
>
> Barrier pairing was a useful tool to check code validity,
> maybe there are other, better tools now.
There are quite a few people who say that smp_store_release() is
easier for the tools to analyze than is smp_wmb(). My experience with
smp_read_barrier_depends() and rcu_dereference() leads me to believe
that they are correct.
Thanx, Paul
^ permalink raw reply
* Re: [PATCH tip/core/rcu 21/21] drivers/vhost: Remove now-redundant read_barrier_depends()
From: Michael S. Tsirkin @ 2017-12-05 19:51 UTC (permalink / raw)
To: Paul E. McKenney
Cc: tglx, kvm, Peter Zijlstra, fweisbec, jiangshanlai, linux-kernel,
rostedt, josh, dhowells, edumazet, netdev, mathieu.desnoyers,
oleg, dipankar, akpm, virtualization, mingo
In-Reply-To: <20171205193339.GP7829@linux.vnet.ibm.com>
On Tue, Dec 05, 2017 at 11:33:39AM -0800, Paul E. McKenney wrote:
> On Tue, Dec 05, 2017 at 09:24:21PM +0200, Michael S. Tsirkin wrote:
> > On Tue, Dec 05, 2017 at 08:17:33PM +0100, Peter Zijlstra wrote:
> > > On Tue, Dec 05, 2017 at 08:57:46PM +0200, Michael S. Tsirkin wrote:
> > >
> > > > I don't see WRITE_ONCE inserting any barriers, release or
> > > > write.
> > >
> > > Correct, never claimed there was.
> > >
> > > Just saying that:
> > >
> > > obj = READ_ONCE(*foo);
> > > val = READ_ONCE(obj->val);
> > >
> > > Never needs a barrier (except on Alpha and we want to make that go
> > > away). Simply because a CPU needs to complete the load of @obj before it
> > > can compute the address &obj->val. Thus the second load _must_ come
> > > after the first load and we get LOAD-LOAD ordering.
> > >
> > > Alpha messing that up is a royal pain, and Alpha not being an
> > > active/living architecture is just not worth the pain of keeping this in
> > > the generic model.
> > >
> >
> > Right. What I am saying is that for writes you need
> >
> > WRITE_ONCE(obj->val, 1);
> > smp_wmb();
> > WRITE_ONCE(*foo, obj);
>
> I believe Peter was instead suggesting:
>
> WRITE_ONCE(obj->val, 1);
> smp_store_release(foo, obj);
Isn't that more expensive though?
> > and this barrier is no longer paired with anything until
> > you realize there's a dependency barrier within READ_ONCE.
> >
> > Barrier pairing was a useful tool to check code validity,
> > maybe there are other, better tools now.
>
> There are quite a few people who say that smp_store_release() is
> easier for the tools to analyze than is smp_wmb(). My experience with
> smp_read_barrier_depends() and rcu_dereference() leads me to believe
> that they are correct.
>
> Thanx, Paul
OK, but smp_store_release is still not paired with anything since we
rely on READ_ONCE to include the implicit dpendendency barrier.
--
MST
^ permalink raw reply
* Re: [PATCH tip/core/rcu 21/21] drivers/vhost: Remove now-redundant read_barrier_depends()
From: Peter Zijlstra @ 2017-12-05 19:55 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: tglx, kvm, fweisbec, jiangshanlai, linux-kernel, rostedt, josh,
dhowells, edumazet, netdev, mathieu.desnoyers, oleg, dipankar,
akpm, Paul E. McKenney, virtualization, mingo
In-Reply-To: <20171205212053-mutt-send-email-mst@kernel.org>
On Tue, Dec 05, 2017 at 09:24:21PM +0200, Michael S. Tsirkin wrote:
> On Tue, Dec 05, 2017 at 08:17:33PM +0100, Peter Zijlstra wrote:
> > On Tue, Dec 05, 2017 at 08:57:46PM +0200, Michael S. Tsirkin wrote:
> >
> > > I don't see WRITE_ONCE inserting any barriers, release or
> > > write.
> >
> > Correct, never claimed there was.
> >
> > Just saying that:
> >
> > obj = READ_ONCE(*foo);
> > val = READ_ONCE(obj->val);
> >
> > Never needs a barrier (except on Alpha and we want to make that go
> > away). Simply because a CPU needs to complete the load of @obj before it
> > can compute the address &obj->val. Thus the second load _must_ come
> > after the first load and we get LOAD-LOAD ordering.
> >
> > Alpha messing that up is a royal pain, and Alpha not being an
> > active/living architecture is just not worth the pain of keeping this in
> > the generic model.
> >
>
> Right. What I am saying is that for writes you need
>
> WRITE_ONCE(obj->val, 1);
> smp_wmb();
> WRITE_ONCE(*foo, obj);
You really should use smp_store_release() here instead of the smp_wmb().
But yes.
> and this barrier is no longer paired with anything until
> you realize there's a dependency barrier within READ_ONCE.
No, there isn't. read_dependecy barriers are no more. They don't exist.
They never did, except on Alpha anyway.
There were a ton of sites that relied on this but never had the
smp_read_barrier_depends() annotation, similarly there were quite a few
sites that had the barrier but nobody could explain wtf for.
What these patches do is return the sane rule that dependent loads are
ordered.
And like all memory ordering; it should come with comments. Any piece of
code that relies on memory ordering and doesn't have big fat comments
that explain the required ordering are broken per definition. Maybe not
now, but they will be after a few years because someone changed it and
didn't know.
> Barrier pairing was a useful tool to check code validity,
> maybe there are other, better tools now.
Same is true for the closely related control dependency, you can pair
against those just fine but they don't have an explicit barrier
annotation.
There are no tools, use brain.
^ permalink raw reply
* Re: [PATCH tip/core/rcu 21/21] drivers/vhost: Remove now-redundant read_barrier_depends()
From: Peter Zijlstra @ 2017-12-05 19:57 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: tglx, kvm, fweisbec, jiangshanlai, linux-kernel, rostedt, josh,
dhowells, edumazet, netdev, mathieu.desnoyers, oleg, dipankar,
akpm, Paul E. McKenney, virtualization, mingo
In-Reply-To: <20171205215020-mutt-send-email-mst@kernel.org>
On Tue, Dec 05, 2017 at 09:51:48PM +0200, Michael S. Tsirkin wrote:
> > > WRITE_ONCE(obj->val, 1);
> > > smp_wmb();
> > > WRITE_ONCE(*foo, obj);
> >
> > I believe Peter was instead suggesting:
> >
> > WRITE_ONCE(obj->val, 1);
> > smp_store_release(foo, obj);
>
> Isn't that more expensive though?
Depends on the architecture. The only architecture where it is more
expensive and people actually still care about is ARM I think.
^ permalink raw reply
* Re: [PATCH tip/core/rcu 21/21] drivers/vhost: Remove now-redundant read_barrier_depends()
From: Paul E. McKenney @ 2017-12-05 20:08 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: tglx, kvm, Peter Zijlstra, fweisbec, jiangshanlai, linux-kernel,
rostedt, josh, dhowells, edumazet, netdev, mathieu.desnoyers,
oleg, dipankar, akpm, virtualization, mingo
In-Reply-To: <20171205215020-mutt-send-email-mst@kernel.org>
On Tue, Dec 05, 2017 at 09:51:48PM +0200, Michael S. Tsirkin wrote:
> On Tue, Dec 05, 2017 at 11:33:39AM -0800, Paul E. McKenney wrote:
> > On Tue, Dec 05, 2017 at 09:24:21PM +0200, Michael S. Tsirkin wrote:
[ . . . ]
> > > and this barrier is no longer paired with anything until
> > > you realize there's a dependency barrier within READ_ONCE.
> > >
> > > Barrier pairing was a useful tool to check code validity,
> > > maybe there are other, better tools now.
> >
> > There are quite a few people who say that smp_store_release() is
> > easier for the tools to analyze than is smp_wmb(). My experience with
> > smp_read_barrier_depends() and rcu_dereference() leads me to believe
> > that they are correct.
>
> OK, but smp_store_release is still not paired with anything since we
> rely on READ_ONCE to include the implicit dpendendency barrier.
Why wouldn't you consider the smp_store_release() to be paired with
the new improved READ_ONCE()?
Thanx, Paul
^ permalink raw reply
* Re: [PATCH tip/core/rcu 21/21] drivers/vhost: Remove now-redundant read_barrier_depends()
From: Michael S. Tsirkin @ 2017-12-05 20:28 UTC (permalink / raw)
To: Peter Zijlstra
Cc: tglx, kvm, fweisbec, jiangshanlai, linux-kernel, rostedt, josh,
dhowells, edumazet, netdev, mathieu.desnoyers, oleg, dipankar,
akpm, Paul E. McKenney, virtualization, mingo
In-Reply-To: <20171205195752.GS3165@worktop.lehotels.local>
On Tue, Dec 05, 2017 at 08:57:52PM +0100, Peter Zijlstra wrote:
> On Tue, Dec 05, 2017 at 09:51:48PM +0200, Michael S. Tsirkin wrote:
> > > > WRITE_ONCE(obj->val, 1);
> > > > smp_wmb();
> > > > WRITE_ONCE(*foo, obj);
> > >
> > > I believe Peter was instead suggesting:
> > >
> > > WRITE_ONCE(obj->val, 1);
> > > smp_store_release(foo, obj);
> >
> > Isn't that more expensive though?
>
> Depends on the architecture. The only architecture where it is more
> expensive and people actually still care about is ARM I think.
Right. Why should I use the more expensive smp_store_release then?
--
MST
^ permalink raw reply
* Re: [PATCH tip/core/rcu 21/21] drivers/vhost: Remove now-redundant read_barrier_depends()
From: Peter Zijlstra @ 2017-12-05 21:17 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: tglx, kvm, fweisbec, jiangshanlai, linux-kernel, rostedt, josh,
dhowells, edumazet, netdev, mathieu.desnoyers, oleg, dipankar,
akpm, Paul E. McKenney, virtualization, mingo
In-Reply-To: <20171205222757-mutt-send-email-mst@kernel.org>
On Tue, Dec 05, 2017 at 10:28:38PM +0200, Michael S. Tsirkin wrote:
> On Tue, Dec 05, 2017 at 08:57:52PM +0100, Peter Zijlstra wrote:
> > On Tue, Dec 05, 2017 at 09:51:48PM +0200, Michael S. Tsirkin wrote:
> > > > > WRITE_ONCE(obj->val, 1);
> > > > > smp_wmb();
> > > > > WRITE_ONCE(*foo, obj);
> > > >
> > > > I believe Peter was instead suggesting:
> > > >
> > > > WRITE_ONCE(obj->val, 1);
> > > > smp_store_release(foo, obj);
> > >
> > > Isn't that more expensive though?
> >
> > Depends on the architecture. The only architecture where it is more
> > expensive and people actually still care about is ARM I think.
>
> Right. Why should I use the more expensive smp_store_release then?
Because it makes more sense. Memory ordering is hard enough, don't make
it harder still if you don't have to.
^ permalink raw reply
* Re: [PATCH tip/core/rcu 21/21] drivers/vhost: Remove now-redundant read_barrier_depends()
From: Michael S. Tsirkin @ 2017-12-05 21:24 UTC (permalink / raw)
To: Paul E. McKenney
Cc: tglx, kvm, Peter Zijlstra, fweisbec, jiangshanlai, linux-kernel,
rostedt, josh, dhowells, edumazet, netdev, mathieu.desnoyers,
oleg, dipankar, akpm, virtualization, mingo
In-Reply-To: <20171205200801.GR7829@linux.vnet.ibm.com>
On Tue, Dec 05, 2017 at 12:08:01PM -0800, Paul E. McKenney wrote:
> On Tue, Dec 05, 2017 at 09:51:48PM +0200, Michael S. Tsirkin wrote:
> > On Tue, Dec 05, 2017 at 11:33:39AM -0800, Paul E. McKenney wrote:
> > > On Tue, Dec 05, 2017 at 09:24:21PM +0200, Michael S. Tsirkin wrote:
>
> [ . . . ]
>
> > > > and this barrier is no longer paired with anything until
> > > > you realize there's a dependency barrier within READ_ONCE.
> > > >
> > > > Barrier pairing was a useful tool to check code validity,
> > > > maybe there are other, better tools now.
> > >
> > > There are quite a few people who say that smp_store_release() is
> > > easier for the tools to analyze than is smp_wmb(). My experience with
> > > smp_read_barrier_depends() and rcu_dereference() leads me to believe
> > > that they are correct.
> >
> > OK, but smp_store_release is still not paired with anything since we
> > rely on READ_ONCE to include the implicit dpendendency barrier.
>
> Why wouldn't you consider the smp_store_release() to be paired with
> the new improved READ_ONCE()?
>
> Thanx, Paul
READ_ONCE is really all over the place (some code literally replaced all
memory accesses with READ/WRITE ONCE).
And I also prefer smp_wmb as it seems to be cheaper on ARM.
Would an API like WRITE_POINTER()/smp_store_pointer make sense,
and READ_POINTER for symmetry?
--
MST
^ permalink raw reply
* Re: [PATCH tip/core/rcu 21/21] drivers/vhost: Remove now-redundant read_barrier_depends()
From: Paul E. McKenney @ 2017-12-05 21:36 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: tglx, kvm, Peter Zijlstra, fweisbec, jiangshanlai, linux-kernel,
rostedt, josh, dhowells, edumazet, netdev, mathieu.desnoyers,
oleg, dipankar, akpm, virtualization, mingo
In-Reply-To: <20171205222857-mutt-send-email-mst@kernel.org>
On Tue, Dec 05, 2017 at 11:24:49PM +0200, Michael S. Tsirkin wrote:
> On Tue, Dec 05, 2017 at 12:08:01PM -0800, Paul E. McKenney wrote:
> > On Tue, Dec 05, 2017 at 09:51:48PM +0200, Michael S. Tsirkin wrote:
> > > On Tue, Dec 05, 2017 at 11:33:39AM -0800, Paul E. McKenney wrote:
> > > > On Tue, Dec 05, 2017 at 09:24:21PM +0200, Michael S. Tsirkin wrote:
> >
> > [ . . . ]
> >
> > > > > and this barrier is no longer paired with anything until
> > > > > you realize there's a dependency barrier within READ_ONCE.
> > > > >
> > > > > Barrier pairing was a useful tool to check code validity,
> > > > > maybe there are other, better tools now.
> > > >
> > > > There are quite a few people who say that smp_store_release() is
> > > > easier for the tools to analyze than is smp_wmb(). My experience with
> > > > smp_read_barrier_depends() and rcu_dereference() leads me to believe
> > > > that they are correct.
> > >
> > > OK, but smp_store_release is still not paired with anything since we
> > > rely on READ_ONCE to include the implicit dpendendency barrier.
> >
> > Why wouldn't you consider the smp_store_release() to be paired with
> > the new improved READ_ONCE()?
>
> READ_ONCE is really all over the place (some code literally replaced all
> memory accesses with READ/WRITE ONCE).
>
> And I also prefer smp_wmb as it seems to be cheaper on ARM.
>
> Would an API like WRITE_POINTER()/smp_store_pointer make sense,
> and READ_POINTER for symmetry?
What we do in some code is to comment the pairings, allowing the other
side of the pairing to be easily located. Would that work for you?
Thanx, Paul
^ permalink raw reply
* Re: [PATCH tip/core/rcu 21/21] drivers/vhost: Remove now-redundant read_barrier_depends()
From: Michael S. Tsirkin @ 2017-12-05 21:42 UTC (permalink / raw)
To: Peter Zijlstra
Cc: tglx, kvm, fweisbec, jiangshanlai, linux-kernel, rostedt, josh,
dhowells, edumazet, netdev, mathieu.desnoyers, oleg, dipankar,
akpm, Paul E. McKenney, virtualization, mingo
In-Reply-To: <20171205211735.GT3165@worktop.lehotels.local>
On Tue, Dec 05, 2017 at 10:17:35PM +0100, Peter Zijlstra wrote:
> On Tue, Dec 05, 2017 at 10:28:38PM +0200, Michael S. Tsirkin wrote:
> > On Tue, Dec 05, 2017 at 08:57:52PM +0100, Peter Zijlstra wrote:
> > > On Tue, Dec 05, 2017 at 09:51:48PM +0200, Michael S. Tsirkin wrote:
> > > > > > WRITE_ONCE(obj->val, 1);
> > > > > > smp_wmb();
> > > > > > WRITE_ONCE(*foo, obj);
> > > > >
> > > > > I believe Peter was instead suggesting:
> > > > >
> > > > > WRITE_ONCE(obj->val, 1);
> > > > > smp_store_release(foo, obj);
> > > >
> > > > Isn't that more expensive though?
> > >
> > > Depends on the architecture. The only architecture where it is more
> > > expensive and people actually still care about is ARM I think.
> >
> > Right. Why should I use the more expensive smp_store_release then?
>
> Because it makes more sense. Memory ordering is hard enough, don't make
> it harder still if you don't have to.
I suspect I have to - ptr_ring is a very low level construct used by
netowrking on data path so making it a bit more complicated for
a bit of performance is probably justified.
--
MST
^ permalink raw reply
* Re: [PATCH tip/core/rcu 21/21] drivers/vhost: Remove now-redundant read_barrier_depends()
From: Michael S. Tsirkin @ 2017-12-05 21:43 UTC (permalink / raw)
To: Paul E. McKenney
Cc: tglx, kvm, Peter Zijlstra, fweisbec, jiangshanlai, linux-kernel,
rostedt, josh, dhowells, edumazet, netdev, mathieu.desnoyers,
oleg, dipankar, akpm, virtualization, mingo
In-Reply-To: <20171205213644.GU7829@linux.vnet.ibm.com>
On Tue, Dec 05, 2017 at 01:36:44PM -0800, Paul E. McKenney wrote:
> On Tue, Dec 05, 2017 at 11:24:49PM +0200, Michael S. Tsirkin wrote:
> > On Tue, Dec 05, 2017 at 12:08:01PM -0800, Paul E. McKenney wrote:
> > > On Tue, Dec 05, 2017 at 09:51:48PM +0200, Michael S. Tsirkin wrote:
> > > > On Tue, Dec 05, 2017 at 11:33:39AM -0800, Paul E. McKenney wrote:
> > > > > On Tue, Dec 05, 2017 at 09:24:21PM +0200, Michael S. Tsirkin wrote:
> > >
> > > [ . . . ]
> > >
> > > > > > and this barrier is no longer paired with anything until
> > > > > > you realize there's a dependency barrier within READ_ONCE.
> > > > > >
> > > > > > Barrier pairing was a useful tool to check code validity,
> > > > > > maybe there are other, better tools now.
> > > > >
> > > > > There are quite a few people who say that smp_store_release() is
> > > > > easier for the tools to analyze than is smp_wmb(). My experience with
> > > > > smp_read_barrier_depends() and rcu_dereference() leads me to believe
> > > > > that they are correct.
> > > >
> > > > OK, but smp_store_release is still not paired with anything since we
> > > > rely on READ_ONCE to include the implicit dpendendency barrier.
> > >
> > > Why wouldn't you consider the smp_store_release() to be paired with
> > > the new improved READ_ONCE()?
> >
> > READ_ONCE is really all over the place (some code literally replaced all
> > memory accesses with READ/WRITE ONCE).
> >
> > And I also prefer smp_wmb as it seems to be cheaper on ARM.
> >
> > Would an API like WRITE_POINTER()/smp_store_pointer make sense,
> > and READ_POINTER for symmetry?
>
> What we do in some code is to comment the pairings, allowing the other
> side of the pairing to be easily located. Would that work for you?
>
> Thanx, Paul
Yes, that's exactly what I did for now.
Thanks!
--
MST
^ 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