From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from out-176.mta1.migadu.com (out-176.mta1.migadu.com [95.215.58.176]) (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 BC14B1DE2BF for ; Thu, 11 Dec 2025 07:41:53 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=95.215.58.176 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1765438916; cv=none; b=nbZc1gQqBOcwVICK1alJYpYs1rrp9kiZpq6LGk+jaeZeTlHAk98O7gL1zqHRkSwZX9kywRtoWFkTcNtHWSIfOMhFGmFYMiPQjP4Hu2ztJUy2+jMO1L+ieknWFRe42RGfjUeOSYwEu2SvJq1tKxPomsi6isXObkYbrXtESpAFk9s= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1765438916; c=relaxed/simple; bh=CtHIhXYZq96YYwa6MGB/AqZixNCAD3DYvim6K8ldgAo=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=Bp+LcLue7sxz+O3YDT01Jnlm3gsw0gvSCbN1IYN48nNv7G3/WmFqQ9kfhsi7OrbXtHAP9+GLjUyQLozd2mi114Zzjxv0KGq5zjG3ogisPRXIVGCwfRaOzznsIkPVmvfEzKzvu+SmwBLdbyBokg2VLVNY2z5hsBFSOAcMbdJJF88= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linux.dev; spf=pass smtp.mailfrom=linux.dev; dkim=pass (1024-bit key) header.d=linux.dev header.i=@linux.dev header.b=PXpImm8n; arc=none smtp.client-ip=95.215.58.176 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linux.dev Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=linux.dev Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=linux.dev header.i=@linux.dev header.b="PXpImm8n" Message-ID: DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.dev; s=key1; t=1765438911; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=VBvrRMnriUUHaU1Jw1FZWeWAyUvaD7SSfif1uAXxErw=; b=PXpImm8nLGaGjx56U5IUVDJ6DG+Y98+2Id9YH2EZrDSTgHwr8Bf0aHWMIlxZc1a2kP/vjk A4knUCnBBHslPHfW++1nGCeUnLCOvtT67f5rKhGSC6eiEvdfHtxJg8wdycQJUbZkuQlZEH G3P0tpHTL8g8yv6HGHv1S68YaYB3dfc= Date: Thu, 11 Dec 2025 15:41:40 +0800 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Subject: Re: [PATCH 1/2] sched: psi: loosen clock sync between scheduler and aggregator To: Johannes Weiner , Peter Zijlstra , Suren Baghdasaryan , Ingo Molnar Cc: Dietmar Eggemann , John Stultz , linux-kernel@vger.kernel.org References: <20251210155805.752523-1-hannes@cmpxchg.org> X-Report-Abuse: Please report any abuse attempt to abuse@migadu.com and include these headers. From: Chengming Zhou In-Reply-To: <20251210155805.752523-1-hannes@cmpxchg.org> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Migadu-Flow: FLOW_OUT On 2025/12/10 23:58, Johannes Weiner wrote: > In the aggregator, catch races between state snooping and task state > conclusions explicitly by checking for sample underflows; then move > the clock reads out of the reader's seqcount protection. > > This shrinks the critical section and allows switching the scheduler > side to looser (cheaper) clock sourcing in the next patch. > > Suggested-by: Chengming Zhou > Signed-off-by: Johannes Weiner LGTM! Reviewed-by: Chengming Zhou Thanks. > --- > kernel/sched/psi.c | 34 +++++++++++++++++++++++++++------- > 1 file changed, 27 insertions(+), 7 deletions(-) > > diff --git a/kernel/sched/psi.c b/kernel/sched/psi.c > index 59fdb7ebbf22..4b7bf8eb46c2 100644 > --- a/kernel/sched/psi.c > +++ b/kernel/sched/psi.c > @@ -285,7 +285,6 @@ static void get_recent_times(struct psi_group *group, int cpu, > /* Snapshot a coherent view of the CPU state */ > do { > seq = psi_read_begin(cpu); > - now = cpu_clock(cpu); > memcpy(times, groupc->times, sizeof(groupc->times)); > state_mask = groupc->state_mask; > state_start = groupc->state_start; > @@ -293,6 +292,9 @@ static void get_recent_times(struct psi_group *group, int cpu, > memcpy(tasks, groupc->tasks, sizeof(groupc->tasks)); > } while (psi_read_retry(cpu, seq)); > > + if (state_mask) > + now = cpu_clock(cpu); > + > /* Calculate state time deltas against the previous snapshot */ > for (s = 0; s < NR_PSI_STATES; s++) { > u32 delta; > @@ -308,7 +310,22 @@ static void get_recent_times(struct psi_group *group, int cpu, > if (state_mask & (1 << s)) > times[s] += now - state_start; > > + /* > + * This snooping ahead can obviously race with the > + * state concluding on the cpu. If we previously > + * snooped to a time past where the state concludes, > + * times[s] can now be behind times_prev[s]. > + * > + * time_after32() would be the obvious choice, but > + * S32_MAX is right around two seconds, which is the > + * aggregation interval; if the aggregator gets > + * delayed, there would be a risk of dismissing > + * genuinely large samples. Use a larger margin. > + */ > delta = times[s] - groupc->times_prev[aggregator][s]; > + if (delta > psi_period + (psi_period >> 1)) > + delta = 0; > + > groupc->times_prev[aggregator][s] = times[s]; > > times[s] = delta; > @@ -908,16 +925,18 @@ static void psi_flags_change(struct task_struct *task, int clear, int set) > > void psi_task_change(struct task_struct *task, int clear, int set) > { > - int cpu = task_cpu(task); > + int cpu; > u64 now; > > if (!task->pid) > return; > > + cpu = task_cpu(task); > + now = cpu_clock(cpu); > + > psi_flags_change(task, clear, set); > > psi_write_begin(cpu); > - now = cpu_clock(cpu); > for_each_group(group, task_psi_group(task)) > psi_group_change(group, cpu, clear, set, now, true); > psi_write_end(cpu); > @@ -928,10 +947,9 @@ void psi_task_switch(struct task_struct *prev, struct task_struct *next, > { > struct psi_group *common = NULL; > int cpu = task_cpu(prev); > - u64 now; > + u64 now = cpu_clock(cpu); > > psi_write_begin(cpu); > - now = cpu_clock(cpu); > > if (next->pid) { > psi_flags_change(next, 0, TSK_ONCPU); > @@ -999,6 +1017,7 @@ void psi_task_switch(struct task_struct *prev, struct task_struct *next, > psi_group_change(group, cpu, clear, set, now, wake_clock); > } > } > + > psi_write_end(cpu); > } > > @@ -1027,9 +1046,9 @@ void psi_account_irqtime(struct rq *rq, struct task_struct *curr, struct task_st > return; > rq->psi_irq_time = irq; > > - psi_write_begin(cpu); > now = cpu_clock(cpu); > > + psi_write_begin(cpu); > for_each_group(group, task_psi_group(curr)) { > if (!group->enabled) > continue; > @@ -1234,8 +1253,9 @@ void psi_cgroup_restart(struct psi_group *group) > > guard(rq_lock_irq)(cpu_rq(cpu)); > > - psi_write_begin(cpu); > now = cpu_clock(cpu); > + > + psi_write_begin(cpu); > psi_group_change(group, cpu, 0, 0, now, true); > psi_write_end(cpu); > }