[PATCH 3/17] arch/mips/kernel: Add missing read_unlock

[PATCH 3/17] arch/mips/kernel: Add missing read_unlock

[Julia Lawall]

From: Julia Lawall <julia@diku.dk>

Add a read_unlock missing on the error path.  Other ways of reaching
out_unlock have tasklist_lock unlocked.

The semantic match that finds this problem is as follows:
(http://coccinelle.lip6.fr/)

// <smpl>
@@
expression E1;
@@

* read_lock(E1,...);
  <+... when != E1
  if (...) {
    ... when != E1
*   return ...;
  }
  ...+>
* read_unlock(E1,...);
// </smpl>

Signed-off-by: Julia Lawall <julia@diku.dk>

---
I wasn't able to find what security_task_setscheduler actually does.  
If it releases tasklist_lock in an error case, then ignire this patch.

 arch/mips/kernel/mips-mt-fpaff.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/mips/kernel/mips-mt-fpaff.c b/arch/mips/kernel/mips-mt-fpaff.c
index f5981c4..73581bf 100644
--- a/arch/mips/kernel/mips-mt-fpaff.c
+++ b/arch/mips/kernel/mips-mt-fpaff.c
@@ -86,8 +86,10 @@ asmlinkage long mipsmt_sys_sched_setaffinity(pid_t pid, unsigned int len,
 	}
 
 	retval = security_task_setscheduler(p, 0, NULL);
-	if (retval)
+	if (retval) {
+		read_unlock(&tasklist_lock);
 		goto out_unlock;
+	}
 
 	/* Record new user-specified CPU set for future reference */
 	p->thread.user_cpus_allowed = new_mask;

Re: [PATCH 3/17] arch/mips/kernel: Add missing read_unlock

[Ralf Baechle]

On Wed, May 26, 2010 at 05:54:55PM +0200, Julia Lawall wrote:

> From: Julia Lawall <julia@diku.dk>
> 
> Add a read_unlock missing on the error path.  Other ways of reaching
> out_unlock have tasklist_lock unlocked.
> 
> The semantic match that finds this problem is as follows:
> (http://coccinelle.lip6.fr/)
> 
> // <smpl>
> @@
> expression E1;
> @@
> 
> * read_lock(E1,...);
>   <+... when != E1
>   if (...) {
>     ... when != E1
> *   return ...;
>   }
>   ...+>
> * read_unlock(E1,...);
> // </smpl>
> 
> Signed-off-by: Julia Lawall <julia@diku.dk>
> 
> ---
> I wasn't able to find what security_task_setscheduler actually does.  
> If it releases tasklist_lock in an error case, then ignire this patch.

Your patch appears correct - and mipsmt_sys_sched_setaffinity() even
more broken than you thought.  It duplicates some code from kernel/sched.c
and has gotten out of sync.

Fixing that up.

   Ralf

Affinity Automation (was Re: [PATCH 3/17] arch/mips/kernel: Add missing

[Kevin D. Kissell]

Ralf Baechle wrote:
> Your patch appears correct - and mipsmt_sys_sched_setaffinity() even
> more broken than you thought.  It duplicates some code from kernel/sched.c
> and has gotten out of sync.
Yeah, that was inevitable.  Since the distribution of the previous 
message seems to cover concerned developers outside the MIPS community, 
let me make one final(?) plea to actually do this right.

The MIPS SMTC support for managing a single FPU context on a processor 
with multiple integer TC contexts involves having the system make 
automous, real time, decisions about scheduling affinity.  It may be a 
first, but it can't possibly be the only case, especially as we've 
started seeing more and more mainstream multi-core, multi-thread 
designs.  System and chip resources are going to be "closer" to one 
processor or another.  The current Linux paradigm is that it's the 
responsibility of programs, or users, to know what the optimal placement 
of processes should be for a given system platform, and while it's 
absolutely appropriate to provide that level of control for the cases 
where the user really does know best, it's mildly insane to make that 
the only way that thread placement can be optimized.  It's really the 
OS's job to match demand to resources.

My contention years ago was, and remains, that it would be a bad idea to 
burden the main scheduler loop with checks for two different levels of 
affinity, system-automatic and user-specified.  It would add 
non-trivially to the cache footprint and execution overhead of thread 
dispatch, and there's no logical need for it.  So the model I proposed, 
and implemented in the cloned affinity system calls for SMTC, was that a 
*single* affinity mask continue to be used by the scheduler, but that 
the per-thread data structures carry two:  The one requested explicitly 
by the user, and the one actually used by the scheduler.  The idea is 
that normally those two are the same, but that the system - in the MIPS 
SMTC case, the FPU emulator - can overlay its constraints with the 
user's constraints to come up with an intersection-of-sets constraint 
that satisfies both (there was a clause which prevents system affinity 
heuristics from restricting the affinity mask to a null set of CPUs, 
though of course the user can do that if he really wants to).

There's nothing particularly MIPS-specific about the problem or the 
solution.  Most of the mechanisms should really be in 
platform-independent code, so we don't get the drift of cloned components.

I don't recall who owned the scheduler at the time, but whoever it was 
was too busy dealing with mainstream processor problems to even engage 
in a dialogue about this.  Is it time to raise the question again?

          Regards,

          Kevin K.

Re: Affinity Automation (was Re: [PATCH 3/17] arch/mips/kernel: Add

[Ralf Baechle]

On Wed, May 26, 2010 at 10:27:53AM -0700, Kevin D. Kissell wrote:
> Date:   Wed, 26 May 2010 10:27:53 -0700
> From: "Kevin D. Kissell" <kevink@paralogos.com>
> To: Ralf Baechle <ralf@linux-mips.org>
> CC: Julia Lawall <julia@diku.dk>, linux-mips@linux-mips.org,
> 	linux-kernel@vger.kernel.org, kernel-janitors@vger.kernel.org
> Subject: Affinity Automation (was Re: [PATCH 3/17] arch/mips/kernel: Add
>  missing
>  read_unlock)
> Content-Type: text/plain; charset=ISO-8859-1; format=flowed
> 
> Ralf Baechle wrote:
> >Your patch appears correct - and mipsmt_sys_sched_setaffinity() even
> >more broken than you thought.  It duplicates some code from kernel/sched.c
> >and has gotten out of sync.
> Yeah, that was inevitable.  Since the distribution of the previous
> message seems to cover concerned developers outside the MIPS
> community, let me make one final(?) plea to actually do this right.
> 
> The MIPS SMTC support for managing a single FPU context on a
> processor with multiple integer TC contexts involves having the
> system make automous, real time, decisions about scheduling
> affinity.  It may be a first, but it can't possibly be the only
> case, especially as we've started seeing more and more mainstream
> multi-core, multi-thread designs.  System and chip resources are
> going to be "closer" to one processor or another.  The current Linux
> paradigm is that it's the responsibility of programs, or users, to
> know what the optimal placement of processes should be for a given
> system platform, and while it's absolutely appropriate to provide
> that level of control for the cases where the user really does know
> best, it's mildly insane to make that the only way that thread
> placement can be optimized.  It's really the OS's job to match
> demand to resources.
> 
> My contention years ago was, and remains, that it would be a bad
> idea to burden the main scheduler loop with checks for two different
> levels of affinity, system-automatic and user-specified.  It would
> add non-trivially to the cache footprint and execution overhead of
> thread dispatch, and there's no logical need for it.  So the model I
> proposed, and implemented in the cloned affinity system calls for
> SMTC, was that a *single* affinity mask continue to be used by the
> scheduler, but that the per-thread data structures carry two:  The
> one requested explicitly by the user, and the one actually used by
> the scheduler.  The idea is that normally those two are the same,
> but that the system - in the MIPS SMTC case, the FPU emulator - can
> overlay its constraints with the user's constraints to come up with
> an intersection-of-sets constraint that satisfies both (there was a
> clause which prevents system affinity heuristics from restricting
> the affinity mask to a null set of CPUs, though of course the user
> can do that if he really wants to).
> 
> There's nothing particularly MIPS-specific about the problem or the
> solution.  Most of the mechanisms should really be in
> platform-independent code, so we don't get the drift of cloned
> components.
> 
> I don't recall who owned the scheduler at the time, but whoever it
> was was too busy dealing with mainstream processor problems to even
> engage in a dialogue about this.  Is it time to raise the question
> again?

That was Ingo Molnar.  The scheduler is owned by these two guys:

SCHEDULER
M:      Ingo Molnar <mingo@elte.hu>
M:      Peter Zijlstra <peterz@infradead.org>
S:      Maintained
F:      kernel/sched*
F:      include/linux/sched.h

I've added them to cc.

  Ralf