From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mailout1.w1.samsung.com (mailout1.w1.samsung.com [210.118.77.11]) by dpdk.org (Postfix) with ESMTP id 6319F3238 for ; Fri, 7 Dec 2018 12:17:02 +0100 (CET) Received: from eucas1p1.samsung.com (unknown [182.198.249.206]) by mailout1.w1.samsung.com (KnoxPortal) with ESMTP id 20181207111701euoutp013d76e9e49b320e10919171ccebf7966d~uCNJB5tZm1468414684euoutp01p for ; Fri, 7 Dec 2018 11:17:01 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 mailout1.w1.samsung.com 20181207111701euoutp013d76e9e49b320e10919171ccebf7966d~uCNJB5tZm1468414684euoutp01p DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=samsung.com; s=mail20170921; t=1544181421; bh=siBWOpDK+hFG1eUPcI6Ldkd6kRv64i+cb3rXztk7TCs=; h=Subject:To:From:Date:In-Reply-To:References:From; b=HkP/J2IiBznBpKiau5DrgxwaL+pArDmf/DMun0PFC8Hbr1WxvbUwk5WznBIHC1KRu g5JsJ7iUko+locyxnBHPLTFlTU7NWAyseP8/gdNcXqG55Ar54IrMxauZzETCTj4sUt 7tSmrvdB8fxLnpLg6xpCNO6npcuw9xNVsnE6l154= Received: from eusmges2new.samsung.com (unknown [203.254.199.244]) by eucas1p2.samsung.com (KnoxPortal) with ESMTP id 20181207111700eucas1p238224e55c0aaec1120ba96844b2d7588~uCNIkmN-I3233232332eucas1p2i; Fri, 7 Dec 2018 11:17:00 +0000 (GMT) Received: from eucas1p1.samsung.com ( [182.198.249.206]) by eusmges2new.samsung.com (EUCPMTA) with SMTP id B5.4D.04294.CA65A0C5; Fri, 7 Dec 2018 11:17:00 +0000 (GMT) Received: from eusmtrp1.samsung.com (unknown [182.198.249.138]) by eucas1p2.samsung.com (KnoxPortal) with ESMTPA id 20181207111659eucas1p2dc02ec5fbfeb2e2f7c118aa197245ac9~uCNHlIR5c3232232322eucas1p2c; Fri, 7 Dec 2018 11:16:59 +0000 (GMT) Received: from eusmgms1.samsung.com (unknown [182.198.249.179]) by eusmtrp1.samsung.com (KnoxPortal) with ESMTP id 20181207111659eusmtrp133dc6260e7f479c11d3ec374629f861c~uCNHWnjtp3177531775eusmtrp13; Fri, 7 Dec 2018 11:16:59 +0000 (GMT) X-AuditID: cbfec7f4-84fff700000010c6-7a-5c0a56ac1c09 Received: from eusmtip1.samsung.com ( [203.254.199.221]) by eusmgms1.samsung.com (EUCPMTA) with SMTP id 11.DD.04284.BA65A0C5; Fri, 7 Dec 2018 11:16:59 +0000 (GMT) Received: from [106.109.129.180] (unknown [106.109.129.180]) by eusmtip1.samsung.com (KnoxPortal) with ESMTPA id 20181207111659eusmtip12308863934e38fce77be4b3b73768b62~uCNG3WjLe0093400934eusmtip1j; Fri, 7 Dec 2018 11:16:59 +0000 (GMT) To: Maxime Coquelin , dev@dpdk.org, jfreimann@redhat.com, tiwei.bie@intel.com, zhihong.wang@intel.com, jasowang@redhat.com From: Ilya Maximets Message-ID: Date: Fri, 7 Dec 2018 14:16:53 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.2.1 MIME-Version: 1.0 In-Reply-To: Content-Language: en-GB Content-Transfer-Encoding: 8bit X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFlrBKsWRmVeSWpSXmKPExsWy7djPc7prwrhiDHbvU7V492k7k8WV9p/s FssufWayOLdmKYvFsc49LBZbG/4zWWy+OInJgd3j14KlrB6L97xk8ni/7yqbR9+WVYwBLFFc NimpOZllqUX6dglcGacmH2Yv+KNV0fzlBnMD4yalLkZODgkBE4nFR3aydDFycQgJrGCU2P91 KiuE84VR4t3rl2wQzmdGiZcbmllgWvZsW88EkVjOKLG8/wiU85FR4tOKiawgVcICVhITvn1k BkmICMxilGh6PZMNJMEmoCNxavURRhCbV8BOYtPqiWA2i4CKxMxli8FWiApESHTcX80GUSMo cXLmE7A4J1D95X+N7CA2s4C4RNOXlawQtrxE89bZYMskBBaxS0y4toMN4lYXie9N9xkhbGGJ V8e3sEPYMhKnJ/dA/VMvcb/lJSNEcwejxPRD/5ggEvYSW16fA2rgANqgKbF+lz5E2FFiw9Q7 bCBhCQE+iRtvBSFu4JOYtG06M0SYV6KjTQiiWkXi98HlzBC2lMTNd5+hLvCQ+Lh3LcsERsVZ SL6cheSzWUg+m4VwwwJGllWM4qmlxbnpqcVGeanlesWJucWleel6yfm5mxiB6ef0v+NfdjDu +pN0iFGAg1GJh7fCgTNGiDWxrLgy9xCjBAezkgivki1XjBBvSmJlVWpRfnxRaU5q8SFGaQ4W JXHeaoYH0UIC6YklqdmpqQWpRTBZJg5OqQbGyq8hk1sDX5aI88gwH/TfkGAodNhs56yotizX mknvVpzTaTV29PxZZLbq9Prtzn4rL/J/KjlUtXC9+PH76v9P/25PZD0d1SbyoW/SNpfvqxsu nXhxs/SIp+/uAv+6b5dPbcxrsDzIrnCo6w7rReMbgfsXGgbu73B+5C0esMQ7qzbtQZZodOlk JZbijERDLeai4kQA4wbLhzsDAAA= X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFlrHIsWRmVeSWpSXmKPExsVy+t/xu7qrw7hiDK7dkbN492k7k8WV9p/s FssufWayOLdmKYvFsc49LBZbG/4zWWy+OInJgd3j14KlrB6L97xk8ni/7yqbR9+WVYwBLFF6 NkX5pSWpChn5xSW2StGGFkZ6hpYWekYmlnqGxuaxVkamSvp2NimpOZllqUX6dgl6GacmH2Yv +KNV0fzlBnMD4yalLkZODgkBE4k929YzdTFycQgJLGWU+DjnJhNEQkrix68LrBC2sMSfa11s ILaQwHtGif4r2SC2sICVxIRvH5lBmkUEZjFKrDwBUgQy6TejRFPnb7BJbAI6EqdWH2EEsXkF 7CQ2rZ4IZrMIqEjMXLaYBcQWFYiQOPtyHVSNoMTJmU/A4pxA9Zf/NbKD2MwC6hJ/5l1ihrDF JZq+rGSFsOUlmrfOZp7AKDgLSfssJC2zkLTMQtKygJFlFaNIamlxbnpusaFecWJucWleul5y fu4mRmBcbTv2c/MOxksbgw8xCnAwKvHwVjhwxgixJpYVV+YeYpTgYFYS4VWy5YoR4k1JrKxK LcqPLyrNSS0+xGgK9NxEZinR5HxgzOeVxBuaGppbWBqaG5sbm1koifOeN6iMEhJITyxJzU5N LUgtgulj4uCUamDsOToj5fdTA/GU9oCHs17uzAyrein7eveP5zwiCu5zpjSZ5XG8kN67RL/h RMPmoqtqG0//Kw+LcN2yV6LCb+3bp1J1pnUpme/7kmrDnz1zm/tU2T788Z9vDKe+HBe34ngW KVcS8mqiOONj1VV//1RzL32/OSnfYprCvpi5sYZcpz9P2ZhybdJeJZbijERDLeai4kQAw2qX JcECAAA= X-CMS-MailID: 20181207111659eucas1p2dc02ec5fbfeb2e2f7c118aa197245ac9 X-Msg-Generator: CA Content-Type: text/plain; charset="utf-8" X-RootMTR: 20181205135231eucas1p1c89281f6525a0fedab4a2fc0d2e21393 X-EPHeader: CA CMS-TYPE: 201P X-CMS-RootMailID: 20181205135231eucas1p1c89281f6525a0fedab4a2fc0d2e21393 References: <20181205094957.1938-6-maxime.coquelin@redhat.com> <1a4073a9-fc9b-a3c6-5761-7269fe955745@samsung.com> Subject: Re: [dpdk-dev] [5/5] vhost: remove useless casts to volatile X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 07 Dec 2018 11:17:02 -0000 On 06.12.2018 19:59, Maxime Coquelin wrote: > Hi Ilya, > > On 12/5/18 2:52 PM, Ilya Maximets wrote: >> On 05.12.2018 12:49, Maxime Coquelin wrote: >>> Cast to volatile is done when reading avail index and writing >>> the used index. This would not be necessary if proper barriers >>> are used. >> >> 'volatile' and barriers are not really connected. 'volatile' is >> the disabling of the compiler optimizations, while barriers are >> for runtime CPU level optimizations. In general, casts here made >> to force compiler to actually read the value and not cache it >> somehow. In fact that vhost library never writes to avail index, >> "very smart" compiler could drop it at all. None of modern compilers >> will do that for a single operation within a function, so, >> volatiles are not really necessary in current code, but they could >> save some nerves in case of code/compiler changes. > > Ok, thanks for the explanation. > Why don't we do the same in Virtio PMD? Maybe we should. It works because in virtio all the accesses wrapped by short access functions like 'vq_update_avail_idx'. And we, actually, never reading the same value twice in the same function. Compilers today does not optimize such memory accesses. > >> OTOH, IMHO, the main purpose of the casts in current code is >> the self-documenting. Casts forces to pay special attention to >> these variables and reminds that they could be updated in other >> process. Casts allows to understand which variables are local and >> which are shared. I don't think that we should remove them anyway. > > It is not only self-documenting, it has an impact on generated code: > >>> >>> Now that the read barrier has been added, we can remove these >>> cast to volatile. >>> >>> Signed-off-by: Maxime Coquelin >>> --- >>>   lib/librte_vhost/virtio_net.c | 7 +++---- >>>   1 file changed, 3 insertions(+), 4 deletions(-) >>> >>> diff --git a/lib/librte_vhost/virtio_net.c b/lib/librte_vhost/virtio_net.c >>> index 679ce388b..eab1a5b4c 100644 >>> --- a/lib/librte_vhost/virtio_net.c >>> +++ b/lib/librte_vhost/virtio_net.c >>> @@ -114,7 +114,7 @@ flush_shadow_used_ring_split(struct virtio_net *dev, struct vhost_virtqueue *vq) >>>         vhost_log_cache_sync(dev, vq); >>>   -    *(volatile uint16_t *)&vq->used->idx += vq->shadow_used_idx; >>> +    vq->used->idx += vq->shadow_used_idx; > > With cast to volatile: >     *(volatile uint16_t *)&vq->used->idx += vq->shadow_used_idx; >     35f8:    49 8b 53 10              mov    0x10(%r11),%rdx >     vq->shadow_used_idx = 0; >     35fc:    31 db                    xor    %ebx,%ebx >     *(volatile uint16_t *)&vq->used->idx += vq->shadow_used_idx; >     35fe:    0f b7 42 02              movzwl 0x2(%rdx),%eax >     3602:    66 41 03 43 70           add    0x70(%r11),%ax >     3607:    66 89 42 02              mov    %ax,0x2(%rdx) >     vq->shadow_used_idx = 0; > > Without it: >     vq->used->idx += vq->shadow_used_idx; >     35f8:    49 8b 43 10              mov    0x10(%r11),%rax >     35fc:    41 0f b7 53 70           movzwl 0x70(%r11),%edx >     vq->shadow_used_idx = 0; >     3601:    31 db                    xor    %ebx,%ebx >     vq->used->idx += vq->shadow_used_idx; >     3603:    66 01 50 02              add    %dx,0x2(%rax) >     vq->shadow_used_idx = 0; > > If my understanding is correct there is no functional change, but we save one instruction by removing the cast to volatile. IMHO, it's a gcc issue that it could not understand that cast and dereference could be dropped. For example, clang on my ubuntu generates equal code: With cast to volatile: *(volatile uint16_t *)&vq->used->idx += vq->shadow_used_idx; 32550: 41 0f b7 42 70 movzwl 0x70(%r10),%eax 32555: 49 8b 4a 10 mov 0x10(%r10),%rcx 32559: 66 01 41 02 add %ax,0x2(%rcx) vq->shadow_used_idx = 0; 3255d: 66 41 c7 42 70 00 00 movw $0x0,0x70(%r10) Without it: vq->used->idx += vq->shadow_used_idx; 32550: 41 0f b7 42 70 movzwl 0x70(%r10),%eax 32555: 49 8b 4a 10 mov 0x10(%r10),%rcx 32559: 66 01 41 02 add %ax,0x2(%rcx) vq->shadow_used_idx = 0; 3255d: 66 41 c7 42 70 00 00 movw $0x0,0x70(%r10) However, different code appears only in '+=' case. Why we have this increment at all? Following change will eliminate the generated code difference: diff --git a/lib/librte_vhost/virtio_net.c b/lib/librte_vhost/virtio_net.c index 5e1a1a727..5776975ca 100644 --- a/lib/librte_vhost/virtio_net.c +++ b/lib/librte_vhost/virtio_net.c @@ -114,7 +114,7 @@ flush_shadow_used_ring_split(struct virtio_net *dev, struct vhost_virtqueue *vq) vhost_log_cache_sync(dev, vq); - *(volatile uint16_t *)&vq->used->idx += vq->shadow_used_idx; + *(volatile uint16_t *)&vq->used->idx = vq->last_used_idx; vq->shadow_used_idx = 0; vhost_log_used_vring(dev, vq, offsetof(struct vring_used, idx), sizeof(vq->used->idx)); --- What do you think? Best regards, Ilya Maximets.