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

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: <1247147206.7439.2.camel@twins>
Date:	Thu, 09 Jul 2009 15:46:46 +0200
From:	Peter Zijlstra <a.p.zijlstra@...llo.nl>
To:	Matt Mackall <mpm@...enic.com>
Cc:	Linus Torvalds <torvalds@...ux-foundation.org>,
	Anton Vorontsov <avorontsov@...mvista.com>,
	Andrew Morton <akpm@...ux-foundation.org>, oleg@...hat.com,
	mingo@...e.hu, linux-kernel@...r.kernel.org, netdev@...r.kernel.org
Subject: Re: [PATCH] netpoll: Fix carrier detection for drivers that are
 using phylib

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.



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html