linux-kernel - Re: [PATCH 07/23] clocksource: rating sorted list

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20070131093414.GA21335@elte.hu>
Date:	Wed, 31 Jan 2007 10:34:14 +0100
From:	Ingo Molnar <mingo@...e.hu>
To:	Daniel Walker <dwalker@...sta.com>
Cc:	akpm@...l.org, linux-kernel@...r.kernel.org, johnstul@...ibm.com,
	Thomas Gleixner <tglx@...utronix.de>
Subject: Re: [PATCH 07/23] clocksource: rating sorted list


* Daniel Walker <dwalker@...sta.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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/