Date: Wed, 31 Jan 2007 10:34:14 +0100
From: Ingo Molnar <mingo@elte.hu>
To: Daniel Walker <dwalker@mvista.com>
Cc: akpm@osdl.org, linux-kernel@vger.kernel.org, johnstul@us.ibm.com,
       Thomas Gleixner <tglx@linutronix.de>
Subject: Re: [PATCH 07/23] clocksource: rating sorted list
Message-ID: <20070131093414.GA21335@elte.hu>
References: <20070131033710.420168478@mvista.com> <20070131033805.183466048@mvista.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20070131033805.183466048@mvista.com>
User-Agent: Mutt/1.4.2.2i
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 1718
Lines: 41


* Daniel Walker <dwalker@mvista.com> wrote:

> Converts the original plain list into a sorted list based on the clock 
> rating. Later in my tree this allows some of the variables to be 
> dropped since the highest rated clock is always at the front of the 
> list. This also does some other nice things like allow the sysfs files 
> to print the clocks in a more interesting order. It's forward looking.

Unfortunately this seems to be a step backwards to me. Please consider:

>  	if (clocksource_tsc.rating != 0 && check_tsc_unstable()) {
>  		clocksource_tsc.rating = 0;
> -		clocksource_reselect();
> +		clocksource_rating_change(&clocksource_tsc);
>  		change = 1;

this should be the following API:

	clocksource_rating_change(&clocksource_tsc, 0);

a rating change can occur due to other things as well, not only due to 
'tsc unstable' events. So keeping an API around that changes the rating 
(and propagates all related changes), makes more sense to me.

also, a pure function call is the most natural (and most flexible) API, 
for a centrally registered resource like a clock source driver. And a 
rating change should imply a reselect too, obviously. That way we 
replace 2 lines with 1 line - that's a real API improvement and a real 
cleanup patch.

and that's precisely what Thomas' patch in -mm that your queue undoes 
implements. (see: simplify-the-registration-of-clocksources.patch in 
-mm) Have you considered Thomas' change when you dropped it?

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