Re: [PATCH/RFC] sched: Remove SYSTEM_RUNNING checks from cond_resched*()

Re: [PATCH/RFC] sched: Remove SYSTEM_RUNNING checks from cond_resched*()

[Andrew Morton]

(belatedly cc'ing netdev)

Original diagnosis:

: Using early netconsole and gianfar driver this error pops up:
: 
:   netconsole: timeout waiting for carrier
: 
: It appears that net/core/netpoll.c:netpoll_setup() is using
: cond_resched() in a loop waiting for a carrier.
: 
: The thing is that cond_resched() is a no-op when system_state !=
: SYSTEM_RUNNING, and so drivers/net/phy/phy.c's state_queue is never
: scheduled, therefore link detection doesn't work

> On Thu, 9 Jul 2009 01:33:31 +0400 Anton Vorontsov <avorontsov@ru.mvista.com> wrote:
> On Wed, Jul 08, 2009 at 02:10:24PM -0700, Andrew Morton wrote:
> > > On Wed, 8 Jul 2009 09:12:30 -0700 (PDT) Linus Torvalds <torvalds@linux-foundation.org> wrote:
> > > That said, I do agree that maybe SYSTEM_RUNNING isn't the right check. 
> > > Testing that the scheduler is initialized may be the more correct one. I 
> > > think the SYSTEM_RUNNING one just comes from that being used for other 
> > > debug issues.
> > 
> > Agreed.  system_state is too general.
> > 
> > If we specifically want to know whether it is safe to call schedule() then
> > let's create a global boolean it_is_safe_to_call_schedule and test that,
> > rather than testing something which indirectly and unreliably implies "it
> > is safe to call schedule".  If that boolean already exists then no-brainer.
> > 
> > All that being said, I wonder if the netconsole code should be using
> > msleep(1) instead.  Spinning on cond_resched() is a bit rude.  But one
> > would have to verify that it is safe to call schedule() at this time, and
> > for the netconsole caller, this is dubious.
> 
> What do you mean by "verify that it is safe"? If it works,
> can I assume that it's safe? ;-) It works, fwiw.
> 

netconsole is supposed to be available as early as possible in boot for
obvious reasons.  I'd say there's a decent risk now and in the future that
netconsole will be initialised prior to the scheduler being available.

In fact, if "netconsole: timeout waiting for carrier" newly added to
netpoll_setup() a depedency on the scheduler being available then perhaps
that was an incorrect change.

[PATCH] netpoll: Fix carrier detection for drivers that are using phylib

[Anton Vorontsov]

Using early netconsole and gianfar driver this error pops up:

  netconsole: timeout waiting for carrier

It appears that net/core/netpoll.c:netpoll_setup() is using
cond_resched() in a loop waiting for a carrier.

The thing is that cond_resched() is a no-op when system_state !=
SYSTEM_RUNNING, and so drivers/net/phy/phy.c's state_queue is never
scheduled, therefore link detection doesn't work.

I belive that the main problem is in cond_resched()[1], but despite
how the cond_resched() story ends, it might be a good idea to call
msleep(1) instead of cond_resched(), as suggested by Andrew Morton.

[1] http://lkml.org/lkml/2009/7/7/463

Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
---

On Wed, Jul 08, 2009 at 02:47:44PM -0700, Andrew Morton wrote:
> (belatedly cc'ing netdev)
> 
> Original diagnosis:
> 
> : Using early netconsole and gianfar driver this error pops up:
> : 
> :   netconsole: timeout waiting for carrier
> : 
> : It appears that net/core/netpoll.c:netpoll_setup() is using
> : cond_resched() in a loop waiting for a carrier.
> : 
> : The thing is that cond_resched() is a no-op when system_state !=
> : SYSTEM_RUNNING, and so drivers/net/phy/phy.c's state_queue is never
> : scheduled, therefore link detection doesn't work
> 
> > On Thu, 9 Jul 2009 01:33:31 +0400 Anton Vorontsov <avorontsov@ru.mvista.com> wrote:
> > On Wed, Jul 08, 2009 at 02:10:24PM -0700, Andrew Morton wrote:
> > > > On Wed, 8 Jul 2009 09:12:30 -0700 (PDT) Linus Torvalds <torvalds@linux-foundation.org> wrote:
> > > > That said, I do agree that maybe SYSTEM_RUNNING isn't the right check. 
> > > > Testing that the scheduler is initialized may be the more correct one. I 
> > > > think the SYSTEM_RUNNING one just comes from that being used for other 
> > > > debug issues.
> > > 
> > > Agreed.  system_state is too general.
> > > 
> > > If we specifically want to know whether it is safe to call schedule() then
> > > let's create a global boolean it_is_safe_to_call_schedule and test that,
> > > rather than testing something which indirectly and unreliably implies "it
> > > is safe to call schedule".  If that boolean already exists then no-brainer.
> > > 
> > > All that being said, I wonder if the netconsole code should be using
> > > msleep(1) instead.  Spinning on cond_resched() is a bit rude.  But one
> > > would have to verify that it is safe to call schedule() at this time, and
> > > for the netconsole caller, this is dubious.
> > 
> > What do you mean by "verify that it is safe"? If it works,
> > can I assume that it's safe? ;-) It works, fwiw.
> > 
> 
> netconsole is supposed to be available as early as possible in boot for
> obvious reasons.  I'd say there's a decent risk now and in the future that
> netconsole will be initialised prior to the scheduler being available.
> 
> In fact, if "netconsole: timeout waiting for carrier" newly added to
> netpoll_setup() a depedency on the scheduler being available then perhaps
> that was an incorrect change.

'git blame' says that carrier detection code didn't change since 2.6.12
(where git history starts), PHYLIB is using workqueue since its
submission (2.6.13). And SYSTEM_RUNNING check was added in 2.6.16.
So it's not a new dependency.

The netpoll code is using msleep() just a few lines below cond_resched(),
so we won't make things worse. ;-)

Thanks!

 net/core/netpoll.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/net/core/netpoll.c b/net/core/netpoll.c
index 9675f31..df30feb 100644
--- a/net/core/netpoll.c
+++ b/net/core/netpoll.c
@@ -740,7 +740,7 @@ int netpoll_setup(struct netpoll *np)
 				       np->name);
 				break;
 			}
-			cond_resched();
+			msleep(1);
 		}
 
 		/* If carrier appears to come up instantly, we don't
-- 
1.6.3.3

Re: [PATCH] netpoll: Fix carrier detection for drivers that are using phylib

[Linus Torvalds]

On Thu, 9 Jul 2009, Anton Vorontsov wrote:
> 
> The netpoll code is using msleep() just a few lines below cond_resched(),
> so we won't make things worse. ;-)

Yeah. That function is definitely sleeping. It does things like 
kmalloc(GFP_KERNEL), rtnl_lock() and synchronize_rcu() etc too, so an 
added msleep() is the least of our problems.

Afaik, it's called from a bog-standard "module_init()", which happens late 
enough that everything works.

In fact, I wonder if we should set SYSTEM_RUNNING much earlier - _before_ 
doing the whole "do_initcalls()". By then we've set up all the core stuff 
and enabled interrupts, so we really _are_ running. We just don't 
necessarily have drivers, filesystems etc loaded yet. But anything that 
happens late enough to be an initcall should be largely considered to 
be during "normal code".

(The "early_initcall" cases are special - those really do happen pretty 
early).

So ACK on Anton's patch, but I wonder if we _also_ should do the 
following?

Looking at the people looking at SYSTEM_RUNNING, I do note some odd cases. 
Why the heck does kernel/perf_counter.c do it, for example?

		Linus

---
 init/main.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/init/main.c b/init/main.c
index 2c5ade7..f10d9cd 100644
--- a/init/main.c
+++ b/init/main.c
@@ -788,6 +788,7 @@ static void __init do_initcalls(void)
 {
 	initcall_t *call;
 
+	system_state = SYSTEM_RUNNING;
 	for (call = __early_initcall_end; call < __initcall_end; call++)
 		do_one_initcall(*call);
 
@@ -839,7 +840,6 @@ static noinline int init_post(void)
 	free_initmem();
 	unlock_kernel();
 	mark_rodata_ro();
-	system_state = SYSTEM_RUNNING;
 	numa_default_policy();
 
 	if (sys_open((const char __user *) "/dev/console", O_RDWR, 0) < 0)

Re: [PATCH] netpoll: Fix carrier detection for drivers that are using phylib

[David Miller]

From: Linus Torvalds <torvalds@linux-foundation.org>
Date: Wed, 8 Jul 2009 17:01:14 -0700 (PDT)

> So ACK on Anton's patch,

I'll integrate it into my tree, thanks.

> but I wonder if we _also_ should do the following?

I think we should set SYSTEM_RUNNING earlier, makes sense
to me.

Re: [PATCH] netpoll: Fix carrier detection for drivers that are using phylib

[Peter Zijlstra]

On Wed, 2009-07-08 at 17:01 -0700, Linus Torvalds wrote:
> Looking at the people looking at SYSTEM_RUNNING, I do note some odd cases. 
> Why the heck does kernel/perf_counter.c do it, for example?

Ah, those are the swcounter and other probe entry points. I've had
several cases where we called into the perf counter code from those
points before it was initialized, getting in kernel segfaults due to
dereferencing uninitialized data etc..

I could keep a variable that tracked the perf_counter_init() state, and
use that instead if you prefer?

Re: [PATCH] netpoll: Fix carrier detection for drivers that are using phylib

[Matt Mackall]

On Thu, 2009-07-09 at 09:56 +0200, Peter Zijlstra wrote:
> On Wed, 2009-07-08 at 17:01 -0700, Linus Torvalds wrote:
> > Looking at the people looking at SYSTEM_RUNNING, I do note some odd cases. 
> > Why the heck does kernel/perf_counter.c do it, for example?
> 
> Ah, those are the swcounter and other probe entry points. I've had
> several cases where we called into the perf counter code from those
> points before it was initialized, getting in kernel segfaults due to
> dereferencing uninitialized data etc..
> 
> I could keep a variable that tracked the perf_counter_init() state, and
> use that instead if you prefer?

Looks like that'd be more accurate. Linus's proposed patch might break
your current assumptions.

-- 
http://selenic.com : development and support for Mercurial and Linux

Re: [PATCH] netpoll: Fix carrier detection for drivers that are using phylib

[Matt Mackall]

On Wed, 2009-07-08 at 17:01 -0700, Linus Torvalds wrote:
> 
> On Thu, 9 Jul 2009, Anton Vorontsov wrote:
> > 
> > The netpoll code is using msleep() just a few lines below cond_resched(),
> > so we won't make things worse. ;-)
> 
> Yeah. That function is definitely sleeping. It does things like 
> kmalloc(GFP_KERNEL), rtnl_lock() and synchronize_rcu() etc too, so an 
> added msleep() is the least of our problems.
> 
> Afaik, it's called from a bog-standard "module_init()", which happens late 
> enough that everything works.
> 
> In fact, I wonder if we should set SYSTEM_RUNNING much earlier - _before_ 
> doing the whole "do_initcalls()".

Well there are two ways of consistently defining SYSTEM_RUNNING:

a) define it with reference to the well-understood notion of booting vs
running and don't switch it until handing off to init

b) define it with reference to its usage by an arbitrary user like
cond_resched()

In the latter case, we obviously need to move it to the earliest point
that scheduling is possible. But there are a number of things like 

http://lxr.linux.no/linux+v2.6.30/kernel/printk.c#L228

that assume the definition is actually (a). We're currently within a
couple lines of a strict definition of (a) already, so I actually think
cond_resched() is just wrong (and we already know it broke a
previously-working user). It should perhaps be using another private
flag that gets set as soon as scheduling is up and running.

But I'd actually go further and say that it's unfortunate to be checking
extra flags in such an important inline, especially since the check is
false for all but the first couple seconds of run time. Seems like we
could avoid adding an extra check by artificially elevating the preempt
count in early boot (or at compile time) then dropping it when
scheduling becomes available.

-- 
http://selenic.com : development and support for Mercurial and Linux

Re: [PATCH] netpoll: Fix carrier detection for drivers that are using phylib

[Peter Zijlstra]

On Thu, 2009-07-09 at 08:26 -0500, Matt Mackall wrote: 
> On Wed, 2009-07-08 at 17:01 -0700, Linus Torvalds wrote:
> > 
> > On Thu, 9 Jul 2009, Anton Vorontsov wrote:
> > > 
> > > The netpoll code is using msleep() just a few lines below cond_resched(),
> > > so we won't make things worse. ;-)
> > 
> > Yeah. That function is definitely sleeping. It does things like 
> > kmalloc(GFP_KERNEL), rtnl_lock() and synchronize_rcu() etc too, so an 
> > added msleep() is the least of our problems.
> > 
> > Afaik, it's called from a bog-standard "module_init()", which happens late 
> > enough that everything works.
> > 
> > In fact, I wonder if we should set SYSTEM_RUNNING much earlier - _before_ 
> > doing the whole "do_initcalls()".
> 
> Well there are two ways of consistently defining SYSTEM_RUNNING:
> 
> a) define it with reference to the well-understood notion of booting vs
> running and don't switch it until handing off to init

This makes the most sense IMHO.

> b) define it with reference to its usage by an arbitrary user like
> cond_resched()
> 
> In the latter case, we obviously need to move it to the earliest point
> that scheduling is possible. But there are a number of things like 
> 
> http://lxr.linux.no/linux+v2.6.30/kernel/printk.c#L228
> 
> that assume the definition is actually (a). We're currently within a
> couple lines of a strict definition of (a) already, so I actually think
> cond_resched() is just wrong (and we already know it broke a
> previously-working user). It should perhaps be using another private
> flag that gets set as soon as scheduling is up and running.

Right as mentioned before in this thread, we grew scheduler_running a
while back which could be used for this.

> But I'd actually go further and say that it's unfortunate to be checking
> extra flags in such an important inline, especially since the check is
> false for all but the first couple seconds of run time. Seems like we
> could avoid adding an extra check by artificially elevating the preempt
> count in early boot (or at compile time) then dropping it when
> scheduling becomes available.

Calling cond_resched() and co when !preemptable is an error so this
wouldn't actually work.

Re: [PATCH] netpoll: Fix carrier detection for drivers that are using phylib

[Matt Mackall]

On Thu, 2009-07-09 at 15:46 +0200, Peter Zijlstra wrote:
> On Thu, 2009-07-09 at 08:26 -0500, Matt Mackall wrote: 
> > On Wed, 2009-07-08 at 17:01 -0700, Linus Torvalds wrote:
> > > 
> > > On Thu, 9 Jul 2009, Anton Vorontsov wrote:
> > > > 
> > > > The netpoll code is using msleep() just a few lines below cond_resched(),
> > > > so we won't make things worse. ;-)
> > > 
> > > Yeah. That function is definitely sleeping. It does things like 
> > > kmalloc(GFP_KERNEL), rtnl_lock() and synchronize_rcu() etc too, so an 
> > > added msleep() is the least of our problems.
> > > 
> > > Afaik, it's called from a bog-standard "module_init()", which happens late 
> > > enough that everything works.
> > > 
> > > In fact, I wonder if we should set SYSTEM_RUNNING much earlier - _before_ 
> > > doing the whole "do_initcalls()".
> > 
> > Well there are two ways of consistently defining SYSTEM_RUNNING:
> > 
> > a) define it with reference to the well-understood notion of booting vs
> > running and don't switch it until handing off to init
> 
> This makes the most sense IMHO.
> 
> > b) define it with reference to its usage by an arbitrary user like
> > cond_resched()
> > 
> > In the latter case, we obviously need to move it to the earliest point
> > that scheduling is possible. But there are a number of things like 
> > 
> > http://lxr.linux.no/linux+v2.6.30/kernel/printk.c#L228
> > 
> > that assume the definition is actually (a). We're currently within a
> > couple lines of a strict definition of (a) already, so I actually think
> > cond_resched() is just wrong (and we already know it broke a
> > previously-working user). It should perhaps be using another private
> > flag that gets set as soon as scheduling is up and running.
> 
> Right as mentioned before in this thread, we grew scheduler_running a
> while back which could be used for this.
> 
> > But I'd actually go further and say that it's unfortunate to be checking
> > extra flags in such an important inline, especially since the check is
> > false for all but the first couple seconds of run time. Seems like we
> > could avoid adding an extra check by artificially elevating the preempt
> > count in early boot (or at compile time) then dropping it when
> > scheduling becomes available.
> 
> Calling cond_resched() and co when !preemptable is an error so this
> wouldn't actually work.

Sorry if I was unclear. I'm suggesting setting the count so the existing
PREEMPT_ACTIVE test here fires:

int __sched _cond_resched(void)
{
        if (need_resched() && !(preempt_count() & PREEMPT_ACTIVE) &&
                                        system_state == SYSTEM_RUNNING) {
                __cond_resched();
                return 1;
        }
        return 0;
}

That should be kosher.

-- 
http://selenic.com : development and support for Mercurial and Linux

Re: [PATCH] netpoll: Fix carrier detection for drivers that are using phylib

[Peter Zijlstra]

On Thu, 2009-07-09 at 09:18 -0500, Matt Mackall wrote:
> 
> Sorry if I was unclear. I'm suggesting setting the count so the existing
> PREEMPT_ACTIVE test here fires:
> 
> int __sched _cond_resched(void)
> {
>         if (need_resched() && !(preempt_count() & PREEMPT_ACTIVE) &&
>                                         system_state == SYSTEM_RUNNING) {
>                 __cond_resched();
>                 return 1;
>         }
>         return 0;
> }

Right, /me read preempt and thought a simple preempt inc but didn't read
the code. Shame on me.

So something like (utterly untested and such)

---

 arch/x86/include/asm/thread_info.h |    2 +-
 kernel/sched.c                     |    4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/x86/include/asm/thread_info.h b/arch/x86/include/asm/thread_info.h
index b078352..7b5dbce 100644
--- a/arch/x86/include/asm/thread_info.h
+++ b/arch/x86/include/asm/thread_info.h
@@ -49,7 +49,7 @@ struct thread_info {
 	.exec_domain	= &default_exec_domain,	\
 	.flags		= 0,			\
 	.cpu		= 0,			\
-	.preempt_count	= 1,			\
+	.preempt_count	= 1 + PREEMPT_ACTIVE,	\
 	.addr_limit	= KERNEL_DS,		\
 	.restart_block = {			\
 		.fn = do_no_restart_syscall,	\
diff --git a/kernel/sched.c b/kernel/sched.c
index fd3ac58..8e81162 100644
--- a/kernel/sched.c
+++ b/kernel/sched.c
@@ -6599,8 +6599,7 @@ static void __cond_resched(void)
 
 int __sched _cond_resched(void)
 {
-	if (need_resched() && !(preempt_count() & PREEMPT_ACTIVE) &&
-					system_state == SYSTEM_RUNNING) {
+	if (need_resched() && !(preempt_count() & PREEMPT_ACTIVE)) {
 		__cond_resched();
 		return 1;
 	}
@@ -9422,6 +9421,7 @@ void __init sched_init(void)
 	perf_counter_init();
 
 	scheduler_running = 1;
+	sub_preempt_count(PREEMPT_ACTIVE);
 }
 
 #ifdef CONFIG_DEBUG_SPINLOCK_SLEEP

Re: [PATCH] netpoll: Fix carrier detection for drivers that are using phylib

[Matt Mackall]

On Thu, 2009-07-09 at 16:31 +0200, Peter Zijlstra wrote:
> On Thu, 2009-07-09 at 09:18 -0500, Matt Mackall wrote:
> > 
> > Sorry if I was unclear. I'm suggesting setting the count so the existing
> > PREEMPT_ACTIVE test here fires:
> > 
> > int __sched _cond_resched(void)
> > {
> >         if (need_resched() && !(preempt_count() & PREEMPT_ACTIVE) &&
> >                                         system_state == SYSTEM_RUNNING) {
> >                 __cond_resched();
> >                 return 1;
> >         }
> >         return 0;
> > }
> 
> Right, /me read preempt and thought a simple preempt inc but didn't read
> the code. Shame on me.
> 
> So something like (utterly untested and such)

Yeah, that's what I had in mind. Probably throw in a define:

/* for disabling scheduling in early boot */
#define PREEMPT_EARLY (1 + PREEMPT_ACTIVE)

and slap a comment on the sub_preempt_count().

Does anything actually use scheduler_running yet? Perhaps my tree is
old.

Also, might_sleep's use of system_state probably bears revisiting.

-- 
http://selenic.com : development and support for Mercurial and Linux

Re: [PATCH] netpoll: Fix carrier detection for drivers that are using phylib

[Peter Zijlstra]

On Thu, 2009-07-09 at 09:43 -0500, Matt Mackall wrote:

> Yeah, that's what I had in mind. Probably throw in a define:
> 
> /* for disabling scheduling in early boot */
> #define PREEMPT_EARLY (1 + PREEMPT_ACTIVE)
> 
> and slap a comment on the sub_preempt_count().

Right, and visit all the other arch init code ;-)

I'll wait to see if Ingo has anything to say about it and then complete
this thing.

> Does anything actually use scheduler_running yet? Perhaps my tree is
> old.

# git grep scheduler_running
kernel/sched.c:static __read_mostly int scheduler_running;
kernel/sched.c: scheduler_running = 1;
kernel/sched_rt.c:      if (unlikely(!scheduler_running))
kernel/sched_rt.c:      if (unlikely(!scheduler_running))

If memory serves there used to be more, but I think that migrated into
kernel/sched_clock.c, which has sched_clock_running.

> Also, might_sleep's use of system_state probably bears revisiting.

Yeah, all that code is from long before we had scheduler_running (which
was introduced around CFS/.23).

Re: [PATCH] netpoll: Fix carrier detection for drivers that are using phylib

[Matt Mackall]

On Thu, 2009-07-09 at 16:51 +0200, Peter Zijlstra wrote:
> > Does anything actually use scheduler_running yet? Perhaps my tree is
> > old.
> 
> # git grep scheduler_running
> kernel/sched.c:static __read_mostly int scheduler_running;
> kernel/sched.c: scheduler_running = 1;
> kernel/sched_rt.c:      if (unlikely(!scheduler_running))
> kernel/sched_rt.c:      if (unlikely(!scheduler_running))
> 
> If memory serves there used to be more, but I think that migrated into
> kernel/sched_clock.c, which has sched_clock_running.

The static in the first line makes that a bit of a head-scratcher? Oh, I
see: it's living in sin. Going to avert my eyes for now.

-- 
http://selenic.com : development and support for Mercurial and Linux

Re: [PATCH] netpoll: Fix carrier detection for drivers that are using phylib

[Linus Torvalds]

On Thu, 9 Jul 2009, Peter Zijlstra wrote:
> 
> So something like (utterly untested and such)

This looks like a good patch. Please make it so - who knows what other 
uses of cond_resched() we have in module init routines that might have 
deadlocks without it. The netpoll case got fixed, but please just do this.

I'd like to do my system_state movement too (the thing is, when you load 
drivers as modules you _will_ have "system_state == SYSTEM_RUNNING", so 
any initcall that depends on it being "early boot" is already broken), but 
there's no way that patch is appropriate for post-rc2. This one, however, 
looks appropriate (modulo getting some testing, of course)

		Linus

Re: [PATCH] netpoll: Fix carrier detection for drivers that are using phylib

[Matt Mackall]

On Thu, 2009-07-09 at 02:20 +0400, Anton Vorontsov wrote:
> Using early netconsole and gianfar driver this error pops up:
> 
>   netconsole: timeout waiting for carrier
> 
> It appears that net/core/netpoll.c:netpoll_setup() is using
> cond_resched() in a loop waiting for a carrier.
> 
> The thing is that cond_resched() is a no-op when system_state !=
> SYSTEM_RUNNING, and so drivers/net/phy/phy.c's state_queue is never
> scheduled, therefore link detection doesn't work.
> 
> I belive that the main problem is in cond_resched()[1], but despite
> how the cond_resched() story ends, it might be a good idea to call
> msleep(1) instead of cond_resched(), as suggested by Andrew Morton.
> 
> [1] http://lkml.org/lkml/2009/7/7/463
> 
> Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
> ---
> 
> On Wed, Jul 08, 2009 at 02:47:44PM -0700, Andrew Morton wrote:
> > (belatedly cc'ing netdev)
> > 
> > Original diagnosis:
> > 
> > : Using early netconsole and gianfar driver this error pops up:
> > : 
> > :   netconsole: timeout waiting for carrier
> > : 
> > : It appears that net/core/netpoll.c:netpoll_setup() is using
> > : cond_resched() in a loop waiting for a carrier.
> > : 
> > : The thing is that cond_resched() is a no-op when system_state !=
> > : SYSTEM_RUNNING, and so drivers/net/phy/phy.c's state_queue is never
> > : scheduled, therefore link detection doesn't work
> > 
> > > On Thu, 9 Jul 2009 01:33:31 +0400 Anton Vorontsov <avorontsov@ru.mvista.com> wrote:
> > > On Wed, Jul 08, 2009 at 02:10:24PM -0700, Andrew Morton wrote:
> > > > > On Wed, 8 Jul 2009 09:12:30 -0700 (PDT) Linus Torvalds <torvalds@linux-foundation.org> wrote:
> > > > > That said, I do agree that maybe SYSTEM_RUNNING isn't the right check. 
> > > > > Testing that the scheduler is initialized may be the more correct one. I 
> > > > > think the SYSTEM_RUNNING one just comes from that being used for other 
> > > > > debug issues.
> > > > 
> > > > Agreed.  system_state is too general.
> > > > 
> > > > If we specifically want to know whether it is safe to call schedule() then
> > > > let's create a global boolean it_is_safe_to_call_schedule and test that,
> > > > rather than testing something which indirectly and unreliably implies "it
> > > > is safe to call schedule".  If that boolean already exists then no-brainer.
> > > > 
> > > > All that being said, I wonder if the netconsole code should be using
> > > > msleep(1) instead.  Spinning on cond_resched() is a bit rude.  But one
> > > > would have to verify that it is safe to call schedule() at this time, and
> > > > for the netconsole caller, this is dubious.
> > > 
> > > What do you mean by "verify that it is safe"? If it works,
> > > can I assume that it's safe? ;-) It works, fwiw.
> > > 
> > 
> > netconsole is supposed to be available as early as possible in boot for
> > obvious reasons.  I'd say there's a decent risk now and in the future that
> > netconsole will be initialised prior to the scheduler being available.
> > 
> > In fact, if "netconsole: timeout waiting for carrier" newly added to
> > netpoll_setup() a depedency on the scheduler being available then perhaps
> > that was an incorrect change.
> 
> 'git blame' says that carrier detection code didn't change since 2.6.12
> (where git history starts), PHYLIB is using workqueue since its
> submission (2.6.13). And SYSTEM_RUNNING check was added in 2.6.16.
> So it's not a new dependency.
> 
> The netpoll code is using msleep() just a few lines below cond_resched(),
> so we won't make things worse. ;-)

I think that's an improvement with or without the SYSTEM_RUNNING fix.

Signed-off-by: Matt Mackall <mpm@selenic.com>

> Thanks!
> 
>  net/core/netpoll.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/net/core/netpoll.c b/net/core/netpoll.c
> index 9675f31..df30feb 100644
> --- a/net/core/netpoll.c
> +++ b/net/core/netpoll.c
> @@ -740,7 +740,7 @@ int netpoll_setup(struct netpoll *np)
>  				       np->name);
>  				break;
>  			}
> -			cond_resched();
> +			msleep(1);
>  		}
>  
>  		/* If carrier appears to come up instantly, we don't

-- 
http://selenic.com : development and support for Mercurial and Linux