From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1161296AbbKFMAj (ORCPT <rfc822;w@1wt.eu>);
	Fri, 6 Nov 2015 07:00:39 -0500
Received: from foss.arm.com ([217.140.101.70]:41475 "EHLO foss.arm.com"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1031835AbbKFMAh (ORCPT <rfc822;linux-kernel@vger.kernel.org>);
	Fri, 6 Nov 2015 07:00:37 -0500
Date: Fri, 6 Nov 2015 12:00:24 +0000
From: Mark Rutland <mark.rutland@arm.com>
To: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: xen-devel@lists.xensource.com, linux@arm.linux.org.uk,
        Ian Campbell <ian.campbell@citrix.com>, arnd@arndb.de,
        marc.zyngier@arm.com, catalin.marinas@arm.com, konrad.wilk@oracle.com,
        will.deacon@arm.com, linux-kernel@vger.kernel.org, olof@lixom.net,
        linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH v11 1/5] xen: move xen_setup_runstate_info and
 get_runstate_snapshot to drivers/xen/time.c
Message-ID: <20151106120024.GB23038@leverpostej>
References: <alpine.DEB.2.02.1511051502270.13541@kaball.uk.xensource.com>
 <1446737696-9749-1-git-send-email-stefano.stabellini@eu.citrix.com>
 <20151105164820.GF32247@leverpostej>
 <alpine.DEB.2.02.1511061110220.13541@kaball.uk.xensource.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <alpine.DEB.2.02.1511061110220.13541@kaball.uk.xensource.com>
User-Agent: Mutt/1.5.21 (2010-09-15)
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Fri, Nov 06, 2015 at 11:11:40AM +0000, Stefano Stabellini wrote:
> On Thu, 5 Nov 2015, Mark Rutland wrote:
> > Hi,
> > 
> > > +static u64 get64(const u64 *p)
> > > +{
> > > +	u64 ret;
> > > +
> > > +	if (BITS_PER_LONG < 64) {
> > > +		u32 *p32 = (u32 *)p;
> > > +		u32 h, l;
> > > +
> > > +		/*
> > > +		 * Read high then low, and then make sure high is
> > > +		 * still the same; this will only loop if low wraps
> > > +		 * and carries into high.
> > > +		 * XXX some clean way to make this endian-proof?
> > > +		 */
> > > +		do {
> > > +			h = p32[1];
> > > +			barrier();
> > > +			l = p32[0];
> > > +			barrier();
> > > +		} while (p32[1] != h);
> > 
> > I realise this is simply a move of existing code, but it may be better
> > to instead have:
> > 
> > do {
> > 	h = READ_ONCE(p32[1]);
> > 	l = READ_ONCE(p32[0]);
> > } while (READ_ONCE(p32[1] != h);
> > 
> > Which ensures that each load is a single access (though it almost
> > certainly would be anyway), and prevents the compiler from having to
> > reload any other memory locations (which the current barrier() usage
> > forces).
> 
> I am happy to make these changes, however for code clarity and review
> simplicity I'll keep them on a separate patch (I like code movement to
> remain code movement). I can squash the two patches together when
> committing, if necessary.

Sure, I also prefer to separate code movement from code rework, so that
makes sense to me.

Thanks,
Mark.