[PATCH] Cleanup __pvclock_read_cycles to remove useless variables

[PATCH] Cleanup __pvclock_read_cycles to remove useless variables

[Minfei Huang]

The value of cycles and flags can be assigned directly without
intermediate variables.

Remove the useless variables.

Signed-off-by: Minfei Huang <mnghuan@gmail.com>
---
 arch/x86/include/asm/pvclock.h | 15 ++++-----------
 1 file changed, 4 insertions(+), 11 deletions(-)

diff --git a/arch/x86/include/asm/pvclock.h b/arch/x86/include/asm/pvclock.h
index fdcc040..fb95dac 100644
--- a/arch/x86/include/asm/pvclock.h
+++ b/arch/x86/include/asm/pvclock.h
@@ -80,19 +80,12 @@ 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;
+	cycle_t offset;
 
 	offset = pvclock_get_nsec_offset(src);
-	ret = src->system_time + offset;
-	ret_flags = src->flags;
-
-	*cycles = ret;
-	*flags = ret_flags;
-	return version;
+	*cycles = src->system_time + offset;
+	*flags = src->flags;
+	return src->version;
 }
 
 struct pvclock_vsyscall_time_info {
-- 
2.6.3

Re: [PATCH] Cleanup __pvclock_read_cycles to remove useless variables

[Minfei Huang]

ping.

Any comment is appreciate.

Thanks
Minfei

On 04/25/16 at 02:53P, Minfei Huang wrote:
> The value of cycles and flags can be assigned directly without
> intermediate variables.
> 
> Remove the useless variables.
> 
> Signed-off-by: Minfei Huang <mnghuan@gmail.com>
> ---
>  arch/x86/include/asm/pvclock.h | 15 ++++-----------
>  1 file changed, 4 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/x86/include/asm/pvclock.h b/arch/x86/include/asm/pvclock.h
> index fdcc040..fb95dac 100644
> --- a/arch/x86/include/asm/pvclock.h
> +++ b/arch/x86/include/asm/pvclock.h
> @@ -80,19 +80,12 @@ 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;
> +	cycle_t offset;
>  
>  	offset = pvclock_get_nsec_offset(src);
> -	ret = src->system_time + offset;
> -	ret_flags = src->flags;
> -
> -	*cycles = ret;
> -	*flags = ret_flags;
> -	return version;
> +	*cycles = src->system_time + offset;
> +	*flags = src->flags;
> +	return src->version;
>  }
>  
>  struct pvclock_vsyscall_time_info {
> -- 
> 2.6.3
> 

Re: [PATCH] Cleanup __pvclock_read_cycles to remove useless variables

[Borislav Petkov]

On Mon, Apr 25, 2016 at 02:53:14PM +0800, Minfei Huang wrote:
> The value of cycles and flags can be assigned directly without
> intermediate variables.
> 
> Remove the useless variables.
> 
> Signed-off-by: Minfei Huang <mnghuan@gmail.com>
> ---
>  arch/x86/include/asm/pvclock.h | 15 ++++-----------
>  1 file changed, 4 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/x86/include/asm/pvclock.h b/arch/x86/include/asm/pvclock.h
> index fdcc040..fb95dac 100644
> --- a/arch/x86/include/asm/pvclock.h
> +++ b/arch/x86/include/asm/pvclock.h
> @@ -80,19 +80,12 @@ 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;
> +	cycle_t offset;
>  
>  	offset = pvclock_get_nsec_offset(src);

You could go a step further and get rid of that
pvclock_get_nsec_offset() simple wrapper too and move its meat into
__pvclock_read_cycles()...

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH] Cleanup __pvclock_read_cycles to remove useless variables

[Andy Lutomirski]

On Sat, Apr 30, 2016 at 6:24 AM, Borislav Petkov <bp@suse.de> wrote:
> On Mon, Apr 25, 2016 at 02:53:14PM +0800, Minfei Huang wrote:
>> The value of cycles and flags can be assigned directly without
>> intermediate variables.
>>
>> Remove the useless variables.
>>
>> Signed-off-by: Minfei Huang <mnghuan@gmail.com>
>> ---
>>  arch/x86/include/asm/pvclock.h | 15 ++++-----------
>>  1 file changed, 4 insertions(+), 11 deletions(-)
>>
>> diff --git a/arch/x86/include/asm/pvclock.h b/arch/x86/include/asm/pvclock.h
>> index fdcc040..fb95dac 100644
>> --- a/arch/x86/include/asm/pvclock.h
>> +++ b/arch/x86/include/asm/pvclock.h
>> @@ -80,19 +80,12 @@ 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;
>> +     cycle_t offset;
>>
>>       offset = pvclock_get_nsec_offset(src);
>
> You could go a step further and get rid of that
> pvclock_get_nsec_offset() simple wrapper too and move its meat into
> __pvclock_read_cycles()...
>

Take a look at vread_pvclock.  I decided that __pvclock_read_cycles
was too ugly to use and was very slow and I just gave up and rewrote
it.

> --
> Regards/Gruss,
>     Boris.
>
> SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
> --



-- 
Andy Lutomirski
AMA Capital Management, LLC

Re: [PATCH] Cleanup __pvclock_read_cycles to remove useless variables

[Borislav Petkov]

On Sat, Apr 30, 2016 at 10:47:49AM -0700, Andy Lutomirski wrote:
> Take a look at vread_pvclock.  I decided that __pvclock_read_cycles
> was too ugly to use and was very slow and I just gave up and rewrote
> it.

Should we kill __pvclock_read_cycles in favor of vread_pvclock? It looks
doable at a quick scan...

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH] Cleanup __pvclock_read_cycles to remove useless variables

[Andy Lutomirski]

On Apr 30, 2016 12:17 PM, "Borislav Petkov" <bp@suse.de> wrote:
>
> On Sat, Apr 30, 2016 at 10:47:49AM -0700, Andy Lutomirski wrote:
> > Take a look at vread_pvclock.  I decided that __pvclock_read_cycles
> > was too ugly to use and was very slow and I just gave up and rewrote
> > it.
>
> Should we kill __pvclock_read_cycles in favor of vread_pvclock? It looks
> doable at a quick scan...
>

The in-kernel version might have to be a bit different because it
needs to handle the !stable case.  If !stable, it should just use the
current CPU's copy which means that, realistically, it should just
get_cpu and use the local copy unconditionally.  Other than that, it
could look a lot like the vread_pvclock variant.

But I agree, the current thing is incomprehensible.

--Andy

> --
> Regards/Gruss,
>     Boris.
>
> SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
> --

Re: [PATCH] Cleanup __pvclock_read_cycles to remove useless variables

[Paolo Bonzini]

On 30/04/2016 23:57, Andy Lutomirski wrote:
>> > Should we kill __pvclock_read_cycles in favor of vread_pvclock? It looks
>> > doable at a quick scan...
>> >
> The in-kernel version might have to be a bit different because it
> needs to handle the !stable case.  If !stable, it should just use the
> current CPU's copy which means that, realistically, it should just
> get_cpu and use the local copy unconditionally.  Other than that, it
> could look a lot like the vread_pvclock variant.
> 
> But I agree, the current thing is incomprehensible.

It also lacks smp_rmb()s.  One is more or less implicit in rdtsc, but
you need one to separate __pvclock_read_cycles's reads of src->foo from
pvclock_read_flags's read of src->version.

Minfei, would you like to take a look?

Paolo

Re: [PATCH] Cleanup __pvclock_read_cycles to remove useless variables

[Minfei Huang]

On 05/11/16 at 04:24P, Paolo Bonzini wrote:
> 
> 
> On 30/04/2016 23:57, Andy Lutomirski wrote:
> >> > Should we kill __pvclock_read_cycles in favor of vread_pvclock? It looks
> >> > doable at a quick scan...
> >> >
> > The in-kernel version might have to be a bit different because it
> > needs to handle the !stable case.  If !stable, it should just use the
> > current CPU's copy which means that, realistically, it should just
> > get_cpu and use the local copy unconditionally.  Other than that, it
> > could look a lot like the vread_pvclock variant.
> > 
> > But I agree, the current thing is incomprehensible.
> 
> It also lacks smp_rmb()s.  One is more or less implicit in rdtsc, but
> you need one to separate __pvclock_read_cycles's reads of src->foo from
> pvclock_read_flags's read of src->version.
> 
> Minfei, would you like to take a look?

Sure. I will take a look about this issue.

Thanks
Minfei

> 
> Paolo