Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751469AbdILXNU (ORCPT ); Tue, 12 Sep 2017 19:13:20 -0400 Received: from mail.kernel.org ([198.145.29.99]:56562 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751003AbdILXNT (ORCPT ); Tue, 12 Sep 2017 19:13:19 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 7004D214C5 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=kernel.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=sstabellini@kernel.org Date: Tue, 12 Sep 2017 16:13:17 -0700 (PDT) From: Stefano Stabellini X-X-Sender: sstabellini@sstabellini-ThinkPad-X260 To: Boris Ostrovsky cc: Stefano Stabellini , xen-devel@lists.xen.org, linux-kernel@vger.kernel.org, jgross@suse.com, Stefano Stabellini Subject: Re: [PATCH v3 11/13] xen/pvcalls: implement poll command In-Reply-To: <73a6ae0f-077c-9d42-fd56-0738ccb30cec@oracle.com> Message-ID: References: <1501541855-7354-1-git-send-email-sstabellini@kernel.org> <1501541855-7354-11-git-send-email-sstabellini@kernel.org> <702cfa9c-5f14-07a3-63ba-93648ff66d9b@oracle.com> <6c9613d8-8219-d06f-9095-fa57474ed518@oracle.com> <73a6ae0f-077c-9d42-fd56-0738ccb30cec@oracle.com> User-Agent: Alpine 2.10 (DEB 1266 2009-07-14) MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2168 Lines: 49 On Tue, 12 Sep 2017, Boris Ostrovsky wrote: > On 09/12/2017 06:17 PM, Stefano Stabellini wrote: > > On Tue, 12 Sep 2017, Boris Ostrovsky wrote: > >>>>> + > >>>>> +unsigned int pvcalls_front_poll(struct file *file, struct socket *sock, > >>>>> + poll_table *wait) > >>>>> +{ > >>>>> + struct pvcalls_bedata *bedata; > >>>>> + struct sock_mapping *map; > >>>>> + > >>>>> + if (!pvcalls_front_dev) > >>>>> + return POLLNVAL; > >>>>> + bedata = dev_get_drvdata(&pvcalls_front_dev->dev); > >>>>> + > >>>>> + map = (struct sock_mapping *) READ_ONCE(sock->sk->sk_send_head); > >>>> I just noticed this --- why is it READ_ONCE? Are you concerned that > >>>> sk_send_head may change? > >>> No, but I wanted to avoid partial reads. A caller could call > >>> pvcalls_front_accept and pvcalls_front_poll on newsock almost at the > >>> same time (it is probably not the correct way to use the API), I wanted > >>> to make sure that "map" is either read correctly, or not read at all. > >> How can you have a partial read on a pointer? > > I don't think that the compiler makes any promises on translating a > > pointer read into a single read instruction. Of couse, I expect gcc to > > actually do it without any need for READ/WRITE_ONCE. > > READ_ONCE() only guarantees ordering but not atomicity. It resolves (for > 64-bit pointers) to > > case 8: *(__u64 *)res = *(volatile __u64 *)p; break; > > so if compiler was breaking accesses into two then nothing would have > prevented it from breaking them here (I don't think volatile declaration > would affect this). Moreover, for sizes >8 bytes READ_ONCE() is > __builtin_memcpy() which is definitely not atomic. > > So you can't rely on READ_ONCE being atomic from that perspective. I thought that READ_ONCE guaranteed atomicity for sizes less or equal to the machine word size. It doesn't make any atomicity guarantees for sizes >8 bytes. > OTOH, I am pretty sure pointer accesses are guaranteed to be atomic. For > example, atomic64_read() is READ_ONCE(u64), which (per above) is > dereferencing of a 64-bit pointer in C. I am happy to remove the READ_ONCE and WRITE_ONCE, if we all think it is safe.