Message-ID: <8715d5cf-1f63-4c57-b855-8abae8debaa2@oracle.com>
Date: Fri, 31 Jan 2025 21:37:09 +1100
From: imran.f.khan@...cle.com
To: Tejun Heo <tj@...nel.org>
Cc: jiangshanlai@...il.com, linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH] workqueue: introduce
 queue_delayed_work_on_offline_safe.

Hello Tejun,

On 14/1/2025 2:01 pm, imran.f.khan@...cle.com wrote:
> Hello Tejun,
> Thanks for taking a look into it.
> On 14/1/2025 5:21 am, Tejun Heo wrote:
>> On Mon, Jan 13, 2025 at 03:35:40PM +1100, Imran Khan wrote:
>> ...
>>> I have kept the patch as RFC because from mailing list,
>>> I could not find any users, of queue_delayed_work_on,
>>> that is ending up queuing dwork on an offlined CPU.
>>> We have some in-house code that is running into this problem,
>>> and currently we are fixing it on caller side of queue_delayed_work_on.
>>> Other users who run into this issue, can also use the approach of
>>> fixing it on caller side or we can use the interface introduced
>>> here for such use cases.
>>
>> I'm not sure how necessary this is. If the timer is okay to run on other
>> CPUs, might as well just use queue_delayed_work().
>>

Could you kindly let me know, if it would be acceptable, to have 
queue_delayed_work_on_offline_safe, as a wrapper around 
queue_delayed_work_on, such that it can check and ensure CPU's
availability. If it can't, then it can simply return false and let
caller decide which cpu to use next. Something like below:


diff --git a/include/linux/workqueue.h b/include/linux/workqueue.h
index b0dc957c3e560..57f39807f3bf1 100644
--- a/include/linux/workqueue.h
+++ b/include/linux/workqueue.h
@@ -589,6 +589,9 @@ extern bool queue_work_node(int node, struct workqueue_struct *wq,
                            struct work_struct *work);
 extern bool queue_delayed_work_on(int cpu, struct workqueue_struct *wq,
                        struct delayed_work *work, unsigned long delay);
+extern bool queue_delayed_work_on_offline_safe(int cpu,
+                       struct workqueue_struct *wq, struct delayed_work *work,
+                       unsigned long delay);
 extern bool mod_delayed_work_on(int cpu, struct workqueue_struct *wq,
                        struct delayed_work *dwork, unsigned long delay);
 extern bool queue_rcu_work(struct workqueue_struct *wq, struct rcu_work *rwork);

diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 9362484a653c4..7d3b8050422e4 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -2565,6 +2565,37 @@ bool queue_delayed_work_on(int cpu, struct workqueue_struct *wq,
 }
 EXPORT_SYMBOL(queue_delayed_work_on);

+/**
+ * queue_delayed_work_on_offline_safe - queue work on specific online CPU after
+ *                                     delay,
+ *
+ * @cpu: CPU number to execute work on
+ * @wq: workqueue to use
+ * @dwork: work to queue
+ * @delay: number of jiffies to wait before queueing
+ *
+ * a wrapper, around queue_delayed_work_on, that checks and ensures that
+ * specified @cpu is online. If @cpu is found to be offline or if its online
+ * status can't be reliably determined, return false and leave the decision,
+ * of selecting new cpu for delayed_work, to caller.
+ *
+ */
+bool queue_delayed_work_on_offline_safe(int cpu, struct workqueue_struct *wq,
+                          struct delayed_work *dwork, unsigned long delay)
+{
+       bool ret = false;
+       int locked = 0;
+
+       if ((locked = cpus_read_trylock()) && cpu_online(cpu))
+               ret = queue_delayed_work_on(cpu, wq, dwork, delay);
+       else if (locked)
+               cpus_read_unlock();
+
+       return ret;
+}
+EXPORT_SYMBOL(queue_delayed_work_on_offline_safe);
+
+

If this looks acceptable to you, I can send a v2 of earlier patch.

Thanks,
Imran

> 
> Yes, right now I can't locate something in upstream kernel that gets
> broken due to the issue mentioned here.
> All (except 3, mentioned further down) users of queued_delayed_work_on
> are using smp_processor_id(), to specify the CPU. So specified CPU can't
> be an already offlined CPU.
> 
> I see below 3 files (in v6.12.6), using queue_delayed_work_on with some sort of cached
> cpu information:
> 
>   drivers/net/ethernet/pensando/ionic/ionic_dev.c -> line 177
>   drivers/scsi/esas2r/esas2r_main.c -> line 1858
>   drivers/scsi/lpfc/lpfc_sli.c
>        -> line 14987
>        -> line 15381 
> But looks like in these cases specified CPU remains online or
> they simply have not encountered the issue mentioned here.
> 
> 
> For this patch, yes the timer is okay to run on other CPUs but that is
> only as a last resort, most of the times it could still run on specified
> CPU (assuming its online)
> 
> Thanks,
> Imran
> 
> 
> 
>> Thanks.
>>
> 

Powered by blists - more mailing lists