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 Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 1DFF0C77B7A for ; Fri, 19 May 2023 21:42:22 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=DYIU1gXVuNbmyL7QhoezNmbJt2G2hdezE2fnq60hKqo=; b=tPJNLgUDCoWJBu zt6QiGv3Xp0OYjmSXcMM5HMu93a/EeRRVaKChIxnU73v0LUqtDkpV4okGF4nq//aLeRJH5A9IUvmH Gf528OZNZnBO1YAuzdbopKPpr5WwpRG/zVDZNWR7PE/ZV5zxApBAw7rEl6cEZPAHOglC1o05gZkcO VpiNsBhlqi2x133MTRVN0wW7j2PjqeQE6yc8e/ATOqEtj2he0tjqJby1UaURYQDMDps6NUtzlg3KD 1cuDCfcUYykiwgbgOna+cmGl19Eiy/7JSB7nmLjL9VrqsRpS9V9eY0TONLrAhZXXWWwWhSNIfE/84 M4j0gXyBMlqXP7pBBeZg==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1q07rH-00HIwa-1c; Fri, 19 May 2023 21:41:55 +0000 Received: from mga02.intel.com ([134.134.136.20]) by bombadil.infradead.org with esmtps (Exim 4.96 #2 (Red Hat Linux)) id 1q07rC-00HIuN-24 for linux-arm-kernel@lists.infradead.org; Fri, 19 May 2023 21:41:52 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1684532510; x=1716068510; h=date:from:to:cc:subject:message-id:references: mime-version:content-transfer-encoding:in-reply-to; bh=YAFT8fMrl0VSOoxAls3ESDXviySfbeFgHcFQlHgN72Q=; b=cS0+PCIDeYfobcxhWfiuBVF7+rSELYy+vPyNUYJHFfb7oiwYLZmaWls/ wvVzKzCLPZP+HltmEh5qtEHlQHeAVTQKCMfRikky218Kp0+Z50Pj0iPnR FYyjIC7XRs19xViPMBeB/ztzeRBv8kc+V3hTrJiGtudDU+Ma2LVj8AGOz fN4+3oXQvHVHjy5rJ50sozBlPkf4pzLqaYw97ntEyPYjT95aTbg5fRXgI 9p9TyFmXOUpIRYewcFg/AtKWACJjyPR46BUmrJ+PPbCAw32n1wunGjCmg DwkSKBRxMkyhpaionIlgqxcpy7KA4rMClsisTsHKxD7G1kIYpk5dkARKn g==; X-IronPort-AV: E=McAfee;i="6600,9927,10715"; a="341938794" X-IronPort-AV: E=Sophos;i="6.00,177,1681196400"; d="scan'208";a="341938794" Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by orsmga101.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 19 May 2023 14:41:33 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6600,9927,10715"; a="814876993" X-IronPort-AV: E=Sophos;i="6.00,178,1681196400"; d="scan'208";a="814876993" Received: from ranerica-svr.sc.intel.com ([172.25.110.23]) by fmsmga002.fm.intel.com with ESMTP; 19 May 2023 14:41:32 -0700 Date: Fri, 19 May 2023 14:44:30 -0700 From: Ricardo Neri To: Radu Rendec Cc: linux-kernel@vger.kernel.org, Catalin Marinas , Will Deacon , Pierre Gondois , Sudeep Holla , linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH v4 1/3] cacheinfo: Add arch specific early level initializer Message-ID: <20230519214430.GA3116@ranerica-svr.sc.intel.com> References: <20230412185759.755408-1-rrendec@redhat.com> <20230412185759.755408-2-rrendec@redhat.com> <20230510191207.GA18514@ranerica-svr.sc.intel.com> <20230511000058.GD18514@ranerica-svr.sc.intel.com> <9020807789b70db0d84d142cbfed2bd8868f366a.camel@redhat.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <9020807789b70db0d84d142cbfed2bd8868f366a.camel@redhat.com> User-Agent: Mutt/1.9.4 (2018-02-28) X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20230519_144150_745100_F5D13A77 X-CRM114-Status: GOOD ( 65.69 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Thu, May 11, 2023 at 03:55:18PM -0400, Radu Rendec wrote: > On Wed, 2023-05-10 at 17:00 -0700, Ricardo Neri wrote: > > On Wed, May 10, 2023 at 04:44:49PM -0400, Radu Rendec wrote: > > > On Wed, 2023-05-10 at 12:12 -0700, Ricardo Neri wrote: > > > > On Wed, Apr 12, 2023 at 02:57:57PM -0400, Radu Rendec wrote: > > > > > This patch gives architecture specific code the ability to initia= lize > > > > > the cache level and allocate cacheinfo memory early, when cache l= evel > > > > > initialization runs on the primary CPU for all possible CPUs. > > > [cut] > > > > > -int detect_cache_attributes(unsigned int cpu) > > > > > +static inline int init_level_allocate_ci(unsigned int cpu) > > > > > =A0{ > > > > > -=A0=A0=A0=A0=A0=A0=A0int ret; > > > > > +=A0=A0=A0=A0=A0=A0=A0unsigned int early_leaves =3D cache_leaves(= cpu); > > > > > =A0 > > > > > =A0=A0=A0=A0=A0=A0=A0=A0/* Since early initialization/allocation = of the cacheinfo is allowed > > > > > =A0=A0=A0=A0=A0=A0=A0=A0 * via fetch_cache_info() and this also g= ets called as CPU hotplug > > > > > =A0=A0=A0=A0=A0=A0=A0=A0 * callbacks via cacheinfo_cpu_online, th= e init/alloc can be skipped > > > > > =A0=A0=A0=A0=A0=A0=A0=A0 * as it will happen only once (the cache= info memory is never freed). > > > > > -=A0=A0=A0=A0=A0=A0=A0 * Just populate the cacheinfo. > > > > > +=A0=A0=A0=A0=A0=A0=A0 * Just populate the cacheinfo. However, if= the cacheinfo has been > > > > > +=A0=A0=A0=A0=A0=A0=A0 * allocated early through the arch-specifi= c early_cache_level() call, > > > > > +=A0=A0=A0=A0=A0=A0=A0 * there is a chance the info is wrong (thi= s can happen on arm64). In > > > > > +=A0=A0=A0=A0=A0=A0=A0 * that case, call init_cache_level() anywa= y to give the arch-specific > > > > > +=A0=A0=A0=A0=A0=A0=A0 * code a chance to make things right. > > > > > =A0=A0=A0=A0=A0=A0=A0=A0 */ > > > > > -=A0=A0=A0=A0=A0=A0=A0if (per_cpu_cacheinfo(cpu)) > > > > > -=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0goto populate_leave= s; > > > > > +=A0=A0=A0=A0=A0=A0=A0if (per_cpu_cacheinfo(cpu) && !ci_cacheinfo= (cpu)->early_ci_levels) > > > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0return 0; > > > > > =A0 > > > > > =A0=A0=A0=A0=A0=A0=A0=A0if (init_cache_level(cpu) || !cache_leave= s(cpu)) > > > > > =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0return -ENOENT; > > > > > =A0 > > > > > -=A0=A0=A0=A0=A0=A0=A0ret =3D allocate_cache_info(cpu); > > > > > +=A0=A0=A0=A0=A0=A0=A0/* > > > > > +=A0=A0=A0=A0=A0=A0=A0 * Now that we have properly initialized th= e cache level info, make > > > > > +=A0=A0=A0=A0=A0=A0=A0 * sure we don't try to do that again the n= ext time we are called > > > > > +=A0=A0=A0=A0=A0=A0=A0 * (e.g. as CPU hotplug callbacks). > > > > > +=A0=A0=A0=A0=A0=A0=A0 */ > > > > > +=A0=A0=A0=A0=A0=A0=A0ci_cacheinfo(cpu)->early_ci_levels =3D fals= e; > > > > > + > > > > > +=A0=A0=A0=A0=A0=A0=A0if (cache_leaves(cpu) <=3D early_leaves) > > > > > +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0return 0; > > > > > + > > > > = > > > > I had posted a patchset[1] for x86 that initializes > > > > ci_cacheinfo(cpu)->num_leaves during SMP boot. > > > > = > > > > This means that early_leaves and a late cache_leaves() are equal but > > > > per_cpu_cacheinfo(cpu) is never allocated. Currently, x86 does not = use > > > > fetch_cache_info(). > > > > = > > > > I think that we should check here that per_cpu_cacheinfo() has been= allocated to > > > > take care of the case in which early and late cache leaves remain t= he same: > > > > = > > > > -=A0=A0=A0=A0=A0=A0 if (cache_leaves(cpu) <=3D early_leaves) > > > > +=A0=A0=A0=A0=A0=A0 if (cache_leaves(cpu) <=3D early_leaves && per_= cpu_cacheinfo(cpu)) > > > > = > > > > Otherwise, in v6.4-rc1 + [1] I observe a NULL pointer dereference f= rom > > > > last_level_cache_is_valid(). > > > > = > > > > I can post a patch with this fix if it makes sense. > > > > = > > > > [1]. https://lore.kernel.org/all/20230424001956.21434-3-ricardo.ner= i-calderon@linux.intel.com/ > > > = > > > Thanks for bringing this to my attention. I need to run some tests on > > > x86 (I did all that work/testing on arm64) and wrap my head around it. > > > = > > > While I don't see any problem with the fix you're proposing, I'm afra= id > > > it may circle back to the other problem I tried to fix initially. Have > > > you tested this on an RT kernel by any chance? > > = > > That is a good point. I did not test on an RT kernel. I'll try that. > = > It looks like the flow is much simpler on x86: detect_cache_attributes() > is called only once for each CPU, and it's called in kthread context. > = > I haven't tested on an RT kernel but I think it should be fine. I put a > msleep() there and saw no issues, which means kmalloc() on RT should be > fine as well. I booted the realtime kernel [3] with CONFIG_PREEMPT_RT and did not observe the BUG splat. I tried before your patchset. Were you able to reproduce on x86? Also, I was not able to reproduce the BUG splat after your changes + [1] + my earlier suggested patch in this thread. > = > > > I'm thinking that if we end up in init_level_allocate_ci() without the > > > cacheinfo memory having been allocated earlier, we're up for a "BUG" > > > splat on RT kernels. > > > = > > > If early_leaves has the right value at that point, the cacheinfo memo= ry > > > should be allocated early (on the primary CPU), so perhaps there's a > > > different problem somewhere else. > > = > > That can work for x86, IMO. Not sure about other archs. As you mention, > > other archs still want the chance to correct the early cache info. > = > You're right. I got confused for a moment because I was used to the > arm64 flow. On x86, there is no "early" cache info per se because, as I > already mentioned, detect_cache_attributes() is called only once for > each CPU. Indeed. > = > I was intrigued about how this worked without your changes, and I > looked closer. Between the initialization of the early_leaves variable > at the beginning of init_level_allocate_ci() and the comparison of > cache_leaves(cpu) and early_leaves, init_cache_level() gets called. > Before your changes, (struct cpu_cacheinfo).num_leaves was initialized > to 0 and then changed in init_cache_level(). That way, early_leaves > ended up as 0, which made the comparison evaluate to false. Yes my changes aim to use (struct cpu_cacheinfo).num_leaves directly. > = > At this point I think the patch you proposed is the right way to fix > this. I don't see any reason why it would interfere with other archs > that really use early allocation. This new patch should probably be > added to your series, since otherwise your other patches would > basically "introduce" a null-pointer deref. > = > My only suggestion would be to add a short comment before the > comparison, to explain that on x86 detect_cache_attributes() is called > only once for each CPU and so early allocation is not possible but > (struct cpu_cacheinfo).num_leaves is already initialized by the time > detect_cache_attributes() is called. Thank you very much for your help! BR, Ricardo _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel