| Message ID | 20180127162351.13858-1-pali.rohar@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
On Sat, Jan 27, 2018 at 05:23:51PM +0100, Pali Rohár wrote: > Measure only inlined asm code, not other functions to have as precise as > possible measured time. > > Signed-off-by: Pali Rohár <pali.rohar@gmail.com> > --- > drivers/hwmon/dell-smm-hwmon.c | 14 ++++++++++---- > 1 file changed, 10 insertions(+), 4 deletions(-) > > diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c > index bf3bb7e1adab..e001afd53f46 100644 > --- a/drivers/hwmon/dell-smm-hwmon.c > +++ b/drivers/hwmon/dell-smm-hwmon.c > @@ -147,14 +147,16 @@ static int i8k_smm_func(void *par) > int ebx = regs->ebx; > unsigned long duration; > ktime_t calltime, delta, rettime; > - > - calltime = ktime_get(); > #endif > > /* SMM requires CPU 0 */ > if (smp_processor_id() != 0) > return -EBUSY; > > +#ifdef DEBUG > + calltime = ktime_get(); > +#endif > + > #if defined(CONFIG_X86_64) > asm volatile("pushq %%rax\n\t" > "movl 0(%%rax),%%edx\n\t" > @@ -208,13 +210,17 @@ static int i8k_smm_func(void *par) > : "a"(regs) > : "%ebx", "%ecx", "%edx", "%esi", "%edi", "memory"); > #endif > - if (rc != 0 || (regs->eax & 0xffff) == 0xffff || regs->eax == eax) > - rc = -EINVAL; > > #ifdef DEBUG > rettime = ktime_get(); > delta = ktime_sub(rettime, calltime); > duration = ktime_to_ns(delta) >> 10; > +#endif > + > + if (rc != 0 || (regs->eax & 0xffff) == 0xffff || regs->eax == eax) > + rc = -EINVAL; > + > +#ifdef DEBUG FWIW, the error introduced by dividing nS by 1,024 instead of 1,000 is much worse than the improvements from moving the calls around. Using specific numbers, the current code reports 500 mS as 488,281 uS. So why bother ? I would have suggested to use ktime_us_delta(ktime_get(), calltime) instead to make the results more accurate. Sure, you get the offset from the additional divide operation, but at least that would be a constant and not a systematic error. I'll hold this patch off for a bit. Please confirm that you want it applied as-is. I applied the other patches from the series. Thanks, Guenter -- To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Saturday 27 January 2018 09:51:45 Guenter Roeck wrote: > On Sat, Jan 27, 2018 at 05:23:51PM +0100, Pali Rohár wrote: > > Measure only inlined asm code, not other functions to have as precise as > > possible measured time. > > > > Signed-off-by: Pali Rohár <pali.rohar@gmail.com> > > --- > > drivers/hwmon/dell-smm-hwmon.c | 14 ++++++++++---- > > 1 file changed, 10 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c > > index bf3bb7e1adab..e001afd53f46 100644 > > --- a/drivers/hwmon/dell-smm-hwmon.c > > +++ b/drivers/hwmon/dell-smm-hwmon.c > > @@ -147,14 +147,16 @@ static int i8k_smm_func(void *par) > > int ebx = regs->ebx; > > unsigned long duration; > > ktime_t calltime, delta, rettime; > > - > > - calltime = ktime_get(); > > #endif > > > > /* SMM requires CPU 0 */ > > if (smp_processor_id() != 0) > > return -EBUSY; > > > > +#ifdef DEBUG > > + calltime = ktime_get(); > > +#endif > > + > > #if defined(CONFIG_X86_64) > > asm volatile("pushq %%rax\n\t" > > "movl 0(%%rax),%%edx\n\t" > > @@ -208,13 +210,17 @@ static int i8k_smm_func(void *par) > > : "a"(regs) > > : "%ebx", "%ecx", "%edx", "%esi", "%edi", "memory"); > > #endif > > - if (rc != 0 || (regs->eax & 0xffff) == 0xffff || regs->eax == eax) > > - rc = -EINVAL; > > > > #ifdef DEBUG > > rettime = ktime_get(); > > delta = ktime_sub(rettime, calltime); > > duration = ktime_to_ns(delta) >> 10; > > +#endif > > + > > + if (rc != 0 || (regs->eax & 0xffff) == 0xffff || regs->eax == eax) > > + rc = -EINVAL; > > + > > +#ifdef DEBUG > > FWIW, the error introduced by dividing nS by 1,024 instead of 1,000 is > much worse than the improvements from moving the calls around. Using > specific numbers, the current code reports 500 mS as 488,281 uS. > So why bother ? Ah, I have not noticed this yet :-( > I would have suggested to use ktime_us_delta(ktime_get(), calltime) > instead to make the results more accurate. Sure, you get the offset from > the additional divide operation, but at least that would be a constant > and not a systematic error. > > I'll hold this patch off for a bit. Please confirm that you want it > applied as-is. I applied the other patches from the series. Ok, I will fix this patch and resend just new version of this one. Anyway, if 2/4 is targeting stable, then 3/4 should too. But now I see that I added Cc only to patch 2/4.
diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c index bf3bb7e1adab..e001afd53f46 100644 --- a/drivers/hwmon/dell-smm-hwmon.c +++ b/drivers/hwmon/dell-smm-hwmon.c @@ -147,14 +147,16 @@ static int i8k_smm_func(void *par) int ebx = regs->ebx; unsigned long duration; ktime_t calltime, delta, rettime; - - calltime = ktime_get(); #endif /* SMM requires CPU 0 */ if (smp_processor_id() != 0) return -EBUSY; +#ifdef DEBUG + calltime = ktime_get(); +#endif + #if defined(CONFIG_X86_64) asm volatile("pushq %%rax\n\t" "movl 0(%%rax),%%edx\n\t" @@ -208,13 +210,17 @@ static int i8k_smm_func(void *par) : "a"(regs) : "%ebx", "%ecx", "%edx", "%esi", "%edi", "memory"); #endif - if (rc != 0 || (regs->eax & 0xffff) == 0xffff || regs->eax == eax) - rc = -EINVAL; #ifdef DEBUG rettime = ktime_get(); delta = ktime_sub(rettime, calltime); duration = ktime_to_ns(delta) >> 10; +#endif + + if (rc != 0 || (regs->eax & 0xffff) == 0xffff || regs->eax == eax) + rc = -EINVAL; + +#ifdef DEBUG pr_debug("smm(0x%.4x 0x%.4x) = 0x%.4x (took %7lu usecs)\n", eax, ebx, (rc ? 0xffff : regs->eax & 0xffff), duration); #endif
Measure only inlined asm code, not other functions to have as precise as possible measured time. Signed-off-by: Pali Rohár <pali.rohar@gmail.com> --- drivers/hwmon/dell-smm-hwmon.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-)