Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1417037 > unrolled thread

Re: [PATCH 3/3 V2] pvclock: Get rid of __pvclock_read_cycles in function pvclock_read_flags

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 3/3 V2] pvclock: Get rid of __pvclock_read_cycles in  function pvclock_read_flags Borislav Petkov <bp@suse.de> - 2016-06-08 10:20 +0200
    Re: [PATCH 3/3 V2] pvclock: Get rid of __pvclock_read_cycles in  function pvclock_read_flags Paolo Bonzini <pbonzini@redhat.com> - 2016-06-09 13:30 +0200
      Re: [PATCH 3/3 V2] pvclock: Get rid of __pvclock_read_cycles in  function pvclock_read_flags Borislav Petkov <bp@suse.de> - 2016-06-09 13:30 +0200
        Re: [PATCH 3/3 V2] pvclock: Get rid of __pvclock_read_cycles in  function pvclock_read_flags Paolo Bonzini <pbonzini@redhat.com> - 2016-06-09 14:40 +0200

#1417037 — Re: [PATCH 3/3 V2] pvclock: Get rid of __pvclock_read_cycles in function pvclock_read_flags

On Sat, May 28, 2016 at 08:27:43PM +0800, Minfei Huang wrote:
> There is a generic function __pvclock_read_cycles to be used to get both
> flags and cycles. For function pvclock_read_flags, it's useless to get
> cycles value. To make this function be more effective, get this variable
> flags directly in function.
> 
> Signed-off-by: Minfei Huang <mnghuan@gmail.com>
> ---
> v1:
> - Get rid of __pvclock_read_cycles according to Andy's suggestion
> ---
>  arch/x86/kernel/pvclock.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kernel/pvclock.c b/arch/x86/kernel/pvclock.c
> index 7f82fe0..06c58ce 100644
> --- a/arch/x86/kernel/pvclock.c
> +++ b/arch/x86/kernel/pvclock.c
> @@ -61,11 +61,14 @@ void pvclock_resume(void)
>  u8 pvclock_read_flags(struct pvclock_vcpu_time_info *src)
>  {
>  	unsigned version;
> -	cycle_t ret;
>  	u8 flags;
>  
>  	do {
> -		version = __pvclock_read_cycles(src, &ret, &flags);
> +		version = src->version;
> +		/* Make the latest version visible */
> +		smp_rmb();
> +
> +		flags = src->flags;
>  		/* Make sure that the version double-check is last. */

What does that comment mean over the barrier? It should be over the
"while" line IMO.

>  		smp_rmb();

Why the two barriers back-to-back? Can't have one at the end for all?

>  	} while ((src->version & 1) || version != src->version);

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
-- 

[toc] | [next] | [standalone]

#1418244

On 08/06/2016 10:17, Borislav Petkov wrote:
>> > -		version = __pvclock_read_cycles(src, &ret, &flags);
>> > +		version = src->version;
>> > +		/* Make the latest version visible */
>> > +		smp_rmb();
>> > +
>> > +		flags = src->flags;
>> >  		/* Make sure that the version double-check is last. */
> What does that comment mean over the barrier? It should be over the
> "while" line IMO.
> 
>> >  		smp_rmb();
> Why the two barriers back-to-back? Can't have one at the end for all?
> 

This is basically implementing a seqcount.  It needs two barriers and,
technically, they should be virt_rmb() -- it really doesn't matter of
course because reads are never reordered on x86.

Paolo

[toc] | [prev] | [next] | [standalone]

#1418245

On Thu, Jun 09, 2016 at 01:21:18PM +0200, Paolo Bonzini wrote:
> This is basically implementing a seqcount.  It needs two barriers and,

Why does it need the two barriers? More details please.

> technically, they should be virt_rmb() -- it really doesn't matter of
> course because reads are never reordered on x86.

You mean

	version = src->version;
	flags = src->flags;

are not reordered?

I don't think so.

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
-- 

[toc] | [prev] | [next] | [standalone]

#1418286

On 09/06/2016 13:28, Borislav Petkov wrote:
>> > technically, they should be virt_rmb() -- it really doesn't matter of
>> > course because reads are never reordered on x86.
> You mean
> 
> 	version = src->version;
> 	flags = src->flags;
> 
> are not reordered?
> 
> I don't think so.

The compiler can reorder them, so smp_rmb() and virt_rmb() have to be
barrier(), but the processor won't.

x86 will only move a store after a subsequent load, if you exclude
special cases such as write combining, non-temporal moves and the like.
LFENCE and SFENCE are only needed for those special cases.

Paolo

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web