From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mgamail.intel.com (mgamail.intel.com [198.175.65.9]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 53328145B16 for ; Tue, 16 Dec 2025 19:53:11 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=198.175.65.9 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1765914794; cv=none; b=rqJa3OJF+J0Xfjx1X2wRTSlbQlivFqHl4Cf1drEWSmrypaqJaJ6TK5yLyLsR9riHMiBe4D3Ou8QFQyUq8LfGuojcrZ8+lex9lwYn1qcgaQ7Ycuc7NbeOopaxbB9/Ios4nSkiYmBEWC01RuyF439CeATaksXvfFNLwPa4gRXWLyc= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1765914794; c=relaxed/simple; bh=C7FqzAZonOj1RT9/OGiri+EoZWfxiAyWbs/fs55igf8=; h=Message-ID:Subject:From:To:Cc:Date:In-Reply-To:References: Content-Type:MIME-Version; b=gV0LRpddvz+JGtK/yJWWpY+W16baBmuKeB3IDiZNjB5NGd38A6L4bW1wCYQkFe/KG2CFKsqX9dPd8aK4mD3vUznDI/GnwMyjWJrPQdF5ABXr+y4ojO+r90O6DU50kyOejmStcu69+QusjKKy1ewfPXg+TbeQSiTjOonm66JHJXE= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linux.intel.com; spf=pass smtp.mailfrom=linux.intel.com; dkim=pass (2048-bit key) header.d=intel.com header.i=@intel.com header.b=ixvgZtrS; arc=none smtp.client-ip=198.175.65.9 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linux.intel.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=linux.intel.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=intel.com header.i=@intel.com header.b="ixvgZtrS" DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1765914791; x=1797450791; h=message-id:subject:from:to:cc:date:in-reply-to: references:content-transfer-encoding:mime-version; bh=C7FqzAZonOj1RT9/OGiri+EoZWfxiAyWbs/fs55igf8=; b=ixvgZtrSfPmI4eXLF0fpyoo+p8j4YmeBVT/OE92ra+YW4FNpAyyx91Fq pZKpuId3gy0YHCy5Kj4+ohIUC4dTeNG8P/mOyg0PHbVUtG3KZlMZSyOjY 69GWT+Y1alktChfJSw2QpKzIHYx5TAyC1esQOpDUqxGcXGp/0DIcikBQR l3IWNyKjh6T/iTIxaWJ2qZl2F4zZvGWcerqOjGqJDum8npaOu39+2ByBx OIDdxslEhk0qPCY7brlwMEFMgEEWho2rIhbJH2wS+PzUDHAiT/TXzLuJh jg6pypAlaainYRKAV4PQxOFxT23xZmntck5nMHKlVemWLkPscr5NsWUP+ g==; X-CSE-ConnectionGUID: M0RZuVv1TKmkGGuhJrZX7w== X-CSE-MsgGUID: ZdD8I/cJSCe2jYN4cD0xnQ== X-IronPort-AV: E=McAfee;i="6800,10657,11644"; a="90502782" X-IronPort-AV: E=Sophos;i="6.21,153,1763452800"; d="scan'208";a="90502782" Received: from fmviesa004.fm.intel.com ([10.60.135.144]) by orvoesa101.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 16 Dec 2025 11:53:08 -0800 X-CSE-ConnectionGUID: xR+pOP6FQVecWOhm1iK5Jw== X-CSE-MsgGUID: lrEI8N1mTDqV45VXqw0fLQ== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="6.21,153,1763452800"; d="scan'208";a="203021976" Received: from schen9-mobl4.amr.corp.intel.com (HELO [10.125.110.205]) ([10.125.110.205]) by fmviesa004-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 16 Dec 2025 11:53:06 -0800 Message-ID: <09a6d7832003733acbdf6bcc5f6f32b55a0caba4.camel@linux.intel.com> Subject: Re: [PATCH v2 04/23] sched/cache: Make LLC id continuous From: Tim Chen To: "Chen, Yu C" , Peter Zijlstra Cc: Ingo Molnar , K Prateek Nayak , "Gautham R . Shenoy" , Vincent Guittot , Juri Lelli , Dietmar Eggemann , Steven Rostedt , Ben Segall , Mel Gorman , Valentin Schneider , Madadi Vineeth Reddy , Hillf Danton , Shrikanth Hegde , Jianyong Wu , Yangyu Chen , Tingyin Duan , Vern Hao , Vern Hao , Len Brown , Aubrey Li , Zhao Liu , Chen Yu , Adam Li , Aaron Lu , Tim Chen , linux-kernel@vger.kernel.org Date: Tue, 16 Dec 2025 11:53:05 -0800 In-Reply-To: References: <20251209115846.GL3707891@noisy.programming.kicks-ass.net> <3bb9bf31222af552ee4b0db30bb1624bd84d3cba.camel@linux.intel.com> Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable User-Agent: Evolution 3.58.1 (3.58.1-1.fc43) Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 On Tue, 2025-12-16 at 13:31 +0800, Chen, Yu C wrote: > On 12/16/2025 4:49 AM, Tim Chen wrote: > > On Tue, 2025-12-09 at 12:58 +0100, Peter Zijlstra wrote: > > > On Wed, Dec 03, 2025 at 03:07:23PM -0800, Tim Chen wrote: > > >=20 > > > > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c > > > > index 710ed9943d27..0a3918269906 100644 > > > > --- a/kernel/sched/fair.c > > > > +++ b/kernel/sched/fair.c > > > > @@ -1210,10 +1210,17 @@ __read_mostly unsigned int llc_imb_pct = =3D 20; > > > > =20 > > > > static int llc_id(int cpu) > > > > { > > > > + int llc; > > > > + > > > > if (cpu < 0) > > > > return -1; > > > > =20 > > > > + llc =3D per_cpu(sd_llc_id, cpu); > > > > + /* avoid race with cpu hotplug */ > > > > + if (unlikely(llc >=3D max_llcs)) > > > > + return -1; > > > > + > > > > + return llc; > > > > } > > > > =20 > > > > void mm_init_sched(struct mm_struct *mm, struct mm_sched __percpu= *_pcpu_sched) > > >=20 > > > > @@ -668,6 +670,55 @@ DEFINE_PER_CPU(struct sched_domain __rcu *, sd= _asym_cpucapacity); > > > > DEFINE_STATIC_KEY_FALSE(sched_asym_cpucapacity); > > > > DEFINE_STATIC_KEY_FALSE(sched_cluster_active); > > > > =20 > > > > +/* > > > > + * Assign continuous llc id for the CPU, and return > > > > + * the assigned llc id. > > > > + */ > > > > +static int update_llc_id(struct sched_domain *sd, > > > > + int cpu) > > > > +{ > > > > + int id =3D per_cpu(sd_llc_id, cpu), i; > > > > + > > > > + if (id >=3D 0) > > > > + return id; > > > > + > > > > + if (sd) { > > > > + /* Look for any assigned id and reuse it.*/ > > > > + for_each_cpu(i, sched_domain_span(sd)) { > > > > + id =3D per_cpu(sd_llc_id, i); > > > > + > > > > + if (id >=3D 0) { > > > > + per_cpu(sd_llc_id, cpu) =3D id; > > > > + return id; > > > > + } > > > > + } > > > > + } > > > > + > > > > + /* > > > > + * When 1. there is no id assigned to this LLC domain, > > > > + * or 2. the sd is NULL, we reach here. > > > > + * Consider the following scenario, > > > > + * CPU0~CPU95 are in the node0, CPU96~CPU191 are > > > > + * in the node1. During bootup, maxcpus=3D96 is > > > > + * appended. > > > > + * case 1: When running cpu_attach_domain(CPU24) > > > > + * during boot up, CPU24 is the first CPU in its > > > > + * non-NULL LLC domain. However, > > > > + * its corresponding llc id has not been assigned yet. > > > > + * > > > > + * case 2: After boot up, the CPU100 is brought up > > > > + * via sysfs manually. As a result, CPU100 has only a > > > > + * Numa domain attached, because CPU100 is the only CPU > > > > + * of a sched domain, all its bottom domains are degenerated. > > > > + * The LLC domain pointer sd is NULL for CPU100. > > > > + * > > > > + * For both cases, we want to increase the number of LLCs. > > > > + */ > > > > + per_cpu(sd_llc_id, cpu) =3D max_llcs++; > > > > + > > > > + return per_cpu(sd_llc_id, cpu); > > > > +} > > >=20 > > > I'm not sure I follow. So partition_sched_domains() first calls > > > detach_destroy_domains() on the old set, and then build_sched_domains= () > > > on the new set. > > >=20 > > > Do detach_destroy_domain() will do: > > >=20 > > > cpu_attach_domain(NULL,..); > > >=20 > > > That is, it will explicitly attach the NULL sched_domain to a CPU. At > > > which point I feel update_llc_id() should be returning -1, no? > > >=20 > > > Then later, build_sched_domains() will set a !NULL sched_domain, at > > > which point update_llc_id() can set a real value. > > >=20 > > > This should then also get rid of that weird max_llcs check in llc_id(= ), > > > right? >=20 > The check for max_llcs was intended to prevent out-of-bounds access > to rq->nr_pref_llc[] at multiple points in the code. > Since dst_llc =3D llc_id(env->dst_cpu); =E2=80=94 and while the LLC ID fo= r the > CPU is updated in update_llc_id(), this update occurs before we realloc= ate > the nr_pref_llc buffer =E2=80=94 dst_llc may end up exceeding the bound= s of the > original nr_pref_llc buffer. >=20 > For this reason, we added a check if (dst_llc > max_llc) in llc_id() > when attempting to access rq->nr_pref_llc[dst_llc]. >=20 > However, I agree that the max_llc check seems to not properly integrated > into the current patch: it should instead be placed in the 7th patch, as > this would better illustrate the rationale for the max_llc check here: > sched/cache: Introduce per runqueue task LLC preference counter >=20 > In the 7th patch, we actually increment new_max_llcs rather than > max_llcs =E2=80=94 meaning max_llcs always represents the "old" number of= LLCs. > As a result, there is a race window between extending the rq->nr_pref_llc > buffer and updating max_llcs. >=20 >=20 > @@ -714,7 +827,7 @@ static int update_llc_id(struct sched_domain *sd, > * > * For both cases, we want to increase the number of LLCs. > */ > - per_cpu(sd_llc_id, cpu) =3D max_llcs++; > + per_cpu(sd_llc_id, cpu) =3D new_max_llcs++; >=20 > return per_cpu(sd_llc_id, cpu); > } >=20 >=20 > > Thanks for pointing this out. Yes, we should take care of the > > attachment of NULL sd. Will update the code accordingly. > >=20 >=20 > My understanding is that, if the sd is NULL, it is either because invoked > by detach_destroy_domain() for the old set, or by case 2 mentioned in=20 > above comments: > Say, CPU0-CPU95 are online during bootup, the boot command line is=20 > maxcpus=3D96. > Later after bootup, the user wants to bring up CPU100, the LLC domain for > CPU100 is NULL in this case(due to sd generation), and a new LLC should b= e > detected. >=20 > That is to say, when we reach update_llc_id(), there could be 2 reasons > for NULL sd. For the detach_destroy_domain() case, update_llc_id() > should return a valid id without increasing the max_llcs, because of > if (id >=3D 0) > return id; > And for the latter, the max_llcs should be increased. > Let me double check on this. The issue is we could offline all CPUs in a LLC and online them later. In the current code, we will assign their ids all to -1. So on attach of CPUs again, we'll be assigning a new LLC. I think the proper thing to do is not to assign llc id of the offlined cpu (the case where sd =3D=3D= NULL) and keep the original llc id assigned. Then we should be okay and not increase max_llcs. Tim >=20 > thanks, > Chenyu >=20 >=20 > > Tim