RE: [PATCH 1/1] Clocksource: Move the Hyper-V clocksource driverout of staging

[KY Srinivasan]

> -----Original Message-----
> From: Thomas Gleixner [mailto:tglx@xxxxxxxxxxxxx]
> Sent: Tuesday, May 24, 2011 6:43 PM
> To: KY Srinivasan
> Cc: gregkh@xxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> devel@xxxxxxxxxxxxxxxxxxxxxx; johnstul@xxxxxxxxxx; hch@xxxxxxxxxxxxx; Hank
> Janssen; Haiyang Zhang
> Subject: Re: [PATCH 1/1] Clocksource: Move the Hyper-V clocksource driver out
> of staging
> 
> On Tue, 24 May 2011, K. Y. Srinivasan wrote:
> > --- a/arch/x86/Kconfig
> > +++ b/arch/x86/Kconfig
> > @@ -595,6 +595,9 @@ config X86_CYCLONE_TIMER
> >  	def_bool y
> >  	depends on X86_32_NON_STANDARD
> >
> > +config HYPERV_CLKSRC
> > +	def_bool y
> 
> Errm, why do we need another random config switch for every other
> feature of hyperv?
> 
> > +#include <linux/clocksource.h>
> > +#include <linux/time.h>
> 
> Why do you need time.h?

For the definition of NSEC_PER_SEC
 
> 
> > +#include <asm/hyperv.h>
> > +#include <asm/mshyperv.h>
> > +#include <asm/hypervisor.h>
> 
> Can we please have one sensible asm/hyperwtf.h include for all of this ?

Well,  hyperv.h has all the Hyper-V specific defines. When mshyperv.h was 
introduced, I tried very hard to merge it with hyperv.h and I was voted down.
With your help, maybe we can merge mshyperv.h and hyperv.h. Greg may remember
the arguments that kept these two related files separate.
 
> 
> > +
> > +static cycle_t read_hv_clock(struct clocksource *arg)
> > +{
> > +	cycle_t current_tick;
> > +	/*
> > +	 * Read the partition counter to get the current tick count. This count
> > +	 * is set to 0 when the partition is created and is incremented in
> > +	 * 100 nanosecond units.
> > +	 */
> 
> Please move that comment above the function. That's really irritating
> to read.
> 
> > +	rdmsrl(HV_X64_MSR_TIME_REF_COUNT, current_tick);
> > +	return current_tick;
> > +}
> > +
> > +static struct clocksource hyperv_cs = {
> > +	.name           = "hyperv_clocksource",
> > +	.rating         = 400, /* use this when running on Hyperv*/
> 
> That tail comment is pretty useless.
> 
> > +	.read           = read_hv_clock,
> > +	.mask           = CLOCKSOURCE_MASK(64),
> > +};
> > +
> > +static int __init init_hv_clocksource(void)
> > +{
> > +	unsigned long	hv_period = 100; /* 100 ns granularity clocksource */
> 
>   unsigned long period_ns = 100;
> 
> would make the horrible tail comment go away and make it obvious to
> the reader what the variable is for. Also please stop adding extra
> useless noise to local variables by adding hv_ or whatever the heck to
> them.
> 
> > +	u32		hv_freq;
> 
> Newline between declarations and code please.
> 
> > +	if ((x86_hyper != &x86_hyper_ms_hyperv) ||
> > +		!(ms_hyperv.features &
> HV_X64_MSR_TIME_REF_COUNT_AVAILABLE))
> > +		return -ENODEV;
> > +
> > +	hv_freq = NSEC_PER_SEC;
> > +	do_div(hv_freq, hv_period);
> 
> ROTFL. Do you really need runtime evaluation of 10^9/10^2 ?
> 
> #define HV_CLK_FREQ	(NSEC_PER_SEC/100)
> 
> would solve that problem with 5 lines less source code and a even
> larger reduction of binary size.
> 
> > +
> > +	printk(KERN_INFO "Registering Hyper-V clock source\n");
> 
> How is that interesting ?
> 
> > +	return clocksource_register_hz(&hyperv_cs, hv_freq);
> > +}
> 
> And if you remove that useless stuff then the ten remaining lines
> should go to arch/x86/ into a file which will contain more than those
> ten lines. It's pretty unlikely that anything else than MSHV will
> reuse that code.

I like the idea of merging this code with some other file under arch/x86/.
I could merge this code into mshyperv.c file that already has hyperv 
specific code. Who would take this patch if I were to merge this 
cleaned up Hyper-V clocksource code into mshyperv.c file under
arch/X86/kernel/cpu.

Thomas, thank you for your patience here.

Regards,

K. Y

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/