From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-3.8 required=3.0 tests=BAYES_00,DKIM_INVALID, DKIM_SIGNED,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_RED autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 20A59C433ED for ; Wed, 28 Apr 2021 15:10:04 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 3545D61434 for ; Wed, 28 Apr 2021 15:10:01 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 3545D61434 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=kernel.org Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id 7D0566B0036; Wed, 28 Apr 2021 11:10:00 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 7A6FE6B0071; Wed, 28 Apr 2021 11:10:00 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 648046B0072; Wed, 28 Apr 2021 11:10:00 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0030.hostedemail.com [216.40.44.30]) by kanga.kvack.org (Postfix) with ESMTP id 475946B0036 for ; Wed, 28 Apr 2021 11:10:00 -0400 (EDT) Received: from smtpin09.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay04.hostedemail.com (Postfix) with ESMTP id 03AC92816 for ; Wed, 28 Apr 2021 15:10:00 +0000 (UTC) X-FDA: 78082110960.09.5C4C165 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by imf24.hostedemail.com (Postfix) with ESMTP id CD8E3A000396 for ; Wed, 28 Apr 2021 15:09:48 +0000 (UTC) Received: by mail.kernel.org (Postfix) with ESMTPSA id 93A8D613F1; Wed, 28 Apr 2021 15:09:54 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1619622598; bh=zfZBsF0nlx3jPpL8Pj7qRtfJrcsoEwL61cXgYA0V1HI=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=bFbgW35ShYJfLrxmGPS8gcSH7tupL6CIXpbQP2F0Vx98SAR7/AK/AAhMkbTdBPHTn qpMnX9iAzTM1fQF6a5f194/q4EnmEwf1lJ9pDQEG08nIpBudo628+9SopwI5Qu/YtB AfIK4L1q26qntnlvZpYeIUWRatAYm8Uqfs8Y6AtfkSw5tokTbuO5AXtK1zXQskW8T/ 46lJ1O1z3ENQZSMtXqe3/cxL9L6jRtz3CQU5wAqvKa9D52KbHmBPoP8LvpZgiOQ6y1 Vi+CYzggC2W8n1+rCkgxebcenVC6xis3tXpXMN4craPpSYJBBBbaxMXi9i/WgJ+HMT 42HYqSHZWPAjA== Date: Wed, 28 Apr 2021 17:09:52 +0200 From: Alexey Gladkov To: Oliver Sang Cc: "Eric W. Biederman" , Linus Torvalds , Alexey Gladkov , 0day robot , LKML , lkp@lists.01.org, "Huang, Ying" , Feng Tang , zhengjun.xing@intel.com, Kernel Hardening , Linux Containers , Linux-MM , Andrew Morton , Christian Brauner , Jann Horn , Jens Axboe , Kees Cook , Oleg Nesterov Subject: Re: 08ed4efad6: stress-ng.sigsegv.ops_per_sec -41.9% regression Message-ID: <20210428150952.mdnvl7i4kimgwswh@example.org> References: <7abe5ab608c61fc2363ba458bea21cf9a4a64588.1617814298.git.gladkov.alexey@gmail.com> <20210408083026.GE1696@xsang-OptiPlex-9020> <20210423024722.GA13968@xsang-OptiPlex-9020> <20210423074431.7ob6aqasome2zjbk@example.org> <20210428143008.GA19916@xsang-OptiPlex-9020> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20210428143008.GA19916@xsang-OptiPlex-9020> X-Stat-Signature: bofptesoco5u9ggnqjjf5zdbee3ktko6 X-Rspamd-Server: rspam04 X-Rspamd-Queue-Id: CD8E3A000396 Received-SPF: none (kernel.org>: No applicable sender policy available) receiver=imf24; identity=mailfrom; envelope-from=""; helo=mail.kernel.org; client-ip=198.145.29.99 X-HE-DKIM-Result: pass/pass X-HE-Tag: 1619622588-380345 Content-Transfer-Encoding: quoted-printable X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On Wed, Apr 28, 2021 at 10:36:40PM +0800, Oliver Sang wrote: > hi, Alexey Gladkov, >=20 > On Fri, Apr 23, 2021 at 09:44:31AM +0200, Alexey Gladkov wrote: > > On Fri, Apr 23, 2021 at 10:47:22AM +0800, Oliver Sang wrote: > > > hi, Eric, > > >=20 > > > On Thu, Apr 08, 2021 at 01:44:43PM -0500, Eric W. Biederman wrote: > > > > Linus Torvalds writes: > > > >=20 > > > > > On Thu, Apr 8, 2021 at 1:32 AM kernel test robot wrote: > > > > >> > > > > >> FYI, we noticed a -41.9% regression of stress-ng.sigsegv.ops_p= er_sec due to commit > > > > >> 08ed4efad684 ("[PATCH v10 6/9] Reimplement RLIMIT_SIGPENDING o= n top of ucounts") > > > > > > > > > > Ouch. > > > >=20 > > > > We were cautiously optimistic when no test problems showed up fro= m > > > > the last posting that there was nothing to look at here. > > > >=20 > > > > Unfortunately it looks like the bots just missed the last posting= .=20 > > >=20 > > > this report is upon v10. do you have newer version which hope bot t= est? > >=20 > > Yes. I posted a new version of this patch set. I would be very gratef= ul if > > you could test it. > >=20 > > https://lore.kernel.org/lkml/cover.1619094428.git.legion@kernel.org/ > > >=20 > we tested this v11 version, and found the regression reduced to about 1= .6%. > please be noted, according to our previous experience, the stress-ng is > kind of sensitive testsuite, so we normally wouldn't report <3% regress= ion. Thank you very much for testing and good news for me !!! Do you have a place where its possible to see if the patch has been teste= d? I mean test passed or not. > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D > class/compiler/cpufreq_governor/disk/kconfig/nr_threads/rootfs/tbox_gro= up/test/testcase/testtime/ucode: > interrupt/gcc-9/performance/1HDD/x86_64-rhel-8.3/100%/debian-10.4-x86= _64-20200603.cgz/lkp-ivb-2ep1/sigsegv/stress-ng/60s/0x42e >=20 > commit: > 00a58a6af1c4 ("Reimplement RLIMIT_MSGQUEUE on top of ucounts") > 8932738fc10c ("Reimplement RLIMIT_SIGPENDING on top of ucounts") >=20 > 00a58a6af1c473c5 8932738fc10c4398521892adfe6 > ---------------- --------------------------- > %stddev %change %stddev > \ | \ > 4.745e+08 -1.6% 4.669e+08 stress-ng.sigsegv.ops > 7908964 -1.6% 7781343 stress-ng.sigsegv.ops_per= _sec >=20 > Below is some data of results from your new branch and base. > b3ad8e1fa3fd8 ucounts: Set ucount_max to the largest positive value the= type can hold 7783421.61 7794441.59 7775793.52 7773683.6 7760744.1 7757= 720.33 > 8932738fc10c4 Reimplement RLIMIT_SIGPENDING on top of ucounts = 7755985.06 7780646.72 7783944.12 7809090.98 7798193.32 77= 60202.59 > 00a58a6af1c47 Reimplement RLIMIT_MSGQUEUE on top of ucounts = 7940474.72 7912442.26 7879195.61 7869803.63 7912693.69 79= 39175.48 > e75074781f173 selftests/resctrl: Change a few printed messages = 7660254.5 7676124.45 7745330.79 7736754.88 7716834.93 766= 0143.13 > 87f1c20e2effd Documentation: kselftest: fix path to test module files = 7729609.16 7726906.92 7760819.26 > 06bd03a57f8c2 selftests/resctrl: Fix MBA/MBM results reporting format = 7692866.06 7730606.11 7681414.48 > a38fd87484648 Linux 5.12-rc2 = 7724932.06=20 >=20 > =20 > > > please be noted, sorry to say, due to various reasons, it will be a > > > big challenge for us to capture each version of a patch set. > > >=20 > > > e.g. we didn't make out a similar performance regression for > > > v8/v9 version of this one.. > > >=20 > > > >=20 > > > > So it seems we are finally pretty much at correct code in need > > > > of performance tuning. > > > >=20 > > > > > I *think* this test may be testing "send so many signals that i= t > > > > > triggers the signal queue overflow case". > > > > > > > > > > And I *think* that the performance degradation may be due to lo= ts of > > > > > unnecessary allocations, because ity looks like that commit cha= nges > > > > > __sigqueue_alloc() to do > > > > > > > > > > struct sigqueue *q =3D kmem_cache_alloc(sigqueue_cachep= , flags); > > > > > > > > > > *before* checking the signal limit, and then if the signal limi= t was > > > > > exceeded, it will just be free'd instead. > > > > > > > > > > The old code would check the signal count against RLIMIT_SIGPEN= DING > > > > > *first*, and if there were m ore pending signals then it wouldn= 't do > > > > > anything at all (including not incrementing that expensive atom= ic > > > > > count). > > > >=20 > > > > This is an interesting test in a lot of ways as it is testing the > > > > synchronous signal delivery path caused by an exception. The tes= t > > > > is either executing *ptr =3D 0 (where ptr points to a read-only p= age) > > > > or it executes an x86 instruction that is excessively long. > > > >=20 > > > > I have found the code but I haven't figured out how it is being > > > > called yet. The core loop is just: > > > > for(;;) { > > > > sigaction(SIGSEGV, &action, NULL); > > > > sigaction(SIGILL, &action, NULL); > > > > sigaction(SIGBUS, &action, NULL); > > > >=20 > > > > ret =3D sigsetjmp(jmp_env, 1); > > > > if (done()) > > > > break; > > > > if (ret) { > > > > /* verify signal */ > > > > } else { > > > > *ptr =3D 0; > > > > } > > > > } > > > >=20 > > > > Code like that fundamentally can not be multi-threaded. So the o= nly way > > > > the sigpending limit is being hit is if there are more processes = running > > > > that code simultaneously than the size of the limit. > > > >=20 > > > > Further it looks like stress-ng pushes RLIMIT_SIGPENDING as high = as it > > > > will go before the test starts. > > > >=20 > > > >=20 > > > > > Also, the old code was very careful to only do the "get_user()"= for > > > > > the *first* signal it added to the queue, and do the "put_user(= )" for > > > > > when removing the last signal. Exactly because those atomics ar= e very > > > > > expensive. > > > > > > > > > > The new code just does a lot of these atomics unconditionally. > > > >=20 > > > > Yes. That seems a likely culprit. > > > >=20 > > > > > I dunno. The profile data in there is a bit hard to read, but t= here's > > > > > a lot more cachee misses, and a *lot* of node crossers: > > > > > > > > > >> 5961544 +190.4% 17314361 perf-stat.i.cach= e-misses > > > > >> 22107466 +119.2% 48457656 perf-stat.i.cach= e-references > > > > >> 163292 =C4=85 3% +4582.0% 7645410 perf-stat.i= .node-load-misses > > > > >> 227388 =C4=85 2% +3708.8% 8660824 perf-stat.i= .node-loads > > > > > > > > > > and (probably as a result) average instruction costs have gone = up enormously: > > > > > > > > > >> 3.47 +66.8% 5.79 perf-stat.overal= l.cpi > > > > >> 22849 -65.6% 7866 perf-stat.overal= l.cycles-between-cache-misses > > > > > > > > > > and it does seem to be at least partly about "put_ucounts()": > > > > > > > > > >> 0.00 +4.5 4.46 perf-profile.cal= ltrace.cycles-pp.put_ucounts.__sigqueue_free.get_signal.arch_do_signal_or= _restart.exit_to_user_mode_prepare > > > > > > > > > > and a lot of "get_ucounts()". > > > > > > > > > > But it may also be that the new "get sigpending" is just *so* m= uch > > > > > more expensive than it used to be. > > > >=20 > > > > That too is possible. > > > >=20 > > > > That node-load-misses number does look like something is bouncing= back > > > > and forth between the nodes a lot more. So I suspect stress-ng i= s > > > > running multiple copies of the sigsegv test in different processe= s at > > > > once. > > > >=20 > > > >=20 > > > >=20 > > > > That really suggests cache line ping pong from get_ucounts and > > > > incrementing sigpending. > > > >=20 > > > > It surprises me that obtaining the cache lines exclusively is > > > > the dominant cost on this code path but obtaining two cache lines > > > > exclusively instead of one cache cache line exclusively is consis= tent > > > > with a causing the exception delivery to take nearly twice as lon= g. > > > >=20 > > > > For the optimization we only care about the leaf count so with a = little > > > > care we can restore the optimization. So that is probably the th= ing > > > > to do here. The fewer changes to worry about the less likely to = find > > > > surprises. > > > >=20 > > > >=20 > > > >=20 > > > > That said for this specific case there is a lot of potential room= for > > > > improvement. As this is a per thread signal the code update sigp= ending > > > > in commit_cred and never worry about needing to pin the struct > > > > user_struct or struct ucounts. As this is a synchronous signal w= e could > > > > skip the sigpending increment, skip the signal queue entirely, an= d > > > > deliver the signal to user-space immediately. The removal of all= cache > > > > ping pongs might make it worth it. > > > >=20 > > > > There is also Thomas Gleixner's recent optimization to cache one > > > > sigqueue entry per task to give more predictable behavior. That > > > > would remove the cost of the allocation. > > > >=20 > > > > Eric > > >=20 > >=20 > > --=20 > > Rgrds, legion > >=20 >=20 --=20 Rgrds, legion