From mboxrd@z Thu Jan  1 00:00:00 1970
From: Marcelo Tosatti <mtosatti@redhat.com>
Subject: Re: [patch 04/16] x86: pvclock: create helper for pvclock data
 retrieval
Date: Thu, 1 Nov 2012 18:57:56 -0200
Message-ID: <20121101205756.GC14888@amt.cnet>
References: <20121031224656.417434866@redhat.com>
 <20121031224824.003238142@redhat.com>
 <50928152.2060005@parallels.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Cc: kvm@vger.kernel.org, johnstul@us.ibm.com, jeremy@goop.org,
	zamsden@gmail.com, gleb@redhat.com, avi@redhat.com,
	pbonzini@redhat.com
To: Glauber Costa <glommer@parallels.com>
Return-path: <kvm-owner@vger.kernel.org>
Received: from mx1.redhat.com ([209.132.183.28]:14041 "EHLO mx1.redhat.com"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1759940Ab2KAVGz (ORCPT <rfc822;kvm@vger.kernel.org>);
	Thu, 1 Nov 2012 17:06:55 -0400
Content-Disposition: inline
In-Reply-To: <50928152.2060005@parallels.com>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>

On Thu, Nov 01, 2012 at 06:04:02PM +0400, Glauber Costa wrote:
> On 11/01/2012 02:47 AM, Marcelo Tosatti wrote:
> > +static __always_inline
> > +unsigned __pvclock_read_cycles(const struct pvclock_vcpu_time_info *src,
> > +			       cycle_t *cycles, u8 *flags)
> > +{
> > +	unsigned version;
> > +	cycle_t ret, offset;
> > +	u8 ret_flags;
> > +
> > +	version = src->version;
> > +	rdtsc_barrier();
> > +	offset = pvclock_get_nsec_offset(src);
> > +	ret = src->system_time + offset;
> > +	ret_flags = src->flags;
> > +	rdtsc_barrier();
> > +
> > +	*cycles = ret;
> > +	*flags = ret_flags;
> > +	return version;
> > +}
> > +
> This interface is a bit weird.
> The actual value you are interested in is "cycles", so why is it
> returned through the parameters? I think it would be clearer to have
> this return cycles, and &version as a parameter.

I disagree because

do {
	version = pvclock_read_cycles();
} while (version != src->version);

Looks fine.