Re: + clocksource-driver-initialize-list-value.patch added to -mm tree

Re: + clocksource-driver-initialize-list-value.patch added to -mm tree

[Thomas Gleixner]

On Sat, 2007-03-31 at 22:23 -0700, akpm@linux-foundation.org wrote:
> The patch titled
>      clocksource: driver initialize list value
> has been added to the -mm tree.  Its filename is
>      clocksource-driver-initialize-list-value.patch
> 
> *** Remember to use Documentation/SubmitChecklist when testing your code ***
> 
> See http://www.zip.com.au/~akpm/linux/patches/stuff/added-to-mm.txt to find
> out what to do about this
> 
> ------------------------------------------------------
> Subject: clocksource: driver initialize list value
> From: Daniel Walker <dwalker@mvista.com>
> 
> Update drivers/clocksource/ with list initialization.

As others pointed out already, can we please have some usefull
description why this change is necessary ? i.e. that it is a preparatory
patch to simplify the clocksource registration logic.

	tglx

Re: + clocksource-driver-initialize-list-value.patch added to -mm tree

[Daniel Walker]

On Sun, 2007-04-01 at 10:43 +0200, Thomas Gleixner wrote:
> On Sat, 2007-03-31 at 22:23 -0700, akpm@linux-foundation.org wrote:
> > The patch titled
> >      clocksource: driver initialize list value
> > has been added to the -mm tree.  Its filename is
> >      clocksource-driver-initialize-list-value.patch
> > 
> > *** Remember to use Documentation/SubmitChecklist when testing your code ***
> > 
> > See http://www.zip.com.au/~akpm/linux/patches/stuff/added-to-mm.txt to find
> > out what to do about this
> > 
> > ------------------------------------------------------
> > Subject: clocksource: driver initialize list value
> > From: Daniel Walker <dwalker@mvista.com>
> > 
> > Update drivers/clocksource/ with list initialization.
> 
> As others pointed out already, can we please have some usefull
> description why this change is necessary ? i.e. that it is a preparatory
> patch to simplify the clocksource registration logic.
> 
> 	tglx

I was planning to do it after Andrew release the next -mm then use same
patch names he has and just submit to him.. I'm not sure what the
easiest method is, and I'd rather like to avoid re-submitting to LKML ..

Daniel

Re: + clocksource-driver-initialize-list-value.patch added to -mm tree

[Andrew Morton]

On Wed, 04 Apr 2007 09:38:32 -0700
Daniel Walker <dwalker@mvista.com> wrote:

> On Sun, 2007-04-01 at 10:43 +0200, Thomas Gleixner wrote:
> > On Sat, 2007-03-31 at 22:23 -0700, akpm@linux-foundation.org wrote:
> > > The patch titled
> > >      clocksource: driver initialize list value
> > > has been added to the -mm tree.  Its filename is
> > >      clocksource-driver-initialize-list-value.patch
> > > 
> > > *** Remember to use Documentation/SubmitChecklist when testing your code ***
> > > 
> > > See http://www.zip.com.au/~akpm/linux/patches/stuff/added-to-mm.txt to find
> > > out what to do about this
> > > 
> > > ------------------------------------------------------
> > > Subject: clocksource: driver initialize list value
> > > From: Daniel Walker <dwalker@mvista.com>
> > > 
> > > Update drivers/clocksource/ with list initialization.
> > 
> > As others pointed out already, can we please have some usefull
> > description why this change is necessary ? i.e. that it is a preparatory
> > patch to simplify the clocksource registration logic.
> > 
> > 	tglx
> 
> I was planning to do it after Andrew release the next -mm then use same
> patch names he has and just submit to him.. I'm not sure what the
> easiest method is, and I'd rather like to avoid re-submitting to LKML ..

Please just send the updated text.  Telling me the name of the patch
into which it is to be pasted is appreciated.

Re: + clocksource-driver-initialize-list-value.patch added to -mm tree

[Daniel Walker]

On Wed, 2007-04-04 at 12:58 -0700, Andrew Morton wrote:
> On Wed, 04 Apr 2007 09:38:32 -0700
> Daniel Walker <dwalker@mvista.com> wrote:
> 
> > On Sun, 2007-04-01 at 10:43 +0200, Thomas Gleixner wrote:
> > > On Sat, 2007-03-31 at 22:23 -0700, akpm@linux-foundation.org wrote:
> > > > The patch titled
> > > >      clocksource: driver initialize list value
> > > > has been added to the -mm tree.  Its filename is
> > > >      clocksource-driver-initialize-list-value.patch
> > > > 
> > > > *** Remember to use Documentation/SubmitChecklist when testing your code ***
> > > > 
> > > > See http://www.zip.com.au/~akpm/linux/patches/stuff/added-to-mm.txt to find
> > > > out what to do about this
> > > > 
> > > > ------------------------------------------------------
> > > > Subject: clocksource: driver initialize list value
> > > > From: Daniel Walker <dwalker@mvista.com>
> > > > 
> > > > Update drivers/clocksource/ with list initialization.
> > > 
> > > As others pointed out already, can we please have some usefull
> > > description why this change is necessary ? i.e. that it is a preparatory
> > > patch to simplify the clocksource registration logic.
> > > 
> > > 	tglx
> > 
> > I was planning to do it after Andrew release the next -mm then use same
> > patch names he has and just submit to him.. I'm not sure what the
> > easiest method is, and I'd rather like to avoid re-submitting to LKML ..
> 
> Please just send the updated text.  Telling me the name of the patch
> into which it is to be pasted is appreciated.

The following text,

---
The struct clocksource .list field is now required to be initialized
before calling clocksource_register().

This is a prerequisite for simplifying the clocksource
registration process.
---

Goes into ,

clocksource-arm-initialize-list-value.patch
clocksource-avr32-initialize-list-value.patch
clocksource-driver-initialize-list-value.patch
clocksource-i386-initialize-list-value.patch
clocksource-mips-initialize-list-value.patch
clocksource-parisc-initialize-list-value.patch
clocksource-s390-initialize-list-value.patch
clocksource-x86_64-initialize-list-value.patch

The patch names are from 2.6.21-rc5-mm4. Heiko, Thomas do either of you
have any comments on the text being added?

Daniel

Re: + clocksource-driver-initialize-list-value.patch added to -mm tree

[Ingo Molnar]

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

> The struct clocksource .list field is now required to be initialized 
> before calling clocksource_register().
> 
> This is a prerequisite for simplifying the clocksource registration 
> process.

why? This patch only pushes some unnecessary code into the clocksource 
drivers:

+       .list           = LIST_HEAD_INIT(clocksource_avr32.list),

NACK unless you can give an explanation of why this is unavoidable. A 
NULL initializer is just as good as an initialized list entry. (in fact 
it's slightly better because it's in the kernel's BSS)

	Ingo

Re: + clocksource-driver-initialize-list-value.patch added to -mm tree

[Daniel Walker]

On Wed, 2007-04-04 at 22:36 +0200, Ingo Molnar wrote:
> * Daniel Walker <dwalker@mvista.com> wrote:
> 
> > The struct clocksource .list field is now required to be initialized 
> > before calling clocksource_register().
> > 
> > This is a prerequisite for simplifying the clocksource registration 
> > process.
> 
> why? This patch only pushes some unnecessary code into the clocksource 
> drivers:
> 
> +       .list           = LIST_HEAD_INIT(clocksource_avr32.list),
> 
> NACK unless you can give an explanation of why this is unavoidable. A 
> NULL initializer is just as good as an initialized list entry. (in fact 
> it's slightly better because it's in the kernel's BSS)

This is only 1 of 9 patches . The 9th patch requires the .list value to
be initialized .. The description change above was suppose to make that
clearer .. By forcing the .list value to be initialized we can simplify
the clocksource registration .

Daniel

Re: + clocksource-driver-initialize-list-value.patch added to -mm tree

[Ingo Molnar]

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

> > > This is a prerequisite for simplifying the clocksource 
> > > registration process.
> > 
> > why? This patch only pushes some unnecessary code into the 
> > clocksource drivers:
> > 
> > +       .list           = LIST_HEAD_INIT(clocksource_avr32.list),
> > 
> > NACK unless you can give an explanation of why this is unavoidable. A 
> > NULL initializer is just as good as an initialized list entry. (in fact 
> > it's slightly better because it's in the kernel's BSS)
> 
> This is only 1 of 9 patches . The 9th patch requires the .list value 
> to be initialized .. The description change above was suppose to make 
> that clearer .. By forcing the .list value to be initialized we can 
> simplify the clocksource registration .

but why do you call that a simplification? Remove 5 lines of code from 
the generic code, by adding +1 line to every clocksource driver, 
totalling to like +20 lines at the moment?

	Ingo

Re: + clocksource-driver-initialize-list-value.patch added to -mm tree

[Daniel Walker]

On Wed, 2007-04-04 at 23:15 +0200, Ingo Molnar wrote:
> * Daniel Walker <dwalker@mvista.com> wrote:
> 
> > > > This is a prerequisite for simplifying the clocksource 
> > > > registration process.
> > > 
> > > why? This patch only pushes some unnecessary code into the 
> > > clocksource drivers:
> > > 
> > > +       .list           = LIST_HEAD_INIT(clocksource_avr32.list),
> > > 
> > > NACK unless you can give an explanation of why this is unavoidable. A 
> > > NULL initializer is just as good as an initialized list entry. (in fact 
> > > it's slightly better because it's in the kernel's BSS)
> > 
> > This is only 1 of 9 patches . The 9th patch requires the .list value 
> > to be initialized .. The description change above was suppose to make 
> > that clearer .. By forcing the .list value to be initialized we can 
> > simplify the clocksource registration .
> 
> but why do you call that a simplification? Remove 5 lines of code from 
> the generic code, by adding +1 line to every clocksource driver, 
> totalling to like +20 lines at the moment?

I guess I don't look at it in terms of lines .. Why do you think
reciting a line count diminishes the "simplification" claim? The 20+
lines that I added to each clocksource don't have a size or runtime
effect ..

Daniel

Re: + clocksource-driver-initialize-list-value.patch added to -mm tree

[Ingo Molnar]

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

> > but why do you call that a simplification? Remove 5 lines of code 
> > from the generic code, by adding +1 line to every clocksource 
> > driver, totalling to like +20 lines at the moment?
> 
> I guess I don't look at it in terms of lines .. Why do you think 
> reciting a line count diminishes the "simplification" claim? The 20+ 
> lines that I added to each clocksource don't have a size or runtime 
> effect ..

but they have a robustness and maintainability effect. Key is to keep 
drivers _simple_ and hard to mess up. If it's 5 extra lines of code to 
simplify a driver then we just do it. This is maintainance 101.

i cannot believe you are still arguing this. I explained this to you 
many weeks ago!

	Ingo

Re: + clocksource-driver-initialize-list-value.patch added to -mm tree

[Daniel Walker]

On Wed, 2007-04-04 at 23:34 +0200, Ingo Molnar wrote:
> * Daniel Walker <dwalker@mvista.com> wrote:
> 
> > > but why do you call that a simplification? Remove 5 lines of code 
> > > from the generic code, by adding +1 line to every clocksource 
> > > driver, totalling to like +20 lines at the moment?
> > 
> > I guess I don't look at it in terms of lines .. Why do you think 
> > reciting a line count diminishes the "simplification" claim? The 20+ 
> > lines that I added to each clocksource don't have a size or runtime 
> > effect ..
> 
> but they have a robustness and maintainability effect. Key is to keep 
> drivers _simple_ and hard to mess up. If it's 5 extra lines of code to 
> simplify a driver then we just do it. This is maintainance 101.
> 
> i cannot believe you are still arguing this. I explained this to you 
> many weeks ago!

I vaguely remember, but I don't think this creates a maintenance
issue .. It's not related to maintenance , it's an issue of creating a
new clocksource .. My perspective is that it has even less an effect
than the CLOCK_SOURCE_IS_CONTINUOUS field .. People actually have to
research that field, but list initialization is fairly clear.

The majority method for creating these clocksources is copy&paste, so
I'm not sure nano argument on this subject particularly relevant ..

Daniel

Re: + clocksource-driver-initialize-list-value.patch added to -mm tree

[Jeremy Fitzhardinge]

Daniel Walker wrote:
> I vaguely remember, but I don't think this creates a maintenance
> issue .. It's not related to maintenance , it's an issue of creating a
> new clocksource .. My perspective is that it has even less an effect
> than the CLOCK_SOURCE_IS_CONTINUOUS field .. People actually have to
> research that field, but list initialization is fairly clear.
>   

Yes, but its just make-work.  It has no bearing on what the clocksource
implementer needs to care about.  CLOCK_SOURCE_IS_CONTINUOUS is a
property of the clocksource they're implementing; it matters to them. 
But "list"?  It's just administration.

> The majority method for creating these clocksources is copy&paste, so
> I'm not sure nano argument on this subject particularly relevant ..

Great, so we end up with some random piece of clocksource internal
implementation detail cut-n-paste replicated all over the kernel.

    J

Re: + clocksource-driver-initialize-list-value.patch added to -mm tree

[Daniel Walker]

On Wed, 2007-04-04 at 16:48 -0700, Jeremy Fitzhardinge wrote:
> Daniel Walker wrote:
> > I vaguely remember, but I don't think this creates a maintenance
> > issue .. It's not related to maintenance , it's an issue of creating a
> > new clocksource .. My perspective is that it has even less an effect
> > than the CLOCK_SOURCE_IS_CONTINUOUS field .. People actually have to
> > research that field, but list initialization is fairly clear.
> >   
> 
> Yes, but its just make-work.  It has no bearing on what the clocksource
> implementer needs to care about.  CLOCK_SOURCE_IS_CONTINUOUS is a
> property of the clocksource they're implementing; it matters to them. 
> But "list"?  It's just administration.

Setting CLOCK_SOURCE_IS_CONTINUOUS is largely administration , do you
know what that flag means?

> > The majority method for creating these clocksources is copy&paste, so
> > I'm not sure nano argument on this subject particularly relevant ..
> 
> Great, so we end up with some random piece of clocksource internal
> implementation detail cut-n-paste replicated all over the kernel.

list values and list initialization are hardly internal details , they
are commonly used all over the kernel.

Daniel

Re: + clocksource-driver-initialize-list-value.patch added to -mm tree

[Jeremy Fitzhardinge]

Daniel Walker wrote:
> Setting CLOCK_SOURCE_IS_CONTINUOUS is largely administration , do you
> know what that flag means?
>   

Sure, but at least it has something to do with clocks and time.

> list values and list initialization are hardly internal details , they
> are commonly used all over the kernel.
>   

The fact that a list is used to string together clocksource structures
is an internal detail of the clock subsystem.  It's annoying that there
has to be a list head in the clocksource structure, but at least as a
clocksource implementer I can ignore it, and if it ever gets changed to
something else I can keep ignoring it.  Your change makes it something
that gets pointlessly replicated all over the kernel.

The real point is that it seems this change just adds work and cruft,
but for no benefit.  Your comment is that it "simplifies the
registration process".  Why does it need to be simpler?  How is it
simpler?  From a clocksource implementers perspective, the registration
is already pretty simple; how does it get simpler?  It looks less simple
to me, because now there's another failure-mode (a BUG if I forget to
initialize the list).

    J

Re: + clocksource-driver-initialize-list-value.patch added to -mm tree

[Daniel Walker]

On Wed, 2007-04-04 at 17:20 -0700, Jeremy Fitzhardinge wrote:
> Daniel Walker wrote:
> > Setting CLOCK_SOURCE_IS_CONTINUOUS is largely administration , do you
> > know what that flag means?
> >   
> 
> Sure, but at least it has something to do with clocks and time.
> 
> > list values and list initialization are hardly internal details , they
> > are commonly used all over the kernel.
> >   
> 
> The fact that a list is used to string together clocksource structures
> is an internal detail of the clock subsystem.  It's annoying that there
> has to be a list head in the clocksource structure, but at least as a
> clocksource implementer I can ignore it, and if it ever gets changed to
> something else I can keep ignoring it.  Your change makes it something
> that gets pointlessly replicated all over the kernel.

It's already been discussed in the past, but the clocksource structure
is already very simple.. Adding a list initializer doesn't change
anything..

Daniel

Re: + clocksource-driver-initialize-list-value.patch added to -mm tree

[Thomas Gleixner]

On Wed, 2007-04-04 at 17:34 -0700, Daniel Walker wrote:
> > The fact that a list is used to string together clocksource structures
> > is an internal detail of the clock subsystem.  It's annoying that there
> > has to be a list head in the clocksource structure, but at least as a
> > clocksource implementer I can ignore it, and if it ever gets changed to
> > something else I can keep ignoring it.  Your change makes it something
> > that gets pointlessly replicated all over the kernel.
> 
> It's already been discussed in the past, but the clocksource structure
> is already very simple.. Adding a list initializer doesn't change
> anything..

And it does not gain anything. 

So what's the gain of this ? Nine useless entries in the changelog.

	tglx

Re: + clocksource-driver-initialize-list-value.patch added to -mm tree

[Ingo Molnar]

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

> [...] Adding a list initializer doesn't change anything..

Daniel, you are still in denial. OF COURSE it changes something: it adds 
a line of code to a driver, where that line was not needed before. 
That's against the fundamental task of a driver model: TO KEEP THINGS 
SIMPLE. Yes, this concept includes single-line changes as well.

yes, a single line might not sound much, but it's exactly one line more 
than necessary.

	Ingo

Re: + clocksource-driver-initialize-list-value.patch added to -mm tree

[Daniel Walker]

On Thu, 2007-04-05 at 09:51 +0200, Ingo Molnar wrote:
> * Daniel Walker <dwalker@mvista.com> wrote:
> 
> > [...] Adding a list initializer doesn't change anything..
> 
> Daniel, you are still in denial. OF COURSE it changes something: it adds 
> a line of code to a driver, where that line was not needed before. 
> That's against the fundamental task of a driver model: TO KEEP THINGS 
> SIMPLE. Yes, this concept includes single-line changes as well.

If that's a fundemental rule then we need to expand our horizons to
something a little more flexible .. I understand what your saying, I do,
but it doesn't apply to this change or clocksources . The reason I think
that is because the potential is ~100 drivers or less, once you reach a
critical mass of drivers there's few if any to be added .. So from my
perspective there no reason to overly simplify the structure..

Daniel

Re: + clocksource-driver-initialize-list-value.patch added to -mm tree

[Andrew Morton]

On Wed, 4 Apr 2007 22:36:47 +0200
Ingo Molnar <mingo@elte.hu> wrote:

> 
> * Daniel Walker <dwalker@mvista.com> wrote:
> 
> > The struct clocksource .list field is now required to be initialized 
> > before calling clocksource_register().
> > 
> > This is a prerequisite for simplifying the clocksource registration 
> > process.
> 
> why?

It's all enablement for
ftp://ftp.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.21-rc5/2.6.21-rc5-mm4/broken-out/clocksource-refactor-duplicate-registration-checking.patch,
on which I have no opinion.


> This patch only pushes some unnecessary code into the clocksource 
> drivers:
> 
> +       .list           = LIST_HEAD_INIT(clocksource_avr32.list),
> 
> NACK unless you can give an explanation of why this is unavoidable. A 
> NULL initializer is just as good as an initialized list entry. (in fact 
> it's slightly better because it's in the kernel's BSS)
> 

No, these structures are already in .data.

        .mask   = CLOCKSOURCE_MASK(32),
        .mult   = 0,
        .shift  = 20,
+       .list   = LIST_HEAD_INIT(clocksource_pit.list),
 };

Re: + clocksource-driver-initialize-list-value.patch added to -mm tree

[Thomas Gleixner]

On Wed, 2007-04-04 at 13:48 -0700, Andrew Morton wrote:
> On Wed, 4 Apr 2007 22:36:47 +0200
> Ingo Molnar <mingo@elte.hu> wrote:
> 
> > 
> > * Daniel Walker <dwalker@mvista.com> wrote:
> > 
> > > The struct clocksource .list field is now required to be initialized 
> > > before calling clocksource_register().
> > > 
> > > This is a prerequisite for simplifying the clocksource registration 
> > > process.
> > 
> > why?
> 
> It's all enablement for
> ftp://ftp.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.21-rc5/2.6.21-rc5-mm4/broken-out/clocksource-refactor-duplicate-registration-checking.patch,
> on which I have no opinion.

I have. Just noticed that my original comment got stuck in mail jam:

> Refactors the duplicate registration checking. This makes it based on the
> clocksource structure list state. I was able to drop some if statements
> making the registration code path slightly smaller and faster, and remove
> some looping which was endemic of the first version of this check.

What does the slightly smaller and faster buy us ? 

Which looping has been removed ? If you want to optimize this code then
you need to break out of the loop, once the rating check has found the
place to stick it in.

But I still do not see any advantage in this. Registration is a one time
operation and not near any fast path. 

	tglx