Re: [PATCH] rwlock_t unfairness and tasklist_lock

Michel Lespinasse
2013-01-12T03:33:52Z

On Fri, Jan 11, 2013 at 03:34:41PM +0100, Thomas Gleixner wrote:
> On Tue, 8 Jan 2013, Michel Lespinasse wrote:
> > - Does anyone know of any current work towards removing the
> > tasklist_lock use of rwlock_t ? Thomas Gleixner mentioned 3 years ago
> > that he'd give it a shot (https://lwn.net/Articles/364601/), did he
> > encounter some unforeseen difficulty that we should learn from ?
>
> I converted quite a bunch of the read side instances to rcu
> protection, but got distracted. There was no fundamental difficulty,
> just lack of time.

All right. Thanks for explaining here and offline; it looks like the
problem is not as intractable as I had thought initially.

> > - Would there be any fundamental objection to implementing a fair
> > rwlock_t and dealing with the reentrancy issues in tasklist_lock ? My
> > proposal there would be along the lines of:
> >
> > 1- implement a fair rwlock_t - the ticket based idea from David
> > Howells seems quite appropriate to me
>
> Nah. Lets get it killed. Most of the stuff can be converted to RCU and
> the remaining bits and pieces are the write lock sides which then can
> be converted to a big standard spinlock. There might be a few more
> complex ones, but Oleg said back then that those should be solved by
> locking the process instead of locking the whole tasklist.

So I looked again at getpriority() since that's what I had used for my
DOS test code, and it looks like everything there is already protected
by RCU or smaller granularity locks and refcounts. Patch attached to
remove this tasklist_lock usage.

Since I'm new to this, I would like someone to double check me.
Also, what is the proper tree to send such patches to so they'll get
some testing before making it into Linus's tree ?

--------------------------------8<-----------------------------

remove use of tasklist_lock in getpriority / setpriority syscalls

I can't see anything in these syscalls that isn't already protected
by RCU (for the task/thread iterations and for mapping pids to tasks)
or by smaller granularity locks (for set_one_prio()) or refcounts
(for find_user()). So, it looks like we can just remove the use of
tasklist_lock...

Signed-off-by: Michel Lespinasse <walken@google.com>

---
kernel/sys.c | 4 ----
1 files changed, 0 insertions(+), 4 deletions(-)

diff --git a/kernel/sys.c b/kernel/sys.c
index 265b37690421..5df66d4b118f 100644
--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -189,7 +189,6 @@ SYSCALL_DEFINE3(setpriority, int, which, int, who, int, niceval)
niceval = 19;

rcu_read_lock();
- read_lock(&tasklist_lock);
switch (which) {
case PRIO_PROCESS:
if (who)
@@ -226,7 +225,6 @@ SYSCALL_DEFINE3(setpriority, int, which, int, who, int, niceval)
break;
}
out_unlock:
- read_unlock(&tasklist_lock);
rcu_read_unlock();
out:
return error;
@@ -251,7 +249,6 @@ SYSCALL_DEFINE2(getpriority, int, which, int, who)
return -EINVAL;

rcu_read_lock();
- read_lock(&tasklist_lock);
switch (which) {
case PRIO_PROCESS:
if (who)
@@ -296,7 +293,6 @@ SYSCALL_DEFINE2(getpriority, int, which, int, who)
break;
}
out_unlock:
- read_unlock(&tasklist_lock);
rcu_read_unlock();

return retval;