2008-10-01 06:13:57

by Grant Grundler

[permalink] [raw]
Subject: Re: [patch 2/2] PNP: don't check disabled PCI BARs for conflicts in quirk_system_pci_resources()

On Tue, Sep 30, 2008 at 12:51:07PM -0700, Linus Torvalds wrote:
....
> But I think we could add a separate notion of a dependancy point, and have
> a setup where we describe "initcall X needs to happen before point A" and
> "initcall Z needs to happen after point A".
>
> And then we can create a separate set of these dependency points, so that
> X and Y don't have to know about each other, they just have to have some
> knowledge about some common synchronization point - one that exists
> regardless of whether X or Y are even compiled in!

We already do this today. :)
Definitions are in include/linux/init.h.
Point A would be "early" ("run before initialing SMP")
The rest could use better definitions and AFAICT aren't that much better
than being named "Point B".

hth,
grant


2008-10-01 08:27:29

by Ingo Molnar

[permalink] [raw]
Subject: Re: [patch 2/2] PNP: don't check disabled PCI BARs for conflicts in quirk_system_pci_resources()


* Grant Grundler <[email protected]> wrote:

> On Tue, Sep 30, 2008 at 12:51:07PM -0700, Linus Torvalds wrote:
> ....
> > But I think we could add a separate notion of a dependancy point, and have
> > a setup where we describe "initcall X needs to happen before point A" and
> > "initcall Z needs to happen after point A".
> >
> > And then we can create a separate set of these dependency points, so that
> > X and Y don't have to know about each other, they just have to have some
> > knowledge about some common synchronization point - one that exists
> > regardless of whether X or Y are even compiled in!
>
> We already do this today. :)
> Definitions are in include/linux/init.h.
> Point A would be "early" ("run before initialing SMP")
> The rest could use better definitions and AFAICT aren't that much better
> than being named "Point B".

the structural problem with the current level-based initcall design is
that the current dependencies are implicit (not spelled out clearly
anywhere in the source code), and bugs in them are often fixed by
experimenting around (seeing whether it breaks), not by design.

Changing it is a ton of work, and the risks of touching that code might
eclipse any (often marginal) advantages a new scheme has. Today boot
code runs only once and it is one of the most under-tested pieces of
kernel code. Boot code's quality and robustness is at least 1 order of
magnitude worse than regular kernel code.

But to play the devil's advocate: users have so many problems with weird
races in boot code today _already_, wouldnt it be better to expose boot
code to more variations, to put it under environmental pressure that
ultimately improves its quality?

Init code is often reused during suspend/resume, so by introducing more
flexibility into initcalls maybe we create enough pressure to fix them
when it's far easier to fix them. (after bootup - fixing after-resume
bugs is much harder because often the console is turned off and no
significant BIOS code ran.)

Today moving an initcall to another level has unknown effects and
nothing warns about broken dependencies but a bootup crash (often only
triggering under a specific .config), or a non-working device or some
other regression.

That is rather fragile and i doubt anyone can argue the opposite.

The question of whether explicit dependencies are better is another
question and up to debate:

in the long run it is _IMHO_ more robust to express explicit
dependencies close to the init functions, in the source code:

initcall_depends_on(this_driver, memory_init);
initcall_depends_on(this_driver, io_resources_init);

than to rely on the implicit (and undocumented and often forgotten)
dependencies we have currently.

For example ordering an initcall to after PNP init would be trivial,
we'd add this to the init dependency list:

initcall_depends_on(this_driver, pnp_init);

With the current scheme we have to find some other integer 'level' and
hope that moving this initcall to that new level does not break other,
implicit dependencies.

And note that once we start doing explicit dependencies, we could
automate much of it: if a piece of .o uses a set of symbols that makes
it rather clear which subsystems it has to rely on. Say it uses
kmalloc() then it should depend on memory_init() done.

Ingo

2008-10-01 15:16:26

by Linus Torvalds

[permalink] [raw]
Subject: Re: [patch 2/2] PNP: don't check disabled PCI BARs for conflicts in quirk_system_pci_resources()



On Wed, 1 Oct 2008, Grant Grundler wrote:
> >
> > And then we can create a separate set of these dependency points, so that
> > X and Y don't have to know about each other, they just have to have some
> > knowledge about some common synchronization point - one that exists
> > regardless of whether X or Y are even compiled in!
>
> We already do this today. :)
> Definitions are in include/linux/init.h.

Absolutely.

The problem with the current <linux/init.h> isn't that it doesn't work -
it's worked pretty well for a long time - it's that we continually tend to
hit the limits of the few fixed points.

I would just extend on that notion a bit, and also make the markers a bit
more dynamic. Instead of having just 7-8 levels of initcalls, and a very
fixed naming that is a bit misleading, I'd like to extend it to maybe 50
levels, and the levels would be named by the subsystems and then just have
one single place that orders them.

The old 7 levels (plus the "pure" one) would still exist, so it wouldn't
need to be a flag-day event.

For example: why would you use "fs_initcall()" for the PnP init? The
reason is simple: it's not a filesystem init, but it's the one that comes
after the subsys init and before the actual low-leveld drivers. We used to
initialize the core filesystem data (VFS) at that stage (we still do,
although most of the actual filesystems are actually just done using the
default module-init much later), which explains the naming, but the
problem is that

(a) we want to order things at a finer granularity than that
(b) we want to have a better and more descriptive naming

but the basic notion of having a fixed ordering certainly isn't wrong (and
I already argued against any _dynamic_ one).

> Point A would be "early" ("run before initialing SMP")
> The rest could use better definitions and AFAICT aren't that much better
> than being named "Point B".

It would be *much* better to give them symbolic names (easy enough - we
just turn them into sections that are symbolic anyway), and then have a
list of ordering for those symbolic names.

So they sure as hell would be much better than being named "Point B". We
could get rid of

subsys_initcall(pci_init)

and instead do

initcall(pci_init, "pci");
..
initcall(pnp_init, "pnp");

and then just list the levels for the linker scripts in one place. So that
we wouldn't really have to worry about link ordering: if the link ordering
is wrong, we just add a new initcall level and insert it in the right
place, and then we can look at the ordering and see it explicitly in one
place instead of looking at the makefiles and checking the order we add
object files to the list in!

Linus

2008-10-01 16:21:36

by Yinghai Lu

[permalink] [raw]
Subject: Re: [patch 2/2] PNP: don't check disabled PCI BARs for conflicts in quirk_system_pci_resources()

On Wed, Oct 1, 2008 at 8:14 AM, Linus Torvalds
<[email protected]> wrote:
>
> and instead do
>
> initcall(pci_init, "pci");
> ..
> initcall(pnp_init, "pnp");
>
> and then just list the levels for the linker scripts in one place. So that
> we wouldn't really have to worry about link ordering: if the link ordering
> is wrong, we just add a new initcall level and insert it in the right
> place, and then we can look at the ordering and see it explicitly in one
> place instead of looking at the makefiles and checking the order we add
> object files to the list in!

don't need to think linking order.
could reorder them in the run time.

struct init_call_st {
int level;
char *name;
initcall_t call;
};

#define INIT_CALL(nameX, levelX, callX) \
static struct init_call_st __init_call_##nameX __initdata = \
{ .name = nameX,\
.level = levelX,\
.call = callX,\
}; \
static struct init_call_st *__init_call_ptr_##nameX __used \
__attribute__((__section__(".init_call.init"))) = \
&__init_call_##nameX

in vmlinux.lds.h
#define INIT_CALL_INIT(align)
\
. = ALIGN((align)); \
.init_call.init : AT(ADDR(.init_call.init) - LOAD_OFFSET) { \
VMLINUX_SYMBOL(__init_call_start) = .; \
*(.init_call.init) \
VMLINUX_SYMBOL(__init_call_end) = .; \
}

let arch vmlinux.lds.S to have
INIT_CALL_INIT(8)

and init/main.c

void __init do_init_calls(void)
{
struct init_call_st **daa;
char *ptr;

/* sort it?, prescan... */
for (daa = __init_call_start ; daa < __init_call_end; daa++) {
struct init_call_st *da = *daa;

...
}

for (daa = __init_call_start ; daa < __init_call_end; daa++) {
struct dyn_array *da = *daa;
...
da->call();
}


YH

2008-10-06 05:34:57

by Grant Grundler

[permalink] [raw]
Subject: Re: [patch 2/2] PNP: don't check disabled PCI BARs for conflicts in quirk_system_pci_resources()

On Wed, Oct 01, 2008 at 10:26:03AM +0200, Ingo Molnar wrote:
...
> > Definitions are in include/linux/init.h.
> > Point A would be "early" ("run before initialing SMP")
> > The rest could use better definitions and AFAICT aren't that much better
> > than being named "Point B".
>
> the structural problem with the current level-based initcall design is
> that the current dependencies are implicit (not spelled out clearly
> anywhere in the source code), and bugs in them are often fixed by
> experimenting around (seeing whether it breaks), not by design.

Ingo,
I totally agree with you and liked yours/Arjan's suggestion to make
the dependency explicit. Since linus pointed out explicit dependencies
would be a disaster to maintain, I don't know where to go next. I agree
with him issues will come up. But frobbing the current scheme by increasing
the number of "init points" from 8 to 50 (or more likely a 100 or 200)
feels like it's not making the dependencies explicit either.

We currently make the subsystem/driver dependencies explicit in the
Kconfig files. We hide the lack of a subsystem with null macros
(e.g. "#define foo() {}" ). I'm wondering if this would be one
step making your original suggestion palatable.

I'm also wondering if we should be using the dependency graph
that is effectively coded in Kconfig files to generate the
init calls somehow. Any university students looking for a
very cool and pratical project this year that will make you
famous and pay for your trip to a linux conference next year?

> Changing it is a ton of work, and the risks of touching that code might
> eclipse any (often marginal) advantages a new scheme has. Today boot
> code runs only once and it is one of the most under-tested pieces of
> kernel code. Boot code's quality and robustness is at least 1 order of
> magnitude worse than regular kernel code.

Agreed as long as the conversation is about subsystem initialization.

> But to play the devil's advocate: users have so many problems with weird
> races in boot code today _already_, wouldnt it be better to expose boot
> code to more variations, to put it under environmental pressure that
> ultimately improves its quality?

Yes. A DEBUG mechanism to record and dump the order when each init
call is started and completed might be one step to satisfy Linus' fear
of not being able to debug the problems.

> Init code is often reused during suspend/resume, so by introducing more
> flexibility into initcalls maybe we create enough pressure to fix them
> when it's far easier to fix them. (after bootup - fixing after-resume
> bugs is much harder because often the console is turned off and no
> significant BIOS code ran.)
>
> Today moving an initcall to another level has unknown effects and
> nothing warns about broken dependencies but a bootup crash (often only
> triggering under a specific .config), or a non-working device or some
> other regression.
>
> That is rather fragile and i doubt anyone can argue the opposite.
>
> The question of whether explicit dependencies are better is another
> question and up to debate:

I'm pretty comfortable that explicit dependencies are better.
The question is how to deal with the issues Linus raised and any
other issues folks find in a sane way.

>
> in the long run it is _IMHO_ more robust to express explicit
> dependencies close to the init functions, in the source code:
>
> initcall_depends_on(this_driver, memory_init);
> initcall_depends_on(this_driver, io_resources_init);

We just need the "memory_init" function to export a stub in
case it's not actually enabled in the .config file.

>
> than to rely on the implicit (and undocumented and often forgotten)
> dependencies we have currently.
>
> For example ordering an initcall to after PNP init would be trivial,
> we'd add this to the init dependency list:
>
> initcall_depends_on(this_driver, pnp_init);
>
> With the current scheme we have to find some other integer 'level' and
> hope that moving this initcall to that new level does not break other,
> implicit dependencies.
>
> And note that once we start doing explicit dependencies, we could
> automate much of it: if a piece of .o uses a set of symbols that makes
> it rather clear which subsystems it has to rely on. Say it uses
> kmalloc() then it should depend on memory_init() done.

*nod*. I think all of it has be automated. Where automation gets
the dependency info from will be the key to making this work.

hth,
grant

>
> Ingo
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html