Query about timer wheel API

Query about timer wheel API

[imran.f.khan]

Hello Thomas,

Could you kindly help me, regarding a query about timer wheel APIs.
Right now we use add_timer or add_timer_on, to add a timer to any
or to a specific CPU respectively.
Would it be useful to have an interface like try_add_timer_on, that
would return an error or would use add_timer, if the specified CPU is
offline.

Recently we have come across some bugs in the RDS code, where a delayed
work was being queued on an offlined CPU and as a result of that the
underlying timer was not firing, which in turn meant that the work was
never able to make it to the intended worker_pool.

I understand that this is something that needs fixing at caller side and
we are taking that approach.

But I also wanted to understand if there is some scope of change on timer
side, for such situations. I saw your reply in [1] and agree with your point.
But that conversation is more than a decade old, so I thought of asking this
question, assuming that there may be some other use cases that can utilize
this new interface.

One can also ask to change queue_delayed_work_on or have an equivalent,
that would check if CPU is online before doing add_timer_on but I am not sure
if workqueue is the only subsystem that can run into this situation.

Thanks in advance for your help,
Imran

[1]: https://lists.linuxcoding.com/kernel/2007-q4/msg27627.html

Re: Query about timer wheel API

[Hillf Danton]

On Mon, 23 Dec 2024 11:14:21 +1100 imran.f.khan@oracle.com
> 
> Recently we have come across some bugs in the RDS code, where a delayed
> work was being queued on an offlined CPU and as a result of that the

Such a queue could not happen given irq disabled in queue_delayed_work_on().
Did you see it upstream?

> underlying timer was not firing, which in turn meant that the work was
> never able to make it to the intended worker_pool.

Re: Query about timer wheel API

[imran.f.khan]

Hello Hillf,
On 23/12/2024 11:51 pm, Hillf Danton wrote:
> On Mon, 23 Dec 2024 11:14:21 +1100 imran.f.khan@oracle.com
>>
>> Recently we have come across some bugs in the RDS code, where a delayed
>> work was being queued on an offlined CPU and as a result of that the
> 
> Such a queue could not happen given irq disabled in queue_delayed_work_on().
> Did you see it upstream?
> 
You mean upstream RDS or upstream workqueue ? For RDS I need to check, but with 
upstream v6.6 kernel, I was able to submit a delayed work to an offlined CPU.
The delayed work would never happen and I can see corresponding timer in timer
list of offlined  CPU (using crash). 
Once the CPU is brought back online, depending on the workload the work handler
gets executed.

I used following test module:

===============

#include <linux/module.h>
#include <linux/types.h>
#include <linux/kernel.h>
#include <linux/workqueue.h>
#include <linux/completion.h>
#include <linux/delay.h>
#include <linux/slab.h>
#include <linux/jiffies.h>

#define TIMEOUT 1 /* test timeout in secs */
#define NUM_WORK_ITEMS 1 /* number of work items to submit */


static DEFINE_MUTEX(mutex);

static DEFINE_MUTEX(dwork_func_mutex);

static void delayed_work_func(struct work_struct *data)
{
	int cpu;
	mutex_lock(&dwork_func_mutex);
	cpu = get_cpu();
	pr_err("%s invoked for work: 0x%px on cpu#%d \n", __func__, data, cpu);
	put_cpu();
	mutex_unlock(&dwork_func_mutex);
}

static int param_set_queue_work_on_cpu(const char *val, const struct kernel_param *kp)
{
	int cpu, this_cpu, i;
	struct delayed_work *dwork = NULL;

	if (!mutex_trylock(&mutex))
		return -EBUSY;

	cpu = simple_strtoul(val, NULL, 0);
	/*if (!cpu_present(cpu)) 
		return -EINVAL;*/

	for (i = 0; i < NUM_WORK_ITEMS; i++) {
		dwork = kzalloc(sizeof(struct delayed_work), GFP_KERNEL);
		if(dwork) {
			this_cpu = get_cpu();
			INIT_DELAYED_WORK(dwork, delayed_work_func);
			queue_delayed_work_on(cpu, system_wq, dwork, msecs_to_jiffies(10000));
			pr_err("Submitted dwork 0x%px on %s cpu#%d \n", dwork, cpu_online(cpu)?"online":"offline", cpu);
			put_cpu();
		}

	}
	mutex_unlock(&mutex);
	return 0;
}

module_param_call(queue_work_on_cpu, param_set_queue_work_on_cpu, NULL, NULL, 0600);

static int __init workqueue_study_init(void)
{
	pr_err("module_init \n");
	
	return 0;
}

static void workqueue_study_exit(void)
{
	pr_err("module_exit \n");
}

MODULE_AUTHOR("Imran Khan <imran.eie.85@gmail.com>");
MODULE_DESCRIPTION("Workqueue study");
MODULE_LICENSE("GPL");

module_init(workqueue_study_init);
module_exit(workqueue_study_exit);

===========

This module gives an interface at:

/sys/module/<module name>/params/queue_work_on_cpu

Writing X there would submit a delayed_work (delay 10 secs)
to CPU X.

We can see if CPU X is online, submitted work gets executed
after around 10 secs. But if CPU X is offline, the submitted
work handler does not get fired unless the CPU has been brought
back online.

Thanks,
Imran
>> underlying timer was not firing, which in turn meant that the work was
>> never able to make it to the intended worker_pool.

Re: Query about timer wheel API

[Hillf Danton]

On Tue, 24 Dec 2024 01:20:48 +1100 imran.f.khan@oracle.com
> 
> static int param_set_queue_work_on_cpu(const char *val, const struct kernel_param *kp)
> {
> 	int cpu, this_cpu, i;
> 	struct delayed_work *dwork = NULL;
> 
> 	if (!mutex_trylock(&mutex))
> 		return -EBUSY;
> 
> 	cpu = simple_strtoul(val, NULL, 0);
> 	/*if (!cpu_present(cpu)) 
> 		return -EINVAL;*/
> 
> 	for (i = 0; i < NUM_WORK_ITEMS; i++) {
> 		dwork = kzalloc(sizeof(struct delayed_work), GFP_KERNEL);
> 		if(dwork) {
> 			this_cpu = get_cpu();

See if checking cpu works for you.

			if (cpu >= nr_cpu_ids || !cpu_online(cpu)) {
 				put_cpu();
				pr_err("%s cpu%d invalid\n", __func__, cpu);
				break;
			}

> 			INIT_DELAYED_WORK(dwork, delayed_work_func);
> 			queue_delayed_work_on(cpu, system_wq, dwork, msecs_to_jiffies(10000));
> 			pr_err("Submitted dwork 0x%px on %s cpu#%d \n", dwork, cpu_online(cpu)?"online":"offline", cpu);
> 			put_cpu();
> 		}
> 
> 	}
> 	mutex_unlock(&mutex);
> 	return 0;
> }

Re: Query about timer wheel API

[imran.f.khan]

Hello Hillf,
On 24/12/2024 9:41 pm, Hillf Danton wrote:
> On Tue, 24 Dec 2024 01:20:48 +1100 imran.f.khan@oracle.com
>>
>> static int param_set_queue_work_on_cpu(const char *val, const struct kernel_param *kp)
>> {
>> 	int cpu, this_cpu, i;
>> 	struct delayed_work *dwork = NULL;
>>
>> 	if (!mutex_trylock(&mutex))
>> 		return -EBUSY;
>>
>> 	cpu = simple_strtoul(val, NULL, 0);
>> 	/*if (!cpu_present(cpu)) 
>> 		return -EINVAL;*/
>>
>> 	for (i = 0; i < NUM_WORK_ITEMS; i++) {
>> 		dwork = kzalloc(sizeof(struct delayed_work), GFP_KERNEL);
>> 		if(dwork) {
>> 			this_cpu = get_cpu();
> 
> See if checking cpu works for you.
> 
> 			if (cpu >= nr_cpu_ids || !cpu_online(cpu)) {
>  				put_cpu();
> 				pr_err("%s cpu%d invalid\n", __func__, cpu);
> 				break;
> 			}
> 

the query was not about why its (not) working with my module. The test module, in its
current form, is just to show that a timer-wheel timer could be inserted in timer list
of an offlined CPU.

What you have suggested, we are already doing it in RDS code (mentioned in my earlier messages).
Also just using cpu_online may not be enough, unless we do it under get/put_online_cpus.

If you see, my query was more towards, what should "add_timer_on" do for such cases or
can we  have another function like try_add_timer_on that tries to put the timer on specific
CPU, but puts it else where if that CPU is offline. Is it worth having such an interface 
or should we stick to the current approach of fixing this on the caller side.

Thanks,
Imran

>> 			INIT_DELAYED_WORK(dwork, delayed_work_func);
>> 			queue_delayed_work_on(cpu, system_wq, dwork, msecs_to_jiffies(10000));
>> 			pr_err("Submitted dwork 0x%px on %s cpu#%d \n", dwork, cpu_online(cpu)?"online":"offline", cpu);
>> 			put_cpu();
>> 		}
>>
>> 	}
>> 	mutex_unlock(&mutex);
>> 	return 0;
>> }

Re: Query about timer wheel API

[Hillf Danton]

On Tue, 24 Dec 2024 23:41:48 +1100 imran.f.khan@oracle.com
> 
> the query was not about why its (not) working with my module. The test module, in its
> current form, is just to show that a timer-wheel timer could be inserted in timer list
> of an offlined CPU.

Your module helps understand your query.
> 
> What you have suggested, we are already doing it in RDS code (mentioned in my earlier messages).
> Also just using cpu_online may not be enough, unless we do it under get/put_online_cpus.
> 
Same pattern is in smp_call_function_single() where ckecking cpu after get_cpu().
But different one in smp_call_on_cpu().

> If you see, my query was more towards, what should "add_timer_on" do for such cases or
> can we  have another function like try_add_timer_on that tries to put the timer on specific
> CPU, but puts it else where if that CPU is offline. Is it worth having such an interface 
> or should we stick to the current approach of fixing this on the caller side.

The number of reports that timer is queued on offline cpu in 2024 alone raises 
the (known) question -- what sense could be made by adding check of cpu in the 
pathes like queuing work and arming timer?

Re: Query about timer wheel API

[imran.f.khan]

Hello Hillf,
On 25/12/2024 10:01 pm, Hillf Danton wrote:
> On Tue, 24 Dec 2024 23:41:48 +1100 imran.f.khan@oracle.com
>>
>> the query was not about why its (not) working with my module. The test module, in its
>> current form, is just to show that a timer-wheel timer could be inserted in timer list
>> of an offlined CPU.
> 
> Your module helps understand your query.
>>
>> What you have suggested, we are already doing it in RDS code (mentioned in my earlier messages).
>> Also just using cpu_online may not be enough, unless we do it under get/put_online_cpus.
>>
> Same pattern is in smp_call_function_single() where ckecking cpu after get_cpu().
> But different one in smp_call_on_cpu().
> 
>> If you see, my query was more towards, what should "add_timer_on" do for such cases or
>> can we  have another function like try_add_timer_on that tries to put the timer on specific
>> CPU, but puts it else where if that CPU is offline. Is it worth having such an interface 
>> or should we stick to the current approach of fixing this on the caller side.
> 
> The number of reports that timer is queued on offline cpu in 2024 alone raises 
> the (known) question -- what sense could be made by adding check of cpu in the 
> pathes like queuing work and arming timer?

Sorry, I could not understand the last part of your comment. Do you mean that its a
known issue that should be addressed or do you mean that my use case of adding delayed_work
and/or timer to offlined CPUs is an invalid use case and should be fixed on the caller side ?

Thanks,
Imran

Re: Query about timer wheel API

[Hillf Danton]

On Fri, 27 Dec 2024 00:07:11 +1100 imran.f.khan@oracle.com
> Sorry, I could not understand the last part of your comment. Do you mean that its a
> known issue that should be addressed or do you mean that my use case of adding delayed_work
> and/or timer to offlined CPUs is an invalid use case and should be fixed on the caller side ?
>
I mean, post a (rfc) patch fixing it.

Re: Query about timer wheel API

[Thomas Gleixner]

On Mon, Dec 23 2024 at 11:14, imran f. khan wrote:
> Could you kindly help me, regarding a query about timer wheel APIs.
> Right now we use add_timer or add_timer_on, to add a timer to any
> or to a specific CPU respectively.
> Would it be useful to have an interface like try_add_timer_on, that
> would return an error or would use add_timer, if the specified CPU is
> offline.
>
> Recently we have come across some bugs in the RDS code, where a delayed
> work was being queued on an offlined CPU and as a result of that the
> underlying timer was not firing, which in turn meant that the work was
> never able to make it to the intended worker_pool.

Urgh. Clearly add_timer_on() lacks a check and a warning for that.

> I understand that this is something that needs fixing at caller side and
> we are taking that approach.
>
> But I also wanted to understand if there is some scope of change on timer
> side, for such situations. I saw your reply in [1] and agree with your point.
> But that conversation is more than a decade old, so I thought of asking this
> question, assuming that there may be some other use cases that can utilize
> this new interface.
>
> One can also ask to change queue_delayed_work_on or have an equivalent,
> that would check if CPU is online before doing add_timer_on but I am not sure
> if workqueue is the only subsystem that can run into this situation.

I have no idea.

That said, I'm not opposed to have a function like
timer_try_add_on_cpu(), which validates that the CPU is online and
returns true if the timer is queued or false if the target CPU is
offline. Then the call site can decided what to do with that
situation. That's way better than queueing it on some randomly picked
online CPU silently.

Thanks,

        tglx

Re: Query about timer wheel API

[imran.f.khan]

Hello Thomas,
Thank you so much for your feedback.
On 15/1/2025 9:32 pm, Thomas Gleixner wrote:
> On Mon, Dec 23 2024 at 11:14, imran f. khan wrote:
>> Could you kindly help me, regarding a query about timer wheel APIs.
>> Right now we use add_timer or add_timer_on, to add a timer to any
>> or to a specific CPU respectively.
>> Would it be useful to have an interface like try_add_timer_on, that
>> would return an error or would use add_timer, if the specified CPU is
>> offline.
>>
>> Recently we have come across some bugs in the RDS code, where a delayed
>> work was being queued on an offlined CPU and as a result of that the
>> underlying timer was not firing, which in turn meant that the work was
>> never able to make it to the intended worker_pool.
> 
> Urgh. Clearly add_timer_on() lacks a check and a warning for that.
> 
>> I understand that this is something that needs fixing at caller side and
>> we are taking that approach.
>>
>> But I also wanted to understand if there is some scope of change on timer
>> side, for such situations. I saw your reply in [1] and agree with your point.
>> But that conversation is more than a decade old, so I thought of asking this
>> question, assuming that there may be some other use cases that can utilize
>> this new interface.
>>
>> One can also ask to change queue_delayed_work_on or have an equivalent,
>> that would check if CPU is online before doing add_timer_on but I am not sure
>> if workqueue is the only subsystem that can run into this situation.
> 
> I have no idea.
> 
> That said, I'm not opposed to have a function like
> timer_try_add_on_cpu(), which validates that the CPU is online and
> returns true if the timer is queued or false if the target CPU is
> offline. Then the call site can decided what to do with that
> situation. That's way better than queueing it on some randomly picked
> online CPU silently.
> 

I have sent a couple of patches at [1] in this regard. Could you kindly
have a look and let me know your feedback.

Thanks,
Imran

[1]: https://lore.kernel.org/all/20250115134111.2703089-1-imran.f.khan@oracle.com/