From: Feng Tang <feng.tang@intel.com> To: Linus Torvalds <torvalds@linux-foundation.org> Cc: Oleg Nesterov <oleg@redhat.com>, "Eric W. Biederman" <ebiederm@xmission.com>, Jiri Olsa <jolsa@redhat.com>, Peter Zijlstra <peterz@infradead.org>, kernel test robot <rong.a.chen@intel.com>, Ingo Molnar <mingo@kernel.org>, Vince Weaver <vincent.weaver@maine.edu>, Jiri Olsa <jolsa@kernel.org>, Alexander Shishkin <alexander.shishkin@linux.intel.com>, Arnaldo Carvalho de Melo <acme@kernel.org>, Arnaldo Carvalho de Melo <acme@redhat.com>, "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com>, Ravi Bangoria <ravi.bangoria@linux.ibm.com>, Stephane Eranian <eranian@google.com>, Thomas Gleixner <tglx@linutronix.de>, LKML <linux-kernel@vger.kernel.org>, lkp@lists.01.org, andi.kleen@intel.com, "Huang, Ying" <ying.huang@intel.com> Subject: Re: [LKP] Re: [perf/x86] 81ec3f3c4c: will-it-scale.per_process_ops -5.5% regression Date: Tue, 25 Feb 2020 10:57:48 +0800 [thread overview] Message-ID: <20200225025748.GB63065@shbuild999.sh.intel.com> (raw) In-Reply-To: <CAHk-=wgbR4ocHAOiaj7x+V7dVoYr-mD2N7Y_MRPJ+Q+GohDYeg@mail.gmail.com> On Mon, Feb 24, 2020 at 12:47:14PM -0800, Linus Torvalds wrote: > [ Adding a few more people that tend to be involved in signal > handling. Just in case - even if they probably don't care ] > > On Mon, Feb 24, 2020 at 12:09 PM Linus Torvalds > > I've tested it, and the profiles on the silly microbenchmark look much > nicer. Now it's just the sigpending update shows up, the refcount case > clearly still occasionally happens, but it's now in the noise. > > I made slight changes to the __sigqueue_alloc() case to generate > better code: since we now use that atomic_inc_return() anyway, we > might as well then use the value that is returned for the > RLIMIT_SIGPENDING check too, instead of reading it again. > > That might avoid another potential cacheline bounce, plus the > generated code just looks better. > > Updated (and now slightly tested!) patch attached. > It would be interesting if this is noticeable on your benchmark > numbers. I didn't actually _time_ anything, I just looked at profiles. > > But my setup clearly isn't going to see the horrible contention case > anyway, so my timing numbers wouldn't be all that interesting. Thanks for the optimization patch for signal! It makes a big difference, that the performance score is tripled! bump from original 17000 to 54000. Also the gap between 5.0-rc6 and 5.0-rc6+Jiri's patch is reduced to around 2%. The test I run is inserting your patch right before 5.0-rc6, then run the test for 5.0-rc6 and 5.0-rc6+Jiri's patch. Sorry it took quite some time, as the test platform is not local but inside 0day's framework, which takes some time for scheduling, kbuilding and testing. Thanks, Feng > Linus > kernel/signal.c | 23 ++++++++++++++--------- > 1 file changed, 14 insertions(+), 9 deletions(-) > > diff --git a/kernel/signal.c b/kernel/signal.c > index 9ad8dea93dbb..5b2396350dd1 100644 > --- a/kernel/signal.c > +++ b/kernel/signal.c > @@ -413,27 +413,32 @@ __sigqueue_alloc(int sig, struct task_struct *t, gfp_t flags, int override_rlimi > { > struct sigqueue *q = NULL; > struct user_struct *user; > + int sigpending; > > /* > * Protect access to @t credentials. This can go away when all > * callers hold rcu read lock. > + * > + * NOTE! A pending signal will hold on to the user refcount, > + * and we get/put the refcount only when the sigpending count > + * changes from/to zero. > */ > rcu_read_lock(); > - user = get_uid(__task_cred(t)->user); > - atomic_inc(&user->sigpending); > + user = __task_cred(t)->user; > + sigpending = atomic_inc_return(&user->sigpending); > + if (sigpending == 1) > + get_uid(user); > rcu_read_unlock(); > > - if (override_rlimit || > - atomic_read(&user->sigpending) <= > - task_rlimit(t, RLIMIT_SIGPENDING)) { > + if (override_rlimit || likely(sigpending <= task_rlimit(t, RLIMIT_SIGPENDING))) { > q = kmem_cache_alloc(sigqueue_cachep, flags); > } else { > print_dropped_signal(sig); > } > > if (unlikely(q == NULL)) { > - atomic_dec(&user->sigpending); > - free_uid(user); > + if (atomic_dec_and_test(&user->sigpending)) > + free_uid(user); > } else { > INIT_LIST_HEAD(&q->list); > q->flags = 0; > @@ -447,8 +452,8 @@ static void __sigqueue_free(struct sigqueue *q) > { > if (q->flags & SIGQUEUE_PREALLOC) > return; > - atomic_dec(&q->user->sigpending); > - free_uid(q->user); > + if (atomic_dec_and_test(&q->user->sigpending)) > + free_uid(q->user); > kmem_cache_free(sigqueue_cachep, q); > } >
WARNING: multiple messages have this Message-ID (diff)
From: Feng Tang <feng.tang@intel.com> To: lkp@lists.01.org Subject: Re: [perf/x86] 81ec3f3c4c: will-it-scale.per_process_ops -5.5% regression Date: Tue, 25 Feb 2020 10:57:48 +0800 [thread overview] Message-ID: <20200225025748.GB63065@shbuild999.sh.intel.com> (raw) In-Reply-To: <CAHk-=wgbR4ocHAOiaj7x+V7dVoYr-mD2N7Y_MRPJ+Q+GohDYeg@mail.gmail.com> [-- Attachment #1: Type: text/plain, Size: 3610 bytes --] On Mon, Feb 24, 2020 at 12:47:14PM -0800, Linus Torvalds wrote: > [ Adding a few more people that tend to be involved in signal > handling. Just in case - even if they probably don't care ] > > On Mon, Feb 24, 2020 at 12:09 PM Linus Torvalds > > I've tested it, and the profiles on the silly microbenchmark look much > nicer. Now it's just the sigpending update shows up, the refcount case > clearly still occasionally happens, but it's now in the noise. > > I made slight changes to the __sigqueue_alloc() case to generate > better code: since we now use that atomic_inc_return() anyway, we > might as well then use the value that is returned for the > RLIMIT_SIGPENDING check too, instead of reading it again. > > That might avoid another potential cacheline bounce, plus the > generated code just looks better. > > Updated (and now slightly tested!) patch attached. > It would be interesting if this is noticeable on your benchmark > numbers. I didn't actually _time_ anything, I just looked at profiles. > > But my setup clearly isn't going to see the horrible contention case > anyway, so my timing numbers wouldn't be all that interesting. Thanks for the optimization patch for signal! It makes a big difference, that the performance score is tripled! bump from original 17000 to 54000. Also the gap between 5.0-rc6 and 5.0-rc6+Jiri's patch is reduced to around 2%. The test I run is inserting your patch right before 5.0-rc6, then run the test for 5.0-rc6 and 5.0-rc6+Jiri's patch. Sorry it took quite some time, as the test platform is not local but inside 0day's framework, which takes some time for scheduling, kbuilding and testing. Thanks, Feng > Linus > kernel/signal.c | 23 ++++++++++++++--------- > 1 file changed, 14 insertions(+), 9 deletions(-) > > diff --git a/kernel/signal.c b/kernel/signal.c > index 9ad8dea93dbb..5b2396350dd1 100644 > --- a/kernel/signal.c > +++ b/kernel/signal.c > @@ -413,27 +413,32 @@ __sigqueue_alloc(int sig, struct task_struct *t, gfp_t flags, int override_rlimi > { > struct sigqueue *q = NULL; > struct user_struct *user; > + int sigpending; > > /* > * Protect access to @t credentials. This can go away when all > * callers hold rcu read lock. > + * > + * NOTE! A pending signal will hold on to the user refcount, > + * and we get/put the refcount only when the sigpending count > + * changes from/to zero. > */ > rcu_read_lock(); > - user = get_uid(__task_cred(t)->user); > - atomic_inc(&user->sigpending); > + user = __task_cred(t)->user; > + sigpending = atomic_inc_return(&user->sigpending); > + if (sigpending == 1) > + get_uid(user); > rcu_read_unlock(); > > - if (override_rlimit || > - atomic_read(&user->sigpending) <= > - task_rlimit(t, RLIMIT_SIGPENDING)) { > + if (override_rlimit || likely(sigpending <= task_rlimit(t, RLIMIT_SIGPENDING))) { > q = kmem_cache_alloc(sigqueue_cachep, flags); > } else { > print_dropped_signal(sig); > } > > if (unlikely(q == NULL)) { > - atomic_dec(&user->sigpending); > - free_uid(user); > + if (atomic_dec_and_test(&user->sigpending)) > + free_uid(user); > } else { > INIT_LIST_HEAD(&q->list); > q->flags = 0; > @@ -447,8 +452,8 @@ static void __sigqueue_free(struct sigqueue *q) > { > if (q->flags & SIGQUEUE_PREALLOC) > return; > - atomic_dec(&q->user->sigpending); > - free_uid(q->user); > + if (atomic_dec_and_test(&q->user->sigpending)) > + free_uid(q->user); > kmem_cache_free(sigqueue_cachep, q); > } >
next prev parent reply other threads:[~2020-02-25 2:57 UTC|newest] Thread overview: 57+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-02-05 12:32 [perf/x86] 81ec3f3c4c: will-it-scale.per_process_ops -5.5% regression kernel test robot 2020-02-05 12:32 ` kernel test robot 2020-02-05 12:58 ` Peter Zijlstra 2020-02-05 12:58 ` Peter Zijlstra 2020-02-06 3:04 ` [LKP] " Li, Philip 2020-02-06 3:04 ` Li, Philip 2020-02-21 8:03 ` [LKP] " Feng Tang 2020-02-21 8:03 ` Feng Tang 2020-02-21 10:58 ` [LKP] " Peter Zijlstra 2020-02-21 10:58 ` Peter Zijlstra 2020-02-21 13:20 ` [LKP] " Jiri Olsa 2020-02-21 13:20 ` Jiri Olsa 2020-02-23 14:11 ` [LKP] " Feng Tang 2020-02-23 14:11 ` Feng Tang 2020-02-23 17:37 ` [LKP] " Linus Torvalds 2020-02-23 17:37 ` Linus Torvalds 2020-02-24 0:33 ` [LKP] " Feng Tang 2020-02-24 0:33 ` Feng Tang 2020-02-24 1:06 ` [LKP] " Linus Torvalds 2020-02-24 1:06 ` Linus Torvalds 2020-02-24 1:58 ` [LKP] " Huang, Ying 2020-02-24 1:58 ` Huang, Ying 2020-02-24 2:19 ` [LKP] " Feng Tang 2020-02-24 2:19 ` Feng Tang 2020-02-24 13:20 ` [LKP] " Feng Tang 2020-02-24 13:20 ` Feng Tang 2020-02-24 19:24 ` [LKP] " Linus Torvalds 2020-02-24 19:24 ` Linus Torvalds 2020-02-24 19:42 ` [LKP] " Kleen, Andi 2020-02-24 19:42 ` Kleen, Andi 2020-02-24 20:09 ` [LKP] " Linus Torvalds 2020-02-24 20:09 ` Linus Torvalds 2020-02-24 20:47 ` [LKP] " Linus Torvalds 2020-02-24 20:47 ` Linus Torvalds 2020-02-24 21:20 ` [LKP] " Eric W. Biederman 2020-02-24 21:20 ` Eric W. Biederman 2020-02-24 21:43 ` [LKP] " Linus Torvalds 2020-02-24 21:43 ` Linus Torvalds 2020-02-24 21:59 ` [LKP] " Eric W. Biederman 2020-02-24 21:59 ` Eric W. Biederman 2020-02-24 22:12 ` [LKP] " Linus Torvalds 2020-02-24 22:12 ` Linus Torvalds 2020-02-25 2:57 ` Feng Tang [this message] 2020-02-25 2:57 ` Feng Tang 2020-02-25 3:15 ` [LKP] " Linus Torvalds 2020-02-25 3:15 ` Linus Torvalds 2020-02-25 4:53 ` [LKP] " Feng Tang 2020-02-25 4:53 ` Feng Tang 2020-02-23 19:36 ` [LKP] " Jiri Olsa 2020-02-23 19:36 ` Jiri Olsa 2020-02-24 1:14 ` Feng Tang 2020-02-21 18:05 ` [LKP] " Kleen, Andi 2020-02-21 18:05 ` Kleen, Andi 2020-02-22 12:43 ` [LKP] " Feng Tang 2020-02-22 12:43 ` Feng Tang 2020-02-22 17:08 ` [LKP] " Kleen, Andi 2020-02-22 17:08 ` Kleen, Andi
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20200225025748.GB63065@shbuild999.sh.intel.com \ --to=feng.tang@intel.com \ --cc=acme@kernel.org \ --cc=acme@redhat.com \ --cc=alexander.shishkin@linux.intel.com \ --cc=andi.kleen@intel.com \ --cc=ebiederm@xmission.com \ --cc=eranian@google.com \ --cc=jolsa@kernel.org \ --cc=jolsa@redhat.com \ --cc=linux-kernel@vger.kernel.org \ --cc=lkp@lists.01.org \ --cc=mingo@kernel.org \ --cc=naveen.n.rao@linux.vnet.ibm.com \ --cc=oleg@redhat.com \ --cc=peterz@infradead.org \ --cc=ravi.bangoria@linux.ibm.com \ --cc=rong.a.chen@intel.com \ --cc=tglx@linutronix.de \ --cc=torvalds@linux-foundation.org \ --cc=vincent.weaver@maine.edu \ --cc=ying.huang@intel.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.