mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
From: Tim Chen <tim.c.chen@linux.intel.com>
To: "Chen, Yu C" <yu.c.chen@intel.com>, Hui Su <sh_def@163.com>
Cc: K Prateek Nayak <kprateek.nayak@amd.com>,
	Juri Lelli <juri.lelli@redhat.com>,
	 Vincent Guittot <vincent.guittot@linaro.org>,
	Dietmar Eggemann <dietmar.eggemann@arm.com>,
	Valentin Schneider	 <vschneid@redhat.com>,
	John Stultz <jstultz@google.com>, Ingo Molnar	 <mingo@redhat.com>,
	Peter Zijlstra <peterz@infradead.org>,
	 linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 1/2] sched/numa: Drive NUMA task tick from execution context
Date: Tue, 08 Sep 2026 09:01:16 -0700	[thread overview]
Message-ID: <e91848b651fbe163676813fffff76970d6895cf9.camel@linux.intel.com> (raw)
In-Reply-To: <57833d3b-bf10-42b9-ae28-82d65a22a6af@intel.com>

On Tue, 2026-09-08 at 13:38 +0800, Chen, Yu C wrote:
> On 9/5/2026 9:58 PM, Hui Su wrote:
> > On Fri, Sep 4, 2026 at 1:15 PM, Tim Chen wrote:
> > > Yes, you have a good point. The code I proposed just look at whether
> > > we have consumed our allotment in the current slice.
> > > 
> > > What we should have looked at is whether the donor's total run time has
> > > exceeded its quota when doing core scheduling. And we may happen to
> > > hit __entity_slice_used() at the front of the slice after advancing
> > > the deadline and __entity_slice_used()
> > > returns false instead of true, even though I have consumed more than
> > > my fair share when looking at longer time period across multiple slices.
> > > 
> > > The accumulated run time of the donor since it was picked for running
> > > should be used for selection time baseline.
> > > 
> > > So maybe a patch like the following instead.
> > 
> 
> [ ... ]
> 
> > 
> > In this example reweight_eevdf() did not change se->vruntime, so the
> > divergence does not depend on a vruntime coordinate adjustment. The
> > weight change alone is enough for the accumulated vused and the
> > current-weight vslice to no longer necessarily use the same scale.
> > 
> 
> Makes sense. In the current kernel with a flat cgroup(commit 85570f10a4c6
> ("sched/eevdf: Move to a single runqueue")), task_tick_fair()
> tries to re-calculate the task's h_load.weight - if the cgroup changes
> its share at runtime, the task se's h_load.weight changes accordingly.
> Thus comparing the delta derived from the snapshot vruntime and the
> slice using the latest weight is unreliable. (While before the flat cgroup
> was introduced, a task's se->load.weight will not be re-evaluated during 
> the tick, even if the cgroup changes its share at runtime.
> > diff --git a/include/linux/sched.h b/include/linux/sched.h
> > index 8b3d47a325cc..c32d9931129f 100644
> > --- a/include/linux/sched.h
> > +++ b/include/linux/sched.h
> > @@ -590,6 +590,9 @@ struct sched_entity {
> >   	u64				sum_exec_runtime;
> >   	u64				prev_sum_exec_runtime;
> >   	u64				vruntime;
> > +#ifdef CONFIG_SCHED_CORE
> > +	u64				core_sched_start;
> > +#endif
> >   	/* Approximated virtual lag: */
> >   	s64				vlag;
> >   	/* 'Protected' deadline, to give out minimum quantums: */
> > diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> > index f78275192036..22ae5dc57337 100644
> > --- a/kernel/sched/core.c
> > +++ b/kernel/sched/core.c
> > @@ -4580,6 +4580,9 @@ static void __sched_fork(u64 clone_flags, struct task_struct *p)
> >   	p->se.prev_sum_exec_runtime	= 0;
> >   	p->se.nr_migrations		= 0;
> >   	p->se.vruntime			= 0;
> > +#ifdef CONFIG_SCHED_CORE
> > +	p->se.core_sched_start		= 0;
> > +#endif
> >   	p->se.vlag			= 0;
> >   	p->se.rel_deadline		= 0;
> >   	INIT_LIST_HEAD(&p->se.group_node);
> > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> > index 8dff37059faf..1bd05c906d3a 100644
> > --- a/kernel/sched/fair.c
> > +++ b/kernel/sched/fair.c
> > @@ -6502,6 +6502,9 @@ set_next_entity(struct cfs_rq *cfs_rq, struct sched_entity *se)
> >   	}
> >   
> >   	se->prev_sum_exec_runtime = se->sum_exec_runtime;
> > +#ifdef CONFIG_SCHED_CORE
> > +	se->core_sched_start = se->exec_start;
> 
> if (entity_is_task(se)) ?
> 
> > +#endif
> >   }
> >   
> >   static bool __dequeue_task(struct rq *rq, struct task_struct *p, int flags);
> > @@ -14748,10 +14751,9 @@ static void rq_offline_fair(struct rq *rq)
> >   static inline bool
> >   __entity_slice_used(struct sched_entity *se, int min_nr_tasks)
> >   {
> > -	u64 rtime = se->sum_exec_runtime - se->prev_sum_exec_runtime;
> > -	u64 slice = se->slice;
> > +	u64 rtime = se->exec_start - se->core_sched_start;
> > 
> > -	return (rtime * min_nr_tasks > slice);
> > +	return (rtime * min_nr_tasks > se->slice);
> >   }
> >   
> >   #define MIN_NR_TASKS_DURING_FORCEIDLE	2
> > 
> > The task-clock version has matched the existing predicate in the
> > non-proxy tests so far and fixes the proxy reproducer as well. I am
> > still validating reselection, migration, and the remaining proxy
> > boundary cases, so I have not posted either implementation.
> > 
> > Do you think keeping the check in the original real-time/task-clock
> > domain is a reasonable direction here, or would you prefer preserving
> > the vruntime approach by carrying the accumulated service across
> > reweights?
> > 
> 
> I would vote for the real-time comparison. What do you think, Tim?
> 

Agree, I think keeping the real-time comparison makes sense.


Tim

  reply	other threads:[~2026-09-08 16:01 UTC|newest]

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-09-03  4:11 [PATCH v2 0/2] sched: Fix execution-context tick handling under proxy execution Hui Su
2026-09-03  4:11 ` [PATCH v2 1/2] sched/numa: Drive NUMA task tick from execution context Hui Su
2026-09-03 12:41   ` Chen, Yu C
2026-09-03 21:30     ` Tim Chen
2026-09-04  5:16       ` Hui Su
2026-09-04 14:10       ` Hui Su
2026-09-04 20:15         ` Tim Chen
2026-09-05 13:58           ` Hui Su
2026-09-08  5:38             ` Chen, Yu C
2026-09-08 16:01               ` Tim Chen [this message]
2026-09-09 10:55                 ` Hui Su
2026-09-04 16:10       ` Chen Yu
2026-09-04 20:24         ` Tim Chen
2026-09-03  4:11 ` [PATCH v2 2/2] sched/cache: Drive cache " Hui Su
2026-09-03  4:37   ` K Prateek Nayak
2026-09-03  4:51     ` Hui Su
2026-09-03 17:23   ` Tim Chen
2026-09-04  4:03     ` Hui Su

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=e91848b651fbe163676813fffff76970d6895cf9.camel@linux.intel.com \
    --to=tim.c.chen@linux.intel.com \
    --cc=dietmar.eggemann@arm.com \
    --cc=jstultz@google.com \
    --cc=juri.lelli@redhat.com \
    --cc=kprateek.nayak@amd.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mingo@redhat.com \
    --cc=peterz@infradead.org \
    --cc=sh_def@163.com \
    --cc=vincent.guittot@linaro.org \
    --cc=vschneid@redhat.com \
    --cc=yu.c.chen@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

all inboxes | Powered by JetHome®