Cleanup __pvclock_read_cycles to remove useless variables

PATCH In review Minfei Huang 4 participants 8 messages Apr 25, 2016 – May 11, 2016 x86

lore

Share & tools

lore

Minfei Huang

author

2016-04-25 06:53 UTC #0 lore raw

PATCH 1/1 Cleanup __pvclock_read_cycles to remove useless variables

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

Remove the useless variables.

Signed-off-by Minfei Huang <[email protected]>

---
 arch/x86/include/asm/pvclock.h | 15 ++++-----------
 1 file changed, 4 insertions(+), 11 deletions(-)

arch/x86/include/asm/pvclock.h +4 ~~-11~~

index fdcc040..fb95dac 100644
@@ -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 {

Signature · 1 line

2.6.3

Minfei Huang

author

2016-04-30 10:12 UTC #1 lore raw

ping.

Any comment is appreciate.

Thanks
Minfei

Quoted 41 lines · Minfei Huang

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

Remove the useless variables.

Signed-off-by Minfei Huang <[email protected]>

---
 arch/x86/include/asm/pvclock.h | 15 ++++-----------
 1 file changed, 4 insertions(+), 11 deletions(-)

arch/x86/include/asm/pvclock.h +4 ~~-11~~

index fdcc040..fb95dac 100644
@@ -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 {

Signature · 1 line

2.6.3

Borislav Petkov

2016-04-30 13:24 UTC #2 lore raw

In reply to Minfei Huang

Quoted 16 lines · Minfei Huang

The value of cycles and flags can be assigned directly without intermediate variables.
Remove the useless variables.
Signed-off-by Minfei Huang <[email protected]>
---
 arch/x86/include/asm/pvclock.h | 15 ++++-----------
 1 file changed, 4 insertions(+), 11 deletions(-)
arch/x86/include/asm/pvclock.h +0 -0
index fdcc040..fb95dac 100644
@@ -80,19 +80,12 @@ static __always_inline
 unsigned __pvclock_read_cycles(const struct pvclock_vcpu_time_info *src,

arch/x86/include/asm/pvclock.h static __always_inline · line 85

Quoted 10 lines

arch/x86/include/asm/pvclock.h +1 -5

 			       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()...

Signature · 5 lines

Regards/Gruss,
    Boris.

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

Andy Lutomirski

2016-04-30 17:48 UTC #3 lore raw

Quoted 26 lines · Borislav Petkov

Quoted 25 lines · Minfei Huang

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

Remove the useless variables.

Signed-off-by Minfei Huang <[email protected]>

---
 arch/x86/include/asm/pvclock.h | 15 ++++-----------
 1 file changed, 4 insertions(+), 11 deletions(-)

arch/x86/include/asm/pvclock.h +1 -5

index fdcc040..fb95dac 100644
@@ -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;

Quoted 6 lines

arch/x86/include/asm/pvclock.h static __always_inline · line 85
Quoted 1 line
arch/x86/include/asm/pvclock.h +0 -0
      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.

Quoted 6 lines

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

Signature · 2 lines

Andy Lutomirski
AMA Capital Management, LLC

Borislav Petkov

2016-04-30 19:17 UTC #4 lore raw

Quoted 3 lines · Andy Lutomirski

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...

Signature · 5 lines

Regards/Gruss,
    Boris.

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

Andy Lutomirski

2016-04-30 21:58 UTC #5 lore raw

Quoted 9 lines · Borislav Petkov

Quoted 3 lines · Andy Lutomirski
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

Quoted 6 lines

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

Paolo Bonzini

2016-05-11 14:24 UTC #6 lore raw

Quoted 10 lines · Andy Lutomirski

Quoted 3 lines
Quoted 3 lines
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

Minfei Huang

author

2016-05-11 15:20 UTC #7 lore raw

Quoted 12 lines · Paolo Bonzini

Quoted 9 lines · Andy Lutomirski
Quoted 3 lines
Quoted 3 lines
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.

Quoted 7 lines

Quoted 1 line
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

Quoted 2 lines

Paolo