Received-SPF: pass (google.com: domain of linux-kernel+bounces-39134-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Sender: Tejun Heo <htejun@gmail.com>
Date: Thu, 25 Jan 2024 08:38:25 -1000
From: Tejun Heo <tj@kernel.org>
To: Leonardo Bras <leobras@redhat.com>
Cc: Lai Jiangshan <jiangshanlai@gmail.com>,
	Marcelo Tosatti <mtosatti@redhat.com>, linux-kernel@vger.kernel.org
Subject: Re: [RFC PATCH v1 1/1] wq: Avoid using isolated cpus' timers on
 unbounded queue_delayed_work
Message-ID: <ZbKqoeC7JVcJULdI@slm.duckdns.org>
References: <20240124082938.2527697-2-leobras@redhat.com>
 <ZbGFce8ixJKb6umE@slm.duckdns.org>
 <ZbG9TjHAMJYIvwsg@LeoBras>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ZbG9TjHAMJYIvwsg@LeoBras>

Hello,

On Wed, Jan 24, 2024 at 10:45:50PM -0300, Leonardo Bras wrote:
> That's a good suggestion, but looking at workqueue_init_early() I see that, 
> in short:
> wq_unbound_cpumask = 	cpu_possible_mask & 
> 			housekeeping_cpumask(HK_TYPE_WQ) & 
> 			housekeeping_cpumask(HK_TYPE_DOMAIN) &
> 			wq_cmdline_cpumask
> 
> So wq_unbound_cpumask relates to domain and workqueue cpu isolation.
> 
> In our case, we are using this to choose in which cpu is the timer we want 
> to use, so it makes sense to use timer-related cpu isolation, instead.

- In the proposed code, when cpu == WORK_CPU_UNBOUND, it's always setting
  cpu to housekeeping_any_cpu(HK_TYPE_TIMER). This may unnecessarily move
  the timer and task away from local CPU. Preferring the local CPU would
  likely make sense.

- If HK_TYPE_TIMER and workqueue masks may not agree, setting dwork->cpu to
  the one returned from HK_TYPE_TIMER is likely problematic. That would
  force __queue_work() to use that CPU instead of picking one from
  wq_unbound_cpumask.

> As of today, your suggestion would work the same, as the only way to enable 
> WQ cpu isolation is to use nohz_full, which also enables TIMER cpu 
> isolation. But since that can change in the future, for any reason, I would 
> suggest that we stick to using the HK_TYPE_TIMER cpumask.
> 
> I can now notice that this can end up introducing an issue: possibly 
> running on a workqueue on a cpu outside of a valid wq_cmdline_cpumask.

Yeap.

> I would suggest fixing this in a couple ways:
> 1 - We introduce a new cpumask which is basically 
>     housekeeping_cpumask(HK_TYPE_DOMAIN) & wq_cmdline_cpumask, allowing us 
>     to keep the timer interrupt in the same cpu as the scheduled function,
> 2- We use the resulting cpu only to pick the right timer.
> 
> What are your thouhts on that?

How about something like the following instead?

- If current CPU is in HK_TYPE_TIMER, pick that CPU.

- If not, pick a CPU from HK_TYPE_TIMER.

- Do add_timer_on() on the selected CPU but leave dwork->cpu as
  WORK_CPU_UNBOUND and leave that part to __queue_work().

Thanks.

-- 
tejun