From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-yx1-f53.google.com (mail-yx1-f53.google.com [74.125.224.53]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 7F9F62820C6 for ; Thu, 18 Jun 2026 04:49:59 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=74.125.224.53 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1781758201; cv=none; b=ldeL6XLO3jyIQ9nD6t+8T47CeBKKC+ZOZLSnQY7HZeqynnJlL6yYtHRgDovmdSoz5TQxC4z4edGxzDbChCoazPkozootOstyvj5Xvd5Kh9X9+z7Nih4E6DNl1ULQgRAPGFa4GXFi3mjRfVq13+9a81lCkViEZzyM1m8trUifpVY= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1781758201; c=relaxed/simple; bh=dq6tE8nzS2xNAdFKISYFANgjJr3guq1W8/FKrYRtb+Q=; h=From:Date:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=ZkyPB/uSYIMyW+D234OHyzxmS3N3wMw6BtS2gqQCin4yNWpgrHdcfTiyGOecXYnWDBPhlLhHLVLwjK7iF/R8vUxlbTv4nYuQH40qqy7TWYWqUlcFtQCAT5MWekE7ytQQTsh2RV1byf3Kwh6S793LneJ6mkBNKk7TXvL9/ftZlVI= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com; spf=pass smtp.mailfrom=gmail.com; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b=j5lzOFKQ; arc=none smtp.client-ip=74.125.224.53 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=gmail.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="j5lzOFKQ" Received: by mail-yx1-f53.google.com with SMTP id 956f58d0204a3-66077e90382so496845d50.3 for ; Wed, 17 Jun 2026 21:49:59 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20251104; t=1781758198; x=1782362998; darn=vger.kernel.org; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:date:from:from:to:cc:subject:date:message-id:reply-to; bh=UNBy01GaEb8+T1/Y51J09nPpknKwTD/SfmCcM/pcFJM=; b=j5lzOFKQLurTRjTtLtPRsLUgCI68BF5UrihdsjSLFK+oLLI5DlfiUlORYEbT6nvCm9 BsGr+aAjH44UW6Dn2rWMgJdOrZrik1nqrIKPQBNdE5YIaBevgshJD8f0o0FPvhQOKE9M dvtSr77JLqP9Ah+qLdoeXGTn39rw2CTEcTbATekfZABT4GKwzFmiYfl8T62J0UBNa3Hx EorlVxqRxBcP5wtQI7wiXH5PT20GaWtSyzNwsqAq/Op4YBnkigp0qLQo5eieQiZvjhCS 4gVGAZCyS01qkrRQ2X6Cl6rxXreBjtJ4u64hm0UT9pvXkLiCfb4dBs/98m4G/lZYUlf2 eb5g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1781758198; x=1782362998; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:date:from:x-gm-gg:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=UNBy01GaEb8+T1/Y51J09nPpknKwTD/SfmCcM/pcFJM=; b=nPFh/g97NOvpugEA5jKFtsaS5OqteowSzs+omNTBhL1f7o311KB0U1m1uCvh2D2WIN zPRWq7M+qyc9LS4ayxJCS0+MxGQlptdXA1F4GFpJOBTCmuGgEwmDK5YqqrNEM8OAp1VY 1oMeoTPPFXJJWg2BCi6I3vaeZnyjQY8jbD44CabmH51N9dLjRUpO3XmQ6xZKMcSuw+rW 6eqHJcxQcKWVkZn3eGu5cNBruP6TiIG6ZNXdyteeNmiBMWP28zZpu8D/jH31hrhXK+pF m3mytNQLASLODz4HH9hLaNgX7355YpC+xCQq85e3UXjuledkqpXvBqe5SYD8Bqgr1Kb2 8EJQ== X-Forwarded-Encrypted: i=1; AFNElJ/MscDsz5B+jgGsREImGAIV1vO8rs7RlPuKeOmXuP+oPii6IM1LOCFRqE+8rfGzmmExzk9b2XhdVLezWQg=@vger.kernel.org X-Gm-Message-State: AOJu0YxM7mvBXtrIoQGWUERuW8A2wQ9SdcOnUAUUPXe7h9CZi1/CIpSU W3IbxaHu4Hd7dBlbcink/08pLP33ED6jviVzkwst4HMsqTKwfoaoOkUQ X-Gm-Gg: AfdE7cnfd1K6eRE6kOF3NtX2q+fadNBprX5dlfwNc8Y4kM8gscSMblZVa1W3j9hwLZJ O9U+CBoa+jqC9nYjDV9YjU7iPey+kpC8nyOdseQh/Mla4dZ3hSmp34+m/Ydj1aDFuUwOwgdD+ml ukS0dpw+5jdyBMFWIcujrXoC57U0qtFR5i4YiNPp7eOZKYMkCV+zWo2w0xdtJH2ekX9r6npHWCh 3cWso/lwWm7oRpS74IQ4TBrjwu3dw79VBFnRe7B+yJDpV9R2a3OTwxFhIqVWy38nhJC/zCdgw3G zGLer7KinafpLbQosBv1Q4M1yjXjqUCOoEYhMF5/JaIj6Y7g43tA6pa8I1lDLVqLksYKRhmIZvh HGUh3NJD13IhfFEqelJ1AeHvdvpBslD4VuxpXZWY9Smvu66dySvAkCye5a/FMSd4sTyDYriZwR9 JyEnHg3aZNnA9Q9ySbY1lBCvJHNM+4BM4k3e+BK3NVu387SQ== X-Received: by 2002:a05:690e:d02:b0:64c:f395:c19 with SMTP id 956f58d0204a3-662ee4a831emr825513d50.64.1781758198290; Wed, 17 Jun 2026 21:49:58 -0700 (PDT) Received: from localhost (syn-035-130-123-074.biz.spectrum.com. [35.130.123.74]) by smtp.gmail.com with ESMTPSA id 00721157ae682-7fd33743bc6sm55564337b3.18.2026.06.17.21.49.57 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 17 Jun 2026 21:49:57 -0700 (PDT) From: Yury Norov X-Google-Original-From: Yury Norov Date: Thu, 18 Jun 2026 00:49:56 -0400 To: Shrikanth Hegde Cc: Yury Norov , linux-kernel@vger.kernel.org, mingo@kernel.org, peterz@infradead.org, juri.lelli@redhat.com, vincent.guittot@linaro.org, kprateek.nayak@amd.com, iii@linux.ibm.com, tglx@kernel.org, gregkh@linuxfoundation.org, pbonzini@redhat.com, seanjc@google.com, vschneid@redhat.com, huschle@linux.ibm.com, rostedt@goodmis.org, dietmar.eggemann@arm.com, mgorman@suse.de, bsegall@google.com, maddy@linux.ibm.com, srikar@linux.ibm.com, hdanton@sina.com, chleroy@kernel.org, vineeth@bitbyteword.org, frederic@kernel.org, arighi@nvidia.com, pauld@redhat.com, christian.loehle@arm.com, tj@kernel.org, tommaso.cucinotta@gmail.com, maz@kernel.org, rafael@kernel.org Subject: Re: [PATCH v4 06/20] sched/core: allow only preferred CPUs in is_cpu_allowed Message-ID: References: <20260617174139.155540-1-sshegde@linux.ibm.com> <20260617174139.155540-7-sshegde@linux.ibm.com> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: On Thu, Jun 18, 2026 at 09:47:49AM +0530, Shrikanth Hegde wrote: > > > On 6/18/26 9:02 AM, Yury Norov wrote: > > On Wed, Jun 17, 2026 at 11:11:25PM +0530, Shrikanth Hegde wrote: > > > When possible, choose a preferred CPUs to pick. > > > > > > Push task mechanism uses stopper thread which going to call > > > select_fallback_rq and use this mechanism to pick only a preferred CPU. > > > > > > When task is affined only to non-preferred CPUs it should continue to > > > run there. Detect that by checking if cpus_ptr and cpu_preferred_mask > > > intersect or not. > > > > > > Since is_cpu_allowed can be called directly or repeatedly in > > > select_fallback_rq, encode the info in task_struct->has_preferred_cpu_state > > > if the path is via select_fallback_rq or not. > > > This helps to avoid N**2 complexity for the rare cases. > > > > > > Signed-off-by: Shrikanth Hegde > > > --- > > > v3->v4: > > > - Missing case of PF_KTHREAD is avoided. > > > - Add a new field in task_struct which encodes intersection of > > > tasks affinity and preferred CPUs and path its coming from. > > > > > > include/linux/sched.h | 1 + > > > kernel/sched/core.c | 34 ++++++++++++++++++++++++++++++++-- > > > kernel/sched/sched.h | 18 ++++++++++++++++++ > > > 3 files changed, 51 insertions(+), 2 deletions(-) > > > > > > diff --git a/include/linux/sched.h b/include/linux/sched.h > > > index fc6ecb3869dd..2d0b1a6d50ac 100644 > > > --- a/include/linux/sched.h > > > +++ b/include/linux/sched.h > > > @@ -1657,6 +1657,7 @@ struct task_struct { > > > #ifdef CONFIG_UNWIND_USER > > > struct unwind_task_info unwind_info; > > > #endif > > > + int has_preferred_cpu_state; > > > > Shouldn't this be protected with the config? > > Since preferred is defined always, i don;t see a reason to add it again here. > > > > > > /* CPU-specific state of this task: */ > > > struct thread_struct thread; > > > diff --git a/kernel/sched/core.c b/kernel/sched/core.c > > > index 9e16946c9d62..714816cfa975 100644 > > > --- a/kernel/sched/core.c > > > +++ b/kernel/sched/core.c > > > @@ -2500,6 +2500,8 @@ static inline bool rq_has_pinned_tasks(struct rq *rq) > > > */ > > > static inline bool is_cpu_allowed(struct task_struct *p, int cpu) > > > { > > > + bool task_check_preferred_cpu = false; > > > > Initialization is not needed. > > ok > > > > > > + > > > /* When not in the task's cpumask, no point in looking further. */ > > > if (!task_allowed_on_cpu(p, cpu)) > > > return false; > > > @@ -2508,9 +2510,22 @@ static inline bool is_cpu_allowed(struct task_struct *p, int cpu) > > > if (is_migration_disabled(p)) > > > return cpu_online(cpu); > > > + /* > > > + * This is essential to maintain user affinities when preferred > > > + * CPUs change. A task pinned on non-preferred CPU should continue > > > + * to run there, since this is non-user triggered. > > > + * > > > + * If CPU is non-preferred and task can run on other CPUs which are > > > + * currently preferred, then choose those other CPUs instead > > > + */ > > > + task_check_preferred_cpu = !cpu_preferred(cpu) && task_has_preferred_cpus(p); > > > + > > > /* Non kernel threads are not allowed during either online or offline. */ > > > - if (!(p->flags & PF_KTHREAD)) > > > + if (!(p->flags & PF_KTHREAD)) { > > > + if (task_check_preferred_cpu) > > > + return false; > > > return cpu_active(cpu); > > > + } > > > /* KTHREAD_IS_PER_CPU is always allowed. */ > > > if (kthread_is_per_cpu(p)) > > > @@ -2520,6 +2535,10 @@ static inline bool is_cpu_allowed(struct task_struct *p, int cpu) > > > if (cpu_dying(cpu)) > > > return false; > > > + /* Try on preferred CPU first if possible*/ > > > + if (task_check_preferred_cpu) > > > + return false; > > > + > > > /* But are allowed during online. */ > > > return cpu_online(cpu); > > > } > > > @@ -3549,6 +3568,14 @@ static int select_fallback_rq(int cpu, struct task_struct *p) > > > enum { cpuset, possible, fail } state = cpuset; > > > int dest_cpu; > > > + /* > > > + * Cache value whether task's affinity spans preferred CPUs. > > > > Because it's cached, it should go inside is_cpu_allowed(), I think. > > > > > + * This helps to avoid repeating the same for each CPU > > > + * later in the loop. Encode call to is_cpu_allowed coming > > > + * via select_fallback_rq. > > > + */ > > > + p->has_preferred_cpu_state = task_has_preferred_cpus(p) << 8 | 0x1; > > > > This looks weird. Your intention is to store three states: not cached, has > > preferred CPUs and has not preferred CPUs, > > > > Why don't you create an enum for it? Or a couple of flags? > > I think what prateek suggested in other thread looks same. I will give that a try. > > > > > > + > > > /* > > > * If the node that the CPU is on has been offlined, cpu_to_node() > > > * will return -1. There is no CPU on the node, and we should > > > @@ -3560,7 +3587,7 @@ static int select_fallback_rq(int cpu, struct task_struct *p) > > > /* Look for allowed, online CPU in same node. */ > > > for_each_cpu(dest_cpu, nodemask) { > > > if (is_cpu_allowed(p, dest_cpu)) > > > - return dest_cpu; > > > + goto clear_and_return; > > > } > > > } > > > @@ -3604,6 +3631,8 @@ static int select_fallback_rq(int cpu, struct task_struct *p) > > > } > > > } > > > +clear_and_return: > > > + p->has_preferred_cpu_state = 0; > > > > It is reset to indicate that any subsequent direct calls to is_cpu_allowed can't use the > old cached value of select_fallback_rq. > > So events could be, > > - cpu marked as non preferred - select_fallback_rq (sets the p->has_preferred_cpu_state) > Lets say CPU(300-450) are marked as non-preferred and Task affinity is (200-350) > - task moved out. Now either task's affinity changed or preferred_mask has changed. > while CPU(400) maybe still marked as non-preferred but CPU(340) is marked as preferred. > - Subsequent call to is_cpu_allowed (CPU=340) can't assume the old value. Please, no top-posting. My point is: out of the scope of the select_fallback_rq(), the p->has_preferred_cpu_state is always 0 because of the line above. It means, it doesn't belong to the task_struct, it belongs the current scope. So either make this caching really surviving the scope exit, or make it a local variable. Not sure I understood the passage above about the possible events, but variables that are always zero out of the function scope should not be placed in global structures. > > What for resetting it here? I think it should be zeroed only on update > > of preferred cpumask. In other words, to properly implement caching, > > you need to have a global counter incremented on each > > cpu_preferred_mask update, and in task_has_preferred_cpus() you do: > > > > { > > if (p->preferred_cpu_updates == atomic_read(preferred_cpumask_updates)) > > return p->has_preferred_cpus; > > > > p->preferred_cpu_updates = atomic_read(preferred_cpumask_updates); > > p->has_preferred_cpus = cpumask_intersects(...); > > } > > > > Do you have any numbers that justify this caching? The best practice > > is to put performance optimizations at the end of the series and > > provide some sort of benchmark supporting it. > > > > This was to avoid N**2 aspect that was there in select_fallback_rq. > Its more of the functional aspect which i mentioned above which this needs > to take care as well. Please, collect the performance data first, then optimize your code, not vice-versa. > > > return dest_cpu; > > > } > > > @@ -4612,6 +4641,7 @@ static void __sched_fork(u64 clone_flags, struct task_struct *p) > > > init_numa_balancing(clone_flags, p); > > > p->wake_entry.u_flags = CSD_TYPE_TTWU; > > > p->migration_pending = NULL; > > > + p->has_preferred_cpu_state = 0; > > > init_sched_mm(p); > > > } > > > diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h > > > index c7c2dea65edd..38fd84b0b8f8 100644 > > > --- a/kernel/sched/sched.h > > > +++ b/kernel/sched/sched.h > > > @@ -4213,4 +4213,22 @@ DEFINE_CLASS_IS_UNCONDITIONAL(sched_change) > > > #include "ext.h" > > > +/* > > > + * has_preferred_cpu_state is encoding two bits of information. > > > + * First Byte is to encode where the call to is_cpu_allowed coming from. > > > + * Second Byte is to encode the intersection of task affinity > > > + * and cpu_preferred_mask. > > > + * > > > + * If 1st Byte is set, call to is_cpu_allowed coming from select_fallback_rq. > > > + * That helps to avoid repeated calculation keeping time complexity same. > > > + */ > > > +static inline bool task_has_preferred_cpus(struct task_struct *p) > > > > This function should be void because you change the task state. > > > > It doesn't alter p->has_preferred_cpu_state. No? It doesn't, but it should. > > > +{ > > > + int cached_value = p->has_preferred_cpu_state; > > > + > > > + if (cached_value & 0x1) > > > + return p->has_preferred_cpu_state >> 8; > > > + else > > > + return cpumask_intersects(p->cpus_ptr, cpu_preferred_mask); > > > +} > > > #endif /* _KERNEL_SCHED_SCHED_H */ > > > -- > > > 2.47.3