From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932253AbdJXNvZ (ORCPT ); Tue, 24 Oct 2017 09:51:25 -0400 Received: from userp1040.oracle.com ([156.151.31.81]:38687 "EHLO userp1040.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751267AbdJXNvY (ORCPT ); Tue, 24 Oct 2017 09:51:24 -0400 Subject: Re: [PATCH v5 08/13] xen/pvcalls: implement accept command To: Stefano Stabellini References: <1507336227-20477-1-git-send-email-sstabellini@kernel.org> <1507336227-20477-8-git-send-email-sstabellini@kernel.org> Cc: xen-devel@lists.xen.org, linux-kernel@vger.kernel.org, jgross@suse.com, Stefano Stabellini From: Boris Ostrovsky Message-ID: Date: Tue, 24 Oct 2017 09:52:53 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-Source-IP: aserv0022.oracle.com [141.146.126.234] Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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