Messages in this thread | | | From | (Eric W. Biederman) | Date | Mon, 13 Dec 2021 16:34:04 -0600 | Subject | Re: [PATCH] rlimits: do not grab tasklist_lock for do_prlimit on current |
| |
Barret Rhoden <brho@google.com> writes:
> The tasklist_lock can be a scalability bottleneck. For current tasks, > we don't need the tasklist_lock to protect tsk->sighand or tsk->signal. > If non-current callers become a bottleneck, we could use > lock_task_sighand().
Do you have any numbers? As the entire point of this change is performance it would be good to see how the performance changes.
Especially as a read_lock should not be too bad as it allows sharing, nor do I expect reading or writing the rlimit values to be particularly frequent. So some insight into what kinds of userspace patterns make this a problem would be nice.
This change is a bit scary as it makes taking a lock conditional and increases the probability of causing a locking mistake.
If you are going to make this change I would say that do_prlimit should become static and taking the tasklist_lock should move into prlimit64.
Looking a little closer it looks like that update_rlimit_cpu should use lock_task_sighand, and once lock_task_sighand is used there is actually no need for the tasklist_lock at all. As holding the reference to tsk guarantees that tsk->signal remains valid.
So I completely agree there are cleanups that can happen in this area. Please make those and show numbers in how they improve things, instead of making the code worse with a conditional lock.
Eric
> Signed-off-by: Barret Rhoden <brho@google.com> > --- > kernel/sys.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/kernel/sys.c b/kernel/sys.c > index 8fdac0d90504..e56d1ae910af 100644 > --- a/kernel/sys.c > +++ b/kernel/sys.c > @@ -1576,7 +1576,8 @@ int do_prlimit(struct task_struct *tsk, unsigned int resource, > } > > /* protect tsk->signal and tsk->sighand from disappearing */ > - read_lock(&tasklist_lock); > + if (tsk != current) > + read_lock(&tasklist_lock); > if (!tsk->sighand) { > retval = -ESRCH; > goto out; > @@ -1611,7 +1612,8 @@ int do_prlimit(struct task_struct *tsk, unsigned int resource, > IS_ENABLED(CONFIG_POSIX_TIMERS)) > update_rlimit_cpu(tsk, new_rlim->rlim_cur); > out: > - read_unlock(&tasklist_lock); > + if (tsk != current) > + read_unlock(&tasklist_lock); > return retval; > }
| |