Received: by 2002:a05:6a10:a841:0:0:0:0 with SMTP id d1csp985309pxy; Thu, 22 Apr 2021 19:35:18 -0700 (PDT) X-Google-Smtp-Source: ABdhPJzGDwc3a+d9JcrcV3MGwiVCNzO7Y/FldNsaovodE/93N2mJogxWUYMCjZbjr2moTtz3RLEh X-Received: by 2002:a63:eb10:: with SMTP id t16mr1642397pgh.393.1619145318594; Thu, 22 Apr 2021 19:35:18 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1619145318; cv=none; d=google.com; s=arc-20160816; b=ZIQ3ckS4UBvtFF7YwB18HYHO5lv42MPzLTrfS0tfrZr5k3irHaE5W9UFAomKeHw+e1 ORFnxnnqjVxz+5DYY3A9DkPKCbc3V6mwqrxfaf5KhYxsbUyU9BmkrU2IB/+QhGszS6Sk kFG49I1+TlxXrTDoQefMvep9f/jJ6gxV/FRnMBXC1ZcUv85UePQvu8ZxcFH+3Gim6jjg jWo0TV+p+FvRIaklgIbviuUqAYFxj5feqUUEFTbJwwfaTxlxOD+7mtmUFbgLNO/Tda99 CzWk9yJOZrpp2qQHEzCtoC7aFMM31jvCq5bAKgmxCBIBxwyzO235QM3T9C97EoJ5g+K5 wkXg== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:user-agent:in-reply-to:content-transfer-encoding :content-disposition:mime-version:references:message-id:subject:cc :to:from:date:ironport-sdr:ironport-sdr; bh=kvg/EaXck2de73JNgpYZC09zPSNpGcWXW84okOUnV3w=; b=FWRbqnSK1W3aQgBxGeOjz1XcMdMY4iePe2mTtj8vvRgmwAvLo9cNI6bVwfrX/Bj0mi U6xnpkbhtGpXvk6VZgK/Freplg9m+lFV5v+3Vv8iIAoehCX2u9uSPWOin+EU/Y74hSkC OCrt3noXPCfUNHNFyt1p0S+D/0/wlcLhFbwcjmgV/xH9Ro08ZI1mlPzp206cl0O53Vlj 7JJVemArbAD3AzM1y5m1QOXX/NXgjy8RrD1e8OYDEqyNbDsMt0J7d8f/xo1jirVDjwBI PruIgAyoa1QZy7odGUJ9KRE2d8IektPmCpZWnJArQs1umRlAyzwmMBqozDvOxRf13rRD BDbw== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=intel.com Return-Path: Received: from vger.kernel.org (vger.kernel.org. [23.128.96.18]) by mx.google.com with ESMTP id d184si3107231pga.273.2021.04.22.19.35.06; Thu, 22 Apr 2021 19:35:18 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18; Authentication-Results: mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=intel.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S237163AbhDWCa7 (ORCPT + 99 others); Thu, 22 Apr 2021 22:30:59 -0400 Received: from mga17.intel.com ([192.55.52.151]:2176 "EHLO mga17.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S236080AbhDWCa7 (ORCPT ); Thu, 22 Apr 2021 22:30:59 -0400 IronPort-SDR: nsBYqubn98nBXbSDggHD0NwsOVFwwe/EKhT/YtkUr7fD0x/4OAM0pPaGjBy5+qj6L8e1VI2ozL m7fjXIrWMV7Q== X-IronPort-AV: E=McAfee;i="6200,9189,9962"; a="176128408" X-IronPort-AV: E=Sophos;i="5.82,244,1613462400"; d="scan'208";a="176128408" Received: from orsmga002.jf.intel.com ([10.7.209.21]) by fmsmga107.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 22 Apr 2021 19:30:23 -0700 IronPort-SDR: 041zm7c8mdnKtAVNeJfVBZZKbigKrKUCxF9Wh+7IK0Le2mfrsZ8U1rGRSju71lU05xj7uEbej4 +PzfezIkGQ9A== X-IronPort-AV: E=Sophos;i="5.82,244,1613462400"; d="scan'208";a="402047000" Received: from xsang-optiplex-9020.sh.intel.com (HELO xsang-OptiPlex-9020) ([10.239.159.140]) by orsmga002-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 22 Apr 2021 19:30:06 -0700 Date: Fri, 23 Apr 2021 10:47:22 +0800 From: Oliver Sang To: "Eric W. Biederman" Cc: 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 , Alexey Gladkov , 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: <20210423024722.GA13968@xsang-OptiPlex-9020> References: <7abe5ab608c61fc2363ba458bea21cf9a4a64588.1617814298.git.gladkov.alexey@gmail.com> <20210408083026.GE1696@xsang-OptiPlex-9020> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: User-Agent: Mutt/1.9.4 (2018-02-28) Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org hi, Eric, On Thu, Apr 08, 2021 at 01:44:43PM -0500, Eric W. Biederman wrote: > Linus Torvalds writes: > > > 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_per_sec due to commit > >> 08ed4efad684 ("[PATCH v10 6/9] Reimplement RLIMIT_SIGPENDING on top of ucounts") > > > > Ouch. > > We were cautiously optimistic when no test problems showed up from > the last posting that there was nothing to look at here. > > Unfortunately it looks like the bots just missed the last posting. this report is upon v10. do you have newer version which hope bot test? 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. e.g. we didn't make out a similar performance regression for v8/v9 version of this one.. > > So it seems we are finally pretty much at correct code in need > of performance tuning. > > > I *think* this test may be testing "send so many signals that it > > triggers the signal queue overflow case". > > > > And I *think* that the performance degradation may be due to lots of > > unnecessary allocations, because ity looks like that commit changes > > __sigqueue_alloc() to do > > > > struct sigqueue *q = kmem_cache_alloc(sigqueue_cachep, flags); > > > > *before* checking the signal limit, and then if the signal limit was > > exceeded, it will just be free'd instead. > > > > The old code would check the signal count against RLIMIT_SIGPENDING > > *first*, and if there were m ore pending signals then it wouldn't do > > anything at all (including not incrementing that expensive atomic > > count). > > This is an interesting test in a lot of ways as it is testing the > synchronous signal delivery path caused by an exception. The test > is either executing *ptr = 0 (where ptr points to a read-only page) > or it executes an x86 instruction that is excessively long. > > 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); > > ret = sigsetjmp(jmp_env, 1); > if (done()) > break; > if (ret) { > /* verify signal */ > } else { > *ptr = 0; > } > } > > Code like that fundamentally can not be multi-threaded. So the only way > the sigpending limit is being hit is if there are more processes running > that code simultaneously than the size of the limit. > > Further it looks like stress-ng pushes RLIMIT_SIGPENDING as high as it > will go before the test starts. > > > > 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 are very > > expensive. > > > > The new code just does a lot of these atomics unconditionally. > > Yes. That seems a likely culprit. > > > I dunno. The profile data in there is a bit hard to read, but there's > > a lot more cachee misses, and a *lot* of node crossers: > > > >> 5961544 +190.4% 17314361 perf-stat.i.cache-misses > >> 22107466 +119.2% 48457656 perf-stat.i.cache-references > >> 163292 ą 3% +4582.0% 7645410 perf-stat.i.node-load-misses > >> 227388 ą 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.overall.cpi > >> 22849 -65.6% 7866 perf-stat.overall.cycles-between-cache-misses > > > > and it does seem to be at least partly about "put_ucounts()": > > > >> 0.00 +4.5 4.46 perf-profile.calltrace.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* much > > more expensive than it used to be. > > That too is possible. > > 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 is > running multiple copies of the sigsegv test in different processes at > once. > > > > That really suggests cache line ping pong from get_ucounts and > incrementing sigpending. > > 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 consistent > with a causing the exception delivery to take nearly twice as long. > > 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 thing > to do here. The fewer changes to worry about the less likely to find > surprises. > > > > 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 sigpending > in commit_cred and never worry about needing to pin the struct > user_struct or struct ucounts. As this is a synchronous signal we could > skip the sigpending increment, skip the signal queue entirely, and > deliver the signal to user-space immediately. The removal of all cache > ping pongs might make it worth it. > > 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. > > Eric