memory barriers in virtq.lua?

memory barriers in virtq.lua?

[Michael S. Tsirkin]

Hi Nikolay,
I poked at src/lib/virtio/virtq.lua a bit -
I was surprised to find no explicit CPU memory
barriers in the virtq implementation.
These are typically required when using virtio
on smp machines - the spec actually mention where
barriers are necessary.
Are the barriers implicit somehow for lua?
I'd be curious to learn.


Thanks,

-- 
MST

Re: memory barriers in virtq.lua?

[Nikolay Nikolaev]

Hello Michael,


On Tue, Jan 27, 2015 at 6:01 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> Hi Nikolay,
> I poked at src/lib/virtio/virtq.lua a bit -
> I was surprised to find no explicit CPU memory
> barriers in the virtq implementation.
> These are typically required when using virtio
> on smp machines - the spec actually mention where
> barriers are necessary.
> Are the barriers implicit somehow for lua?
> I'd be curious to learn.
>


thanks for looking at our code and providing your feedback.

The virtq.lua implements the virtq operations from a device point of
view. We compile this with LuaJIT which is guaranteed to not reorder
operations [1]. We also target the x86 architecture, which is
guaranteed to not reorder stores [2]:
"Stores Are Seen in a Consistent Order by Other Processors".
We rely on both these facts and don't use barrier in the virtq code.
However I do agree that we'll have to put barriers once we switch to
other architectures and/or LuaJIT implements ordering optmisations.

Finally, I checked the virtio 1.0 spec again and didn't see any
explicit mentioning of memory barriers regarding the device side of
the spec. There are several places where memory barriers are mentioned
and these all are about the driver. Maybe they are omitted because
they are implicit somehow? Please clarify.

regards,
Nikolay Nikolaev

[1] https://www.freelists.org/post/luajit/Compiler-loadstore-barrier-volatile-pointer-barriers-in-general,1
[2] http://www.intel.com/content/dam/www/public/us/en/documents/manuals/64-ia-32-architectures-software-developer-vol-3a-part-1-manual.pdf
 - 8.2.3.7

>
> Thanks,
>
> --
> MST

Re: [snabb-devel] Re: memory barriers in virtq.lua?

[Luke Gorrie]

Hi Michael,

I'm writing to follow up the previous discussion about memory barriers in
virtio-net device implementations, and Cc'ing the DPDK list because I
believe this is relevant to them too.

First, thanks again for getting in touch and reviewing our code.

I have now found a missed case where we *do* require a hardware memory
barrier on x86 in our vhost/virtio-net device. That is when checking the
interrupt suppression flag after updating used->idx. This is needed because
x86 can reorder the write to used->idx after the read from avail->flags,
and that causes the guest to see a stale value of used->idx after it
toggles interrupt suppression.

If I may spell out my mental model, for the sake of being corrected and/or
as an example of how third party developers are reading and interpreting
the Virtio-net spec:

Relating this to Virtio 1.0, the most relevant section is 3.2.1 (Supplying
Buffers to the Device) which calls for two "suitable memory barriers". The
spec talks about these from the driver perspective, but they are both
relevant to the device side too.

The first barrier (write to descriptor table before write to used->idx) is
implicit on x86 because writes by the same core are not reordered. This
means that no explicit hardware barrier is needed. (A compiler barrier may
be needed, however.)

The second memory barrier (write to used->idx before reading avail->flags)
is not implicit on x86 because stores are reordered after loads. So an
explicit hardware memory barrier is needed.

I hope that is a correct assessment of the situation. (Forgive my
x86centricity, I am sure that seems very foreign to kernel hackers.)

If this assessment is correct then the DPDK developers might also want to
review librte_vhost/vhost_rxtx.c and consider adding a hardware memory
barrier between writing used->idx and reading avail->flags.

Cheers,
-Luke

P.S. I notice that the Linux virtio-net driver does not seem to tolerate
spurious interrupts, even though the Virtio 1.0 spec requires this
("must"). On 3.13.11-ckt15 I see them trigger an "irq nobody cared" kernel
log message and then the irq is disabled. If that sounds suspicious I can
supply more information.

Re: [snabb-devel] Re: memory barriers in virtq.lua?

[Michael S. Tsirkin]

On Tue, Apr 07, 2015 at 04:22:42PM +0200, Luke Gorrie wrote:
> Hi Michael,
> 
> I'm writing to follow up the previous discussion about memory barriers in
> virtio-net device implementations, and Cc'ing the DPDK list because I believe
> this is relevant to them too.
> 
> First, thanks again for getting in touch and reviewing our code.
> 
> I have now found a missed case where we *do* require a hardware memory barrier
> on x86 in our vhost/virtio-net device. That is when checking the interrupt
> suppression flag after updating used->idx. This is needed because x86 can
> reorder the write to used->idx after the read from avail->flags, and that
> causes the guest to see a stale value of used->idx after it toggles interrupt
> suppression.
> 
> If I may spell out my mental model, for the sake of being corrected and/or as
> an example of how third party developers are reading and interpreting the
> Virtio-net spec:
> 
> Relating this to Virtio 1.0, the most relevant section is 3.2.1 (Supplying
> Buffers to the Device) which calls for two "suitable memory barriers". The spec
> talks about these from the driver perspective, but they are both relevant to
> the device side too.
> 
> The first barrier (write to descriptor table before write to used->idx) is
> implicit on x86 because writes by the same core are not reordered. This means
> that no explicit hardware barrier is needed. (A compiler barrier may be needed,
> however.)
> 
> The second memory barrier (write to used->idx before reading avail->flags) is
> not implicit on x86 because stores are reordered after loads. So an explicit
> hardware memory barrier is needed.
> 
> I hope that is a correct assessment of the situation. (Forgive my
> x86centricity, I am sure that seems very foreign to kernel hackers.)
> 
> If this assessment is correct then the DPDK developers might also want to
> review librte_vhost/vhost_rxtx.c and consider adding a hardware memory barrier
> between writing used->idx and reading avail->flags.
> 
> Cheers,
> -Luke

I agree, this looks like a bug in dpdk.

> P.S. I notice that the Linux virtio-net driver does not seem to tolerate
> spurious interrupts, even though the Virtio 1.0 spec requires this ("must"). On
> 3.13.11-ckt15 I see them trigger an "irq nobody cared" kernel log message and
> then the irq is disabled. If that sounds suspicious I can supply more
> information.
> 
>

More information might be useful, yes.

Just guessing from the available info:

I think you refer to this:
        The driver MUST handle spurious interrupts from the device.

The intent is to be able to handle some spurious interrupts once in a
while.  AFAIK linux triggers the message if it gets a huge number of
spurious interrupts for an extended period of time.
For example, this will trigger if the device does not clear interrupt
line after interrupt register read.

Re: [snabb-devel] Re: memory barriers in virtq.lua?

[Michael S. Tsirkin]

On Tue, Apr 07, 2015 at 04:22:42PM +0200, Luke Gorrie wrote:
> Hi Michael,
> 
> I'm writing to follow up the previous discussion about memory barriers in
> virtio-net device implementations, and Cc'ing the DPDK list because I believe
> this is relevant to them too.
> 
> First, thanks again for getting in touch and reviewing our code.
> 
> I have now found a missed case where we *do* require a hardware memory barrier
> on x86 in our vhost/virtio-net device. That is when checking the interrupt
> suppression flag after updating used->idx. This is needed because x86 can
> reorder the write to used->idx after the read from avail->flags, and that
> causes the guest to see a stale value of used->idx after it toggles interrupt
> suppression.
> 
> If I may spell out my mental model, for the sake of being corrected and/or as
> an example of how third party developers are reading and interpreting the
> Virtio-net spec:
> 
> Relating this to Virtio 1.0, the most relevant section is 3.2.1 (Supplying
> Buffers to the Device) which calls for two "suitable memory barriers". The spec
> talks about these from the driver perspective, but they are both relevant to
> the device side too.
> 
> The first barrier (write to descriptor table before write to used->idx) is
> implicit on x86 because writes by the same core are not reordered. This means
> that no explicit hardware barrier is needed. (A compiler barrier may be needed,
> however.)
> 
> The second memory barrier (write to used->idx before reading avail->flags) is
> not implicit on x86 because stores are reordered after loads. So an explicit
> hardware memory barrier is needed.
> 
> I hope that is a correct assessment of the situation. (Forgive my
> x86centricity, I am sure that seems very foreign to kernel hackers.)
> 
> If this assessment is correct then the DPDK developers might also want to
> review librte_vhost/vhost_rxtx.c and consider adding a hardware memory barrier
> between writing used->idx and reading avail->flags.
> 
> Cheers,
> -Luke

I agree, this looks like a bug in dpdk.

> P.S. I notice that the Linux virtio-net driver does not seem to tolerate
> spurious interrupts, even though the Virtio 1.0 spec requires this ("must"). On
> 3.13.11-ckt15 I see them trigger an "irq nobody cared" kernel log message and
> then the irq is disabled. If that sounds suspicious I can supply more
> information.
> 
>

More information might be useful, yes.

Just guessing from the available info:

I think you refer to this:
        The driver MUST handle spurious interrupts from the device.

The intent is to be able to handle some spurious interrupts once in a
while.  AFAIK linux triggers the message if it gets a huge number of
spurious interrupts for an extended period of time.
For example, this will trigger if the device does not clear interrupt
line after interrupt register read.
 

Re: [snabb-devel] Re: memory barriers in virtq.lua?

[Luke Gorrie]

On 7 April 2015 at 17:30, Michael S. Tsirkin <mst-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:

> Just guessing from the available info:
>
> I think you refer to this:
>         The driver MUST handle spurious interrupts from the device.
>
> The intent is to be able to handle some spurious interrupts once in a
> while.  AFAIK linux triggers the message if it gets a huge number of
> spurious interrupts for an extended period of time.
> For example, this will trigger if the device does not clear interrupt
> line after interrupt register read.
>

Thanks for that info.

The only spurious interrupt that I think we need is one when vhost-user
reconnects. That would be to cover the case where the vswitch is restarted
after writing used->idx but before sending the interrupt.

Or perhaps there is a better solution to that case?

Looking forward to getting an upstream vhost-user reconnect. one thing at a
time.. :)

Cheers,
-Luke

Re: [snabb-devel] Re: memory barriers in virtq.lua?

[Luke Gorrie]

[-- Attachment #1.1: Type: text/plain, Size: 907 bytes --]

On 7 April 2015 at 17:30, Michael S. Tsirkin <mst@redhat.com> wrote:

> Just guessing from the available info:
>
> I think you refer to this:
>         The driver MUST handle spurious interrupts from the device.
>
> The intent is to be able to handle some spurious interrupts once in a
> while.  AFAIK linux triggers the message if it gets a huge number of
> spurious interrupts for an extended period of time.
> For example, this will trigger if the device does not clear interrupt
> line after interrupt register read.
>

Thanks for that info.

The only spurious interrupt that I think we need is one when vhost-user
reconnects. That would be to cover the case where the vswitch is restarted
after writing used->idx but before sending the interrupt.

Or perhaps there is a better solution to that case?

Looking forward to getting an upstream vhost-user reconnect. one thing at a
time.. :)

Cheers,
-Luke

[-- Attachment #1.2: Type: text/html, Size: 1475 bytes --]

[-- Attachment #2: Type: text/plain, Size: 183 bytes --]

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

Re: [dpdk-dev] [snabb-devel] Re: memory barriers in virtq.lua?

[Xie, Huawei]

On 4/7/2015 10:23 PM, Luke Gorrie wrote:
> Hi Michael,
>
> I'm writing to follow up the previous discussion about memory barriers in
> virtio-net device implementations, and Cc'ing the DPDK list because I
> believe this is relevant to them too.
>
> First, thanks again for getting in touch and reviewing our code.
>
> I have now found a missed case where we *do* require a hardware memory
> barrier on x86 in our vhost/virtio-net device. That is when checking the
> interrupt suppression flag after updating used->idx. This is needed because
> x86 can reorder the write to used->idx after the read from avail->flags,
> and that causes the guest to see a stale value of used->idx after it
> toggles interrupt suppression.
luke:
1. host read the flag. 2 guest toggles the flag 3.guest checks the used.
4. host update used.
Is this your case?

>
> If I may spell out my mental model, for the sake of being corrected and/or
> as an example of how third party developers are reading and interpreting
> the Virtio-net spec:
>
> Relating this to Virtio 1.0, the most relevant section is 3.2.1 (Supplying
> Buffers to the Device) which calls for two "suitable memory barriers". The
> spec talks about these from the driver perspective, but they are both
> relevant to the device side too.
>
> The first barrier (write to descriptor table before write to used->idx) is
> implicit on x86 because writes by the same core are not reordered. This
> means that no explicit hardware barrier is needed. (A compiler barrier may
> be needed, however.)
>
> The second memory barrier (write to used->idx before reading avail->flags)
> is not implicit on x86 because stores are reordered after loads. So an
> explicit hardware memory barrier is needed.
>
> I hope that is a correct assessment of the situation. (Forgive my
> x86centricity, I am sure that seems very foreign to kernel hackers.)
>
> If this assessment is correct then the DPDK developers might also want to
> review librte_vhost/vhost_rxtx.c and consider adding a hardware memory
> barrier between writing used->idx and reading avail->flags.
>
> Cheers,
> -Luke
>
> P.S. I notice that the Linux virtio-net driver does not seem to tolerate
> spurious interrupts, even though the Virtio 1.0 spec requires this
> ("must"). On 3.13.11-ckt15 I see them trigger an "irq nobody cared" kernel
> log message and then the irq is disabled. If that sounds suspicious I can
> supply more information.
>
>

Re: [dpdk-dev] Re: memory barriers in virtq.lua?

[Xie, Huawei]

On 4/7/2015 10:23 PM, Luke Gorrie wrote:
> Hi Michael,
>
> I'm writing to follow up the previous discussion about memory barriers in
> virtio-net device implementations, and Cc'ing the DPDK list because I
> believe this is relevant to them too.
>
> First, thanks again for getting in touch and reviewing our code.
>
> I have now found a missed case where we *do* require a hardware memory
> barrier on x86 in our vhost/virtio-net device. That is when checking the
> interrupt suppression flag after updating used->idx. This is needed because
> x86 can reorder the write to used->idx after the read from avail->flags,
> and that causes the guest to see a stale value of used->idx after it
> toggles interrupt suppression.
luke:
1. host read the flag. 2 guest toggles the flag 3.guest checks the used.
4. host update used.
Is this your case?

>
> If I may spell out my mental model, for the sake of being corrected and/or
> as an example of how third party developers are reading and interpreting
> the Virtio-net spec:
>
> Relating this to Virtio 1.0, the most relevant section is 3.2.1 (Supplying
> Buffers to the Device) which calls for two "suitable memory barriers". The
> spec talks about these from the driver perspective, but they are both
> relevant to the device side too.
>
> The first barrier (write to descriptor table before write to used->idx) is
> implicit on x86 because writes by the same core are not reordered. This
> means that no explicit hardware barrier is needed. (A compiler barrier may
> be needed, however.)
>
> The second memory barrier (write to used->idx before reading avail->flags)
> is not implicit on x86 because stores are reordered after loads. So an
> explicit hardware memory barrier is needed.
>
> I hope that is a correct assessment of the situation. (Forgive my
> x86centricity, I am sure that seems very foreign to kernel hackers.)
>
> If this assessment is correct then the DPDK developers might also want to
> review librte_vhost/vhost_rxtx.c and consider adding a hardware memory
> barrier between writing used->idx and reading avail->flags.
>
> Cheers,
> -Luke
>
> P.S. I notice that the Linux virtio-net driver does not seem to tolerate
> spurious interrupts, even though the Virtio 1.0 spec requires this
> ("must"). On 3.13.11-ckt15 I see them trigger an "irq nobody cared" kernel
> log message and then the irq is disabled. If that sounds suspicious I can
> supply more information.
>
>

Re: [dpdk-dev] [snabb-devel] Re: memory barriers in virtq.lua?

[Luke Gorrie]

[-- Attachment #1.1: Type: text/plain, Size: 260 bytes --]

Howdy,

On 8 April 2015 at 17:15, Xie, Huawei <huawei.xie@intel.com> wrote:

> luke:
> 1. host read the flag. 2 guest toggles the flag 3.guest checks the used.
> 4. host update used.
> Is this your case?
>

Yep, that is exactly the case I mean.

Cheers,
-Luke

[-- Attachment #1.2: Type: text/html, Size: 651 bytes --]

[-- Attachment #2: Type: text/plain, Size: 183 bytes --]

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

Re: [dpdk-dev] Re: memory barriers in virtq.lua?

[Luke Gorrie]

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

Howdy,

On 8 April 2015 at 17:15, Xie, Huawei <huawei.xie@intel.com> wrote:

> luke:
> 1. host read the flag. 2 guest toggles the flag 3.guest checks the used.
> 4. host update used.
> Is this your case?
>

Yep, that is exactly the case I mean.

Cheers,
-Luke

-- 
You received this message because you are subscribed to the Google Groups "Snabb Switch development" group.
To unsubscribe from this group and stop receiving emails from it, send an email to snabb-devel+unsubscribe@googlegroups.com.
To post to this group, send an email to snabb-devel@googlegroups.com.
Visit this group at http://groups.google.com/group/snabb-devel.

[-- Attachment #2: Type: text/html, Size: 1246 bytes --]

RE: [dpdk-dev] [snabb-devel] Re: memory barriers in virtq.lua?

[Xie, Huawei]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Luke Gorrie
> Sent: Thursday, April 09, 2015 11:13 AM
> To: snabb-devel@googlegroups.com
> Cc: dev@dpdk.org; VirtualOpenSystems Technical Team; Michael S. Tsirkin;
> virtualization@lists.linux-foundation.org
> Subject: Re: [dpdk-dev] [snabb-devel] Re: memory barriers in virtq.lua?
> 
> Howdy,
> 
> On 8 April 2015 at 17:15, Xie, Huawei <huawei.xie@intel.com> wrote:
> 
> > luke:
> > 1. host read the flag. 2 guest toggles the flag 3.guest checks the used.
> > 4. host update used.
> > Is this your case?
> >
> 
> Yep, that is exactly the case I mean.
Thanks. Will provide fix after evaluation.
> 
> Cheers,
> -Luke

Re: [snabb-devel] Re: memory barriers in virtq.lua?

[Xie, Huawei]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Luke Gorrie
> Sent: Thursday, April 09, 2015 11:13 AM
> To: snabb-devel@googlegroups.com
> Cc: dev@dpdk.org; VirtualOpenSystems Technical Team; Michael S. Tsirkin;
> virtualization@lists.linux-foundation.org
> Subject: Re: [dpdk-dev] [snabb-devel] Re: memory barriers in virtq.lua?
> 
> Howdy,
> 
> On 8 April 2015 at 17:15, Xie, Huawei <huawei.xie@intel.com> wrote:
> 
> > luke:
> > 1. host read the flag. 2 guest toggles the flag 3.guest checks the used.
> > 4. host update used.
> > Is this your case?
> >
> 
> Yep, that is exactly the case I mean.
Thanks. Will provide fix after evaluation.
> 
> Cheers,
> -Luke

Re: [snabb-devel] Re: memory barriers in virtq.lua?

[Luke Gorrie]

[-- Attachment #1.1: Type: text/plain, Size: 2186 bytes --]

Hi Michael,

I'm writing to follow up the previous discussion about memory barriers in
virtio-net device implementations, and Cc'ing the DPDK list because I
believe this is relevant to them too.

First, thanks again for getting in touch and reviewing our code.

I have now found a missed case where we *do* require a hardware memory
barrier on x86 in our vhost/virtio-net device. That is when checking the
interrupt suppression flag after updating used->idx. This is needed because
x86 can reorder the write to used->idx after the read from avail->flags,
and that causes the guest to see a stale value of used->idx after it
toggles interrupt suppression.

If I may spell out my mental model, for the sake of being corrected and/or
as an example of how third party developers are reading and interpreting
the Virtio-net spec:

Relating this to Virtio 1.0, the most relevant section is 3.2.1 (Supplying
Buffers to the Device) which calls for two "suitable memory barriers". The
spec talks about these from the driver perspective, but they are both
relevant to the device side too.

The first barrier (write to descriptor table before write to used->idx) is
implicit on x86 because writes by the same core are not reordered. This
means that no explicit hardware barrier is needed. (A compiler barrier may
be needed, however.)

The second memory barrier (write to used->idx before reading avail->flags)
is not implicit on x86 because stores are reordered after loads. So an
explicit hardware memory barrier is needed.

I hope that is a correct assessment of the situation. (Forgive my
x86centricity, I am sure that seems very foreign to kernel hackers.)

If this assessment is correct then the DPDK developers might also want to
review librte_vhost/vhost_rxtx.c and consider adding a hardware memory
barrier between writing used->idx and reading avail->flags.

Cheers,
-Luke

P.S. I notice that the Linux virtio-net driver does not seem to tolerate
spurious interrupts, even though the Virtio 1.0 spec requires this
("must"). On 3.13.11-ckt15 I see them trigger an "irq nobody cared" kernel
log message and then the irq is disabled. If that sounds suspicious I can
supply more information.

[-- Attachment #1.2: Type: text/html, Size: 3139 bytes --]

[-- Attachment #2: Type: text/plain, Size: 183 bytes --]

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