From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751719AbdJXQm0 (ORCPT ); Tue, 24 Oct 2017 12:42:26 -0400 Received: from mail.kernel.org ([198.145.29.99]:35714 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751319AbdJXQmX (ORCPT ); Tue, 24 Oct 2017 12:42:23 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 0128121871 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, 24 Oct 2017 09:42:22 -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 v5 08/13] xen/pvcalls: implement accept command In-Reply-To: Message-ID: References: <1507336227-20477-1-git-send-email-sstabellini@kernel.org> <1507336227-20477-8-git-send-email-sstabellini@kernel.org> 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 On Tue, 24 Oct 2017, Boris Ostrovsky wrote: > On 10/23/2017 07:03 PM, Stefano Stabellini wrote: > > On Tue, 17 Oct 2017, Boris Ostrovsky wrote: > >> On 10/06/2017 08:30 PM, Stefano Stabellini wrote: > >>> + /* > >>> + * Backend only supports 1 inflight accept request, will return > >>> + * errors for the others > >>> + */ > >>> + if (test_and_set_bit(PVCALLS_FLAG_ACCEPT_INFLIGHT, > >>> + (void *)&map->passive.flags)) { > >>> + req_id = READ_ONCE(map->passive.inflight_req_id); > >>> + if (req_id != PVCALLS_INVALID_ID && > >>> + READ_ONCE(bedata->rsp[req_id].req_id) == req_id) { > >> > >> READ_ONCE (especially the second one)? I know I may sound fixated on > >> this but I really don't understand how compiler may do anything wrong if > >> straight reads were used. > >> > >> For the first case, I guess, theoretically the compiler may decide to > >> re-fetch map->passive.inflight_req_id. But even if it did, would that be > >> a problem? Both of these READ_ONCE targets are updated below before > >> PVCALLS_FLAG_ACCEPT_INFLIGHT is cleared so there should not be any > >> change between re-fetching, I think. (The only exception is the noblock > >> case, which does WRITE_ONCE that don't understand either) > > READ_ONCE is reasonably cheap: do we really want to have this kind of > > conversation every time we touch this code in the future? Personally, I > > would have used READ/WRITE_ONCE everywhere for inflight_req_id and > > req_id, because it makes the code easier to understand. > > I guess it's a matter of opinion. I actually think it's harder to read. > > But it doesn't make the code wrong so... > > > > > We have already limited their usage, but at least we have followed a set > > of guidelines. Doing further optimizations on this code seems > > unnecessary and prone to confuse the reader. > > > > > > >>> + ret = create_active(map2, &evtchn); > >>> + if (ret < 0) { > >>> + kfree(map2); > >>> + clear_bit(PVCALLS_FLAG_ACCEPT_INFLIGHT, > >>> + (void *)&map->passive.flags); > >>> + spin_unlock(&bedata->socket_lock); > >>> + pvcalls_exit(); > >>> + return -ENOMEM; > >> Why not ret? > > yes, good idea. > > With that fixed (and extra space removed in 'ret = create_active(map2, > &evtchn);') > > Reviewed-by: Boris Ostrovsky Thank you!