lkml.org 
[lkml]   [2021]   [Dec]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [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;
    > }

    \
     
     \ /
      Last update: 2021-12-13 23:35    [W:2.134 / U:0.560 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site