From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Xie, Huawei" Subject: Re: [PATCH] virtio: use volatile to get used->idx in the loop Date: Wed, 25 May 2016 15:24:16 +0000 Message-ID: References: <1464106601-981-1-git-send-email-huawei.xie@intel.com> <20160525113224-mutt-send-email-mst@redhat.com> <20160525094729.GA4256@bricha3-MOBL3> <20160525124815-mutt-send-email-mst@redhat.com> <20160525100039.GA7228@bricha3-MOBL3> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Cc: "dev@dpdk.org" , "stephen@networkplumber.org" , "Ananyev, Konstantin" , "thomas.monjalon@6wind.com" , Yuanhan Liu , "Tan, Jianfeng" To: "Richardson, Bruce" , "Michael S. Tsirkin" Return-path: Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) by dpdk.org (Postfix) with ESMTP id ABAEA2BF1 for ; Wed, 25 May 2016 17:24:21 +0200 (CEST) Content-Language: en-US List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On 5/25/2016 6:01 PM, Richardson, Bruce wrote:=0A= > On Wed, May 25, 2016 at 12:50:02PM +0300, Michael S. Tsirkin wrote:=0A= >> On Wed, May 25, 2016 at 10:47:30AM +0100, Bruce Richardson wrote:=0A= >>> On Wed, May 25, 2016 at 11:34:24AM +0300, Michael S. Tsirkin wrote:=0A= >>>> On Wed, May 25, 2016 at 08:25:20AM +0000, Xie, Huawei wrote:=0A= >>>>> On 5/25/2016 4:12 PM, Xie, Huawei wrote:=0A= >>>>>> There is no external function call or any barrier in the loop,=0A= >>>>>> the used->idx would only be retrieved once.=0A= >>>>>>=0A= >>>>>> Signed-off-by: Huawei Xie =0A= >>>>>> ---=0A= >>>>>> drivers/net/virtio/virtio_ethdev.c | 3 ++-=0A= >>>>>> 1 file changed, 2 insertions(+), 1 deletion(-)=0A= >>>>>>=0A= >>>>>> diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio= /virtio_ethdev.c=0A= >>>>>> index c3fb628..f6d6305 100644=0A= >>>>>> --- a/drivers/net/virtio/virtio_ethdev.c=0A= >>>>>> +++ b/drivers/net/virtio/virtio_ethdev.c=0A= >>>>>> @@ -204,7 +204,8 @@ virtio_send_command(struct virtqueue *vq, struct= virtio_pmd_ctrl *ctrl,=0A= >>>>>> usleep(100);=0A= >>>>>> }=0A= >>>>>> =0A= >>>>>> - while (vq->vq_used_cons_idx !=3D vq->vq_ring.used->idx) {=0A= >>>>>> + while (vq->vq_used_cons_idx !=3D=0A= >>>>>> + *((volatile uint16_t *)(&vq->vq_ring.used->idx))) {=0A= >>>>>> uint32_t idx, desc_idx, used_idx;=0A= >>>>>> struct vring_used_elem *uep;=0A= >>>>>> =0A= >>>>> Find this issue when do the code rework of RX/TX queue.=0A= >>>>> As in other places, we also have loop retrieving the value of avial->= idx=0A= >>>>> or used->idx, i prefer to declare the index in vq structure as volati= le=0A= >>>>> to avoid potential issue.=0A= >>> Is there a reason why the value is not always volatile? I would have th= ought=0A= >>> it would be generally safer to mark the actual value as volatile inside= the=0A= >>> structure definition itself? In any cases where we do want to store the= value=0A= >>> locally and not re-access the structure, a local variable can be used.= =0A= >>>=0A= >>> Regards,=0A= >>> /Bruce=0A= >> Linux generally discourages volatile as a general style guidance:=0A= >> https://www.kernel.org/doc/Documentation/volatile-considered-harmful.txt= =0A= >> it doesn't have to apply to dpdk which has a different coding style=0A= >> but IIUC this structure is inherited from linux, deviating=0A= >> will make keeping things up to date harder.=0A= > The prohibition on volatile indeed doesn't apply to DPDK, due to the fact= that=0A= > we so seldom use locks, and do a lot of direct register accesses in out P= MDs.=0A= > [I also still have the scars from previous issues where we had nice subtl= e bugs=0A= > in our PMDs - which only occurred with specific subversions of gcc - all = due=0A= > to a missing "volatile" on one structure element.]=0A= >=0A= > However, in this case, I take your point about keeping things consistent = with=0A= > the kernel. :-)=0A= =0A= At least for virtio PMD, we have to support both Linux and FreeBSD, so=0A= DPDK defines its own vring structure instead of including linux header file= .=0A= Two solutions for this volatile issue, 1) declare used->idx and=0A= avail->idx as volatile 2) define similar=0A= access_once/read_once/write_once macro.=0A= Would take the first one. In future, we could consider define=0A= access_once, and apply to all other data structures if we want to use=0A= the kernel style.=0A= =0A= One thing i am confusing is other DPDK components include Linux header=0A= files, do they compile on FreeBSD?=0A= =0A= >=0A= > /Bruce=0A= >=0A= >>>> It might be a good idea to wrap this in a macro=0A= >>>> similar to ACCESS_ONCE in Linux.=0A= >>>>=0A= >>>>> Stephen:=0A= >>>>> Another question is why we need a loop here?=0A= >>>>>=0A= >>>>> /huawei=0A= >>>> -- =0A= >>>> MST=0A= =0A=