Re: [PATCH] virtio: use volatile to get used->idx in the loop

On 5/25/2016 6:01 PM, Richardson, Bruce wrote:

On Wed, May 25, 2016 at 12:50:02PM +0300, Michael S. Tsirkin wrote:

quoted

On Wed, May 25, 2016 at 10:47:30AM +0100, Bruce Richardson wrote:

quoted

On Wed, May 25, 2016 at 11:34:24AM +0300, Michael S. Tsirkin wrote:

quoted

On Wed, May 25, 2016 at 08:25:20AM +0000, Xie, Huawei wrote:

quoted

On 5/25/2016 4:12 PM, Xie, Huawei wrote:

quoted

There is no external function call or any barrier in the loop,
the used->idx would only be retrieved once.

Signed-off-by: Huawei Xie <redacted>
---
 drivers/net/virtio/virtio_ethdev.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index c3fb628..f6d6305 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c

@@ -204,7 +204,8 @@ virtio_send_command(struct virtqueue *vq, struct virtio_pmd_ctrl *ctrl,
 		usleep(100);
 	}
 
-	while (vq->vq_used_cons_idx != vq->vq_ring.used->idx) {
+	while (vq->vq_used_cons_idx !=
+	       *((volatile uint16_t *)(&vq->vq_ring.used->idx))) {
 		uint32_t idx, desc_idx, used_idx;
 		struct vring_used_elem *uep;
 

Find this issue when do the code rework of RX/TX queue.
As in other places, we also have loop retrieving the value of avial->idx
or used->idx, i prefer to declare the index in vq structure as volatile
to avoid potential issue.

Is there a reason why the value is not always volatile? I would have thought
it would be generally safer to mark the actual value as volatile inside the
structure definition itself? In any cases where we do want to store the value
locally and not re-access the structure, a local variable can be used.

Regards,
/Bruce

Linux generally discourages volatile as a general style guidance:
https://www.kernel.org/doc/Documentation/volatile-considered-harmful.txt
it doesn't have to apply to dpdk which has a different coding style
but IIUC this structure is inherited from linux, deviating
will make keeping things up to date harder.

The prohibition on volatile indeed doesn't apply to DPDK, due to the fact that
we so seldom use locks, and do a lot of direct register accesses in out PMDs.
[I also still have the scars from previous issues where we had nice subtle bugs
in our PMDs - which only occurred with specific subversions of gcc - all due
to a missing "volatile" on one structure element.]

However, in this case, I take your point about keeping things consistent with
the kernel. :-)

At least for virtio PMD, we have to support both Linux and FreeBSD, so
DPDK defines its own vring structure instead of including linux header file.
Two solutions for this volatile issue, 1) declare  used->idx and
avail->idx as volatile 2) define similar
access_once/read_once/write_once macro.
Would take the first one. In future, we could consider define
access_once, and apply to all other data structures if we want to use
the kernel style.

One thing i am confusing is other DPDK components include Linux header
files, do they compile on FreeBSD?

/Bruce

quoted

quoted

quoted

It might be a good idea to wrap this in a macro
similar to ACCESS_ONCE in Linux.

quoted

Stephen:
Another question is why we need a loop here?

/huawei

-- 
MST