linux-kernel - Re: [rfc] forked kernel task and mm structures imbalanced on NUMA

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: <1275383152.27810.26387.camel@twins>
Date:	Tue, 01 Jun 2010 11:05:52 +0200
From:	Peter Zijlstra <peterz@...radead.org>
To:	Nick Piggin <npiggin@...e.de>
Cc:	Linus Torvalds <torvalds@...ux-foundation.org>,
	Ingo Molnar <mingo@...hat.com>, linux-mm@...ck.org,
	linux-kernel@...r.kernel.org,
	Lee Schermerhorn <lee.schermerhorn@...com>
Subject: Re: [rfc] forked kernel task and mm structures imbalanced on NUMA

On Tue, 2010-06-01 at 18:41 +1000, Nick Piggin wrote:
> > So I think it would make sense to rework the fork balancing muck to be
> > called only once and stick with its decision.
> 
> Just need to close that race somehow. AFAIKS we can't use TASK_WAKING
> because that must not be preempted?

Right its basically a bit-spinlock and since it interacts with the
rq->lock it needs to have IRQs disabled while we have it set -- which
isn't a problem for the wakeup path, but it would be for the whole fork
path.

> > One thing that would make the whole fork path much easier is fully
> > ripping out that child_runs_first mess for CONFIG_SMP, I think its been
> > disabled by default for long enough, and its always been broken in the
> > face of fork balancing anyway.
> 
> Interesting problem. vfork is nice for fork+exec, but it's a bit
> restrictive.

Right, and all that is a separate issue, its broken now, its still
broken with child_runs_first ripped out.
 
> > So basically we have to move fork balancing back to sched_fork(), I'd
> > have to again look at wth happens to ->cpus_allowed, but I guess it
> > should be fixable, and I don't think we should care overly much about
> > cpu-hotplug.
> 
> No more than simply getting it right. Simply calling into the balancer
> again seems to be the simplest way to do it.

Right.

> > A specific code comment:
> > 
> > > @@ -2550,14 +2561,16 @@ void wake_up_new_task(struct task_struct
> > >          * We set TASK_WAKING so that select_task_rq() can drop rq->lock
> > >          * without people poking at ->cpus_allowed.
> > >          */
> > > -       cpu = select_task_rq(rq, p, SD_BALANCE_FORK, 0);
> > > -       set_task_cpu(p, cpu);
> > > -
> > > -       p->state = TASK_RUNNING;
> > > -       task_rq_unlock(rq, &flags);
> > > +       if (!cpumask_test_cpu(cpu, &p->cpus_allowed)) {
> > > +               p->state = TASK_WAKING;
> > > +               cpu = select_task_rq(rq, p, SD_BALANCE_FORK, 0);
> > > +               set_task_cpu(p, cpu);
> > > +               p->state = TASK_RUNNING;
> > > +               task_rq_unlock(rq, &flags);
> > > +               rq = task_rq_lock(p, &flags);
> > > +       }
> > >  #endif
> > 
> > That's iffy because p->cpus_allowed isn't stable when we're not holding
> > the task's current rq->lock or p->state is not TASK_WAKING.
> > 
> 
> Oop, yeah missed that. Half hearted attempt to avoid more rq locks. 

Yeah, something well worth the effort. At one point I considered making
p->cpus_allowed an RCU managed cpumask, but I never sat down and ran
through all the interesting races that that would bring.
--
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/