Re: [4/4] hwmon: (dell-smm) Measure time duration of SMM call around inlined asm

[Pali RohÃr]

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

-- 
Pali RohÃr
pali.rohar@xxxxxxxxx