* [PATCH] virtio: use volatile to get used->idx in the loop @ 2016-05-24 16:16 Huawei Xie 2016-05-25 8:25 ` Xie, Huawei 2016-05-30 8:22 ` Yuanhan Liu 0 siblings, 2 replies; 14+ messages in thread From: Huawei Xie @ 2016-05-24 16:16 UTC (permalink / raw) To: dev; +Cc: stephen, konstantin.ananyev, thomas.monjalon, Huawei Xie 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 <huawei.xie@intel.com> --- 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; -- 1.8.1.4 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] virtio: use volatile to get used->idx in the loop 2016-05-24 16:16 [PATCH] virtio: use volatile to get used->idx in the loop Huawei Xie @ 2016-05-25 8:25 ` Xie, Huawei 2016-05-25 8:34 ` Michael S. Tsirkin 2016-05-30 8:22 ` Yuanhan Liu 1 sibling, 1 reply; 14+ messages in thread From: Xie, Huawei @ 2016-05-25 8:25 UTC (permalink / raw) To: dev Cc: stephen, Ananyev, Konstantin, thomas.monjalon, ms >> Michael S. Tsirkin, Yuanhan Liu, Tan, Jianfeng On 5/25/2016 4:12 PM, Xie, Huawei wrote: > 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 <huawei.xie@intel.com> > --- > 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. Stephen: Another question is why we need a loop here? /huawei ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] virtio: use volatile to get used->idx in the loop 2016-05-25 8:25 ` Xie, Huawei @ 2016-05-25 8:34 ` Michael S. Tsirkin 2016-05-25 9:47 ` Bruce Richardson 0 siblings, 1 reply; 14+ messages in thread From: Michael S. Tsirkin @ 2016-05-25 8:34 UTC (permalink / raw) To: Xie, Huawei Cc: dev, stephen, Ananyev, Konstantin, thomas.monjalon, Yuanhan Liu, Tan, Jianfeng On Wed, May 25, 2016 at 08:25:20AM +0000, Xie, Huawei wrote: > On 5/25/2016 4:12 PM, Xie, Huawei wrote: > > 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 <huawei.xie@intel.com> > > --- > > 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. It might be a good idea to wrap this in a macro similar to ACCESS_ONCE in Linux. > > Stephen: > Another question is why we need a loop here? > > /huawei -- MST ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] virtio: use volatile to get used->idx in the loop 2016-05-25 8:34 ` Michael S. Tsirkin @ 2016-05-25 9:47 ` Bruce Richardson 2016-05-25 9:50 ` Michael S. Tsirkin 0 siblings, 1 reply; 14+ messages in thread From: Bruce Richardson @ 2016-05-25 9:47 UTC (permalink / raw) To: Michael S. Tsirkin Cc: Xie, Huawei, dev, stephen, Ananyev, Konstantin, thomas.monjalon, Yuanhan Liu, Tan, Jianfeng On Wed, May 25, 2016 at 11:34:24AM +0300, Michael S. Tsirkin wrote: > On Wed, May 25, 2016 at 08:25:20AM +0000, Xie, Huawei wrote: > > On 5/25/2016 4:12 PM, Xie, Huawei wrote: > > > 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 <huawei.xie@intel.com> > > > --- > > > 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 > > It might be a good idea to wrap this in a macro > similar to ACCESS_ONCE in Linux. > > > > > Stephen: > > Another question is why we need a loop here? > > > > /huawei > > -- > MST ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] virtio: use volatile to get used->idx in the loop 2016-05-25 9:47 ` Bruce Richardson @ 2016-05-25 9:50 ` Michael S. Tsirkin 2016-05-25 10:00 ` Bruce Richardson 0 siblings, 1 reply; 14+ messages in thread From: Michael S. Tsirkin @ 2016-05-25 9:50 UTC (permalink / raw) To: Bruce Richardson Cc: Xie, Huawei, dev, stephen, Ananyev, Konstantin, thomas.monjalon, Yuanhan Liu, Tan, Jianfeng On Wed, May 25, 2016 at 10:47:30AM +0100, Bruce Richardson wrote: > On Wed, May 25, 2016 at 11:34:24AM +0300, Michael S. Tsirkin wrote: > > On Wed, May 25, 2016 at 08:25:20AM +0000, Xie, Huawei wrote: > > > On 5/25/2016 4:12 PM, Xie, Huawei wrote: > > > > 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 <huawei.xie@intel.com> > > > > --- > > > > 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. > > > > It might be a good idea to wrap this in a macro > > similar to ACCESS_ONCE in Linux. > > > > > > > > Stephen: > > > Another question is why we need a loop here? > > > > > > /huawei > > > > -- > > MST ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] virtio: use volatile to get used->idx in the loop 2016-05-25 9:50 ` Michael S. Tsirkin @ 2016-05-25 10:00 ` Bruce Richardson 2016-05-25 15:24 ` Xie, Huawei 0 siblings, 1 reply; 14+ messages in thread From: Bruce Richardson @ 2016-05-25 10:00 UTC (permalink / raw) To: Michael S. Tsirkin Cc: Xie, Huawei, dev, stephen, Ananyev, Konstantin, thomas.monjalon, Yuanhan Liu, Tan, Jianfeng On Wed, May 25, 2016 at 12:50:02PM +0300, Michael S. Tsirkin wrote: > On Wed, May 25, 2016 at 10:47:30AM +0100, Bruce Richardson wrote: > > On Wed, May 25, 2016 at 11:34:24AM +0300, Michael S. Tsirkin wrote: > > > On Wed, May 25, 2016 at 08:25:20AM +0000, Xie, Huawei wrote: > > > > On 5/25/2016 4:12 PM, Xie, Huawei wrote: > > > > > 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 <huawei.xie@intel.com> > > > > > --- > > > > > 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. :-) /Bruce > > > > > > > It might be a good idea to wrap this in a macro > > > similar to ACCESS_ONCE in Linux. > > > > > > > > > > > Stephen: > > > > Another question is why we need a loop here? > > > > > > > > /huawei > > > > > > -- > > > MST ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] virtio: use volatile to get used->idx in the loop 2016-05-25 10:00 ` Bruce Richardson @ 2016-05-25 15:24 ` Xie, Huawei 0 siblings, 0 replies; 14+ messages in thread From: Xie, Huawei @ 2016-05-25 15:24 UTC (permalink / raw) To: Richardson, Bruce, Michael S. Tsirkin Cc: dev, stephen, Ananyev, Konstantin, thomas.monjalon, Yuanhan Liu, Tan, Jianfeng On 5/25/2016 6:01 PM, Richardson, Bruce wrote: > On Wed, May 25, 2016 at 12:50:02PM +0300, Michael S. Tsirkin wrote: >> On Wed, May 25, 2016 at 10:47:30AM +0100, Bruce Richardson wrote: >>> On Wed, May 25, 2016 at 11:34:24AM +0300, Michael S. Tsirkin wrote: >>>> On Wed, May 25, 2016 at 08:25:20AM +0000, Xie, Huawei wrote: >>>>> On 5/25/2016 4:12 PM, Xie, Huawei wrote: >>>>>> 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 <huawei.xie@intel.com> >>>>>> --- >>>>>> 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 > >>>> It might be a good idea to wrap this in a macro >>>> similar to ACCESS_ONCE in Linux. >>>> >>>>> Stephen: >>>>> Another question is why we need a loop here? >>>>> >>>>> /huawei >>>> -- >>>> MST ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] virtio: use volatile to get used->idx in the loop 2016-05-24 16:16 [PATCH] virtio: use volatile to get used->idx in the loop Huawei Xie 2016-05-25 8:25 ` Xie, Huawei @ 2016-05-30 8:22 ` Yuanhan Liu 2016-06-01 5:40 ` Xie, Huawei 1 sibling, 1 reply; 14+ messages in thread From: Yuanhan Liu @ 2016-05-30 8:22 UTC (permalink / raw) To: Huawei Xie; +Cc: dev, stephen, konstantin.ananyev, thomas.monjalon On Wed, May 25, 2016 at 12:16:41AM +0800, Huawei Xie wrote: > 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 <huawei.xie@intel.com> > --- > 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))) { I'm wondering maybe we could fix VIRTQUEUE_NUSED (which has no such qualifier) and use this macro here? If you check the reference of that macro, you might find similar issues, say, it is also used inside the while-loop of virtio_recv_mergeable_pkts(). --yliu ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] virtio: use volatile to get used->idx in the loop 2016-05-30 8:22 ` Yuanhan Liu @ 2016-06-01 5:40 ` Xie, Huawei 2016-06-01 6:05 ` Yuanhan Liu 0 siblings, 1 reply; 14+ messages in thread From: Xie, Huawei @ 2016-06-01 5:40 UTC (permalink / raw) To: Yuanhan Liu; +Cc: dev, stephen, Ananyev, Konstantin, thomas.monjalon On 5/30/2016 4:20 PM, Yuanhan Liu wrote: > On Wed, May 25, 2016 at 12:16:41AM +0800, Huawei Xie wrote: >> 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 <huawei.xie@intel.com> >> --- >> 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))) { > I'm wondering maybe we could fix VIRTQUEUE_NUSED (which has no such > qualifier) and use this macro here? > > If you check the reference of that macro, you might find similar > issues, say, it is also used inside the while-loop of > virtio_recv_mergeable_pkts(). > > --yliu > > Yes, seems it has same issue though haven't confirmed with asm code. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] virtio: use volatile to get used->idx in the loop 2016-06-01 5:40 ` Xie, Huawei @ 2016-06-01 6:05 ` Yuanhan Liu 2016-06-02 8:39 ` Xie, Huawei 0 siblings, 1 reply; 14+ messages in thread From: Yuanhan Liu @ 2016-06-01 6:05 UTC (permalink / raw) To: Xie, Huawei; +Cc: dev, stephen, Ananyev, Konstantin, thomas.monjalon On Wed, Jun 01, 2016 at 05:40:08AM +0000, Xie, Huawei wrote: > On 5/30/2016 4:20 PM, Yuanhan Liu wrote: > > On Wed, May 25, 2016 at 12:16:41AM +0800, Huawei Xie wrote: > >> 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 <huawei.xie@intel.com> > >> --- > >> 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))) { > > I'm wondering maybe we could fix VIRTQUEUE_NUSED (which has no such > > qualifier) and use this macro here? > > > > If you check the reference of that macro, you might find similar > > issues, say, it is also used inside the while-loop of > > virtio_recv_mergeable_pkts(). > > > > --yliu > > > > > > Yes, seems it has same issue though haven't confirmed with asm code. So, move the "volatile" qualifier to VIRTQUEUE_NUSED? --yliu ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] virtio: use volatile to get used->idx in the loop 2016-06-01 6:05 ` Yuanhan Liu @ 2016-06-02 8:39 ` Xie, Huawei 2016-06-02 8:52 ` Yuanhan Liu 0 siblings, 1 reply; 14+ messages in thread From: Xie, Huawei @ 2016-06-02 8:39 UTC (permalink / raw) To: Yuanhan Liu; +Cc: dev, stephen, Ananyev, Konstantin, thomas.monjalon On 6/1/2016 2:03 PM, Yuanhan Liu wrote: > On Wed, Jun 01, 2016 at 05:40:08AM +0000, Xie, Huawei wrote: >> On 5/30/2016 4:20 PM, Yuanhan Liu wrote: >>> On Wed, May 25, 2016 at 12:16:41AM +0800, Huawei Xie wrote: >>>> 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 <huawei.xie@intel.com> >>>> --- >>>> 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))) { >>> I'm wondering maybe we could fix VIRTQUEUE_NUSED (which has no such >>> qualifier) and use this macro here? >>> >>> If you check the reference of that macro, you might find similar >>> issues, say, it is also used inside the while-loop of >>> virtio_recv_mergeable_pkts(). >>> >>> --yliu >>> >>> >> Yes, seems it has same issue though haven't confirmed with asm code. > So, move the "volatile" qualifier to VIRTQUEUE_NUSED? > > --yliu > Yes, anyway this is just intermediate fix. In next patch, will declare the idx as volatile, and remove the qualifier in the macro. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] virtio: use volatile to get used->idx in the loop 2016-06-02 8:39 ` Xie, Huawei @ 2016-06-02 8:52 ` Yuanhan Liu 2016-06-02 8:54 ` Xie, Huawei 0 siblings, 1 reply; 14+ messages in thread From: Yuanhan Liu @ 2016-06-02 8:52 UTC (permalink / raw) To: Xie, Huawei; +Cc: dev, stephen, Ananyev, Konstantin, thomas.monjalon On Thu, Jun 02, 2016 at 08:39:36AM +0000, Xie, Huawei wrote: > On 6/1/2016 2:03 PM, Yuanhan Liu wrote: > > On Wed, Jun 01, 2016 at 05:40:08AM +0000, Xie, Huawei wrote: > >> On 5/30/2016 4:20 PM, Yuanhan Liu wrote: > >>> On Wed, May 25, 2016 at 12:16:41AM +0800, Huawei Xie wrote: > >>>> 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 <huawei.xie@intel.com> > >>>> --- > >>>> 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))) { > >>> I'm wondering maybe we could fix VIRTQUEUE_NUSED (which has no such > >>> qualifier) and use this macro here? > >>> > >>> If you check the reference of that macro, you might find similar > >>> issues, say, it is also used inside the while-loop of > >>> virtio_recv_mergeable_pkts(). > >>> > >>> --yliu > >>> > >>> > >> Yes, seems it has same issue though haven't confirmed with asm code. > > So, move the "volatile" qualifier to VIRTQUEUE_NUSED? > > > > --yliu > > > > Yes, anyway this is just intermediate fix. In next patch, will declare > the idx as volatile, and remove the qualifier in the macro. Hmm.., why we need an intermediate fix then, if we can come up with an ultimate fix very quickly? --yliu ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] virtio: use volatile to get used->idx in the loop 2016-06-02 8:52 ` Yuanhan Liu @ 2016-06-02 8:54 ` Xie, Huawei 2016-06-14 13:23 ` Yuanhan Liu 0 siblings, 1 reply; 14+ messages in thread From: Xie, Huawei @ 2016-06-02 8:54 UTC (permalink / raw) To: Yuanhan Liu; +Cc: dev, stephen, Ananyev, Konstantin, thomas.monjalon On 6/2/2016 4:52 PM, Yuanhan Liu wrote: > On Thu, Jun 02, 2016 at 08:39:36AM +0000, Xie, Huawei wrote: >> On 6/1/2016 2:03 PM, Yuanhan Liu wrote: >>> On Wed, Jun 01, 2016 at 05:40:08AM +0000, Xie, Huawei wrote: >>>> On 5/30/2016 4:20 PM, Yuanhan Liu wrote: >>>>> On Wed, May 25, 2016 at 12:16:41AM +0800, Huawei Xie wrote: >>>>>> 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 <huawei.xie@intel.com> >>>>>> --- >>>>>> 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))) { >>>>> I'm wondering maybe we could fix VIRTQUEUE_NUSED (which has no such >>>>> qualifier) and use this macro here? >>>>> >>>>> If you check the reference of that macro, you might find similar >>>>> issues, say, it is also used inside the while-loop of >>>>> virtio_recv_mergeable_pkts(). >>>>> >>>>> --yliu >>>>> >>>>> >>>> Yes, seems it has same issue though haven't confirmed with asm code. >>> So, move the "volatile" qualifier to VIRTQUEUE_NUSED? >>> >>> --yliu >>> >> Yes, anyway this is just intermediate fix. In next patch, will declare >> the idx as volatile, and remove the qualifier in the macro. > Hmm.., why we need an intermediate fix then, if we can come up with an > ultimate fix very quickly? > > --yliu > ... Either is OK. I have no preference. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] virtio: use volatile to get used->idx in the loop 2016-06-02 8:54 ` Xie, Huawei @ 2016-06-14 13:23 ` Yuanhan Liu 0 siblings, 0 replies; 14+ messages in thread From: Yuanhan Liu @ 2016-06-14 13:23 UTC (permalink / raw) To: Xie, Huawei; +Cc: dev, stephen, Ananyev, Konstantin, thomas.monjalon On Thu, Jun 02, 2016 at 08:54:38AM +0000, Xie, Huawei wrote: > On 6/2/2016 4:52 PM, Yuanhan Liu wrote: > > On Thu, Jun 02, 2016 at 08:39:36AM +0000, Xie, Huawei wrote: > >> On 6/1/2016 2:03 PM, Yuanhan Liu wrote: > >>> On Wed, Jun 01, 2016 at 05:40:08AM +0000, Xie, Huawei wrote: > >>>> On 5/30/2016 4:20 PM, Yuanhan Liu wrote: > >>>>> On Wed, May 25, 2016 at 12:16:41AM +0800, Huawei Xie wrote: > >>>>>> 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 <huawei.xie@intel.com> > >>>>>> --- > >>>>>> 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))) { > >>>>> I'm wondering maybe we could fix VIRTQUEUE_NUSED (which has no such > >>>>> qualifier) and use this macro here? > >>>>> > >>>>> If you check the reference of that macro, you might find similar > >>>>> issues, say, it is also used inside the while-loop of > >>>>> virtio_recv_mergeable_pkts(). > >>>>> > >>>>> --yliu > >>>>> > >>>>> > >>>> Yes, seems it has same issue though haven't confirmed with asm code. > >>> So, move the "volatile" qualifier to VIRTQUEUE_NUSED? > >>> > >>> --yliu > >>> > >> Yes, anyway this is just intermediate fix. In next patch, will declare > >> the idx as volatile, and remove the qualifier in the macro. > > Hmm.., why we need an intermediate fix then, if we can come up with an > > ultimate fix very quickly? > > > > --yliu > > > ... Either is OK. I have no preference. Mind to send an ultimate fix then? --yliu ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2016-06-14 13:21 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-05-24 16:16 [PATCH] virtio: use volatile to get used->idx in the loop Huawei Xie 2016-05-25 8:25 ` Xie, Huawei 2016-05-25 8:34 ` Michael S. Tsirkin 2016-05-25 9:47 ` Bruce Richardson 2016-05-25 9:50 ` Michael S. Tsirkin 2016-05-25 10:00 ` Bruce Richardson 2016-05-25 15:24 ` Xie, Huawei 2016-05-30 8:22 ` Yuanhan Liu 2016-06-01 5:40 ` Xie, Huawei 2016-06-01 6:05 ` Yuanhan Liu 2016-06-02 8:39 ` Xie, Huawei 2016-06-02 8:52 ` Yuanhan Liu 2016-06-02 8:54 ` Xie, Huawei 2016-06-14 13:23 ` Yuanhan Liu
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.