deleting timer that re-registers itself

deleting timer that re-registers itself

[Hyeonggon Yoo]

Does del_timer work well for timers that re-registers itself?
what if the timer is currently running, and del_timer is called,
and the running timer re-registers itself?

how should I handle it?

Below is my simple kernel timer example.


timer.c

#include <linux/kernel.h>
#include <linux/module.h>
#include <linux/time.h>

#define MODULE_NAME "TIMER"
#define DELAY (1 * HZ)
struct timer_data {
        int value;
        struct timer_list timer;
};

struct timer_data data = {
        .value = 0
};

void timer_callback(struct timer_list *timer) {
        struct timer_data *data = from_timer(data, timer, timer);

        data->value++;
        printk(KERN_INFO "[%s] value is = %d\n", __func__, data->value);
        mod_timer(timer, jiffies + DELAY);
}

int __init timer_init(void) {
        printk("[%s] creating timer...\n", __func__);
        timer_setup(&data.timer, timer_callback, 0);
        mod_timer(&data.timer, jiffies + DELAY);
        return 0;
}

void __exit timer_exit(void) {
        int ret = del_timer(&data.timer);
        printk("[%s] deleting timer..., ret = %d\n", __func__, ret);
}

module_init(timer_init);
module_exit(timer_exit);

MODULE_LICENSE("GPL");

_______________________________________________
Kernelnewbies mailing list
Kernelnewbies@kernelnewbies.org
https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies

Re: deleting timer that re-registers itself

[Valdis Klētnieks]

[-- Attachment #1.1: Type: text/plain, Size: 981 bytes --]

On Tue, 04 May 2021 23:59:12 +0900, Hyeonggon Yoo said:
> Does del_timer work well for timers that re-registers itself?
> what if the timer is currently running, and del_timer is called,
> and the running timer re-registers itself?

Minor nit: while the timer is running, there's no problem.
When the timer has *expired* and timer_callback() is running
is when you have a race condition.

So who's going to call del_timer()? The only thing that does that is
timer_exit. Soo...  Apply some silly locking:

static int exiting = 0;

void timer_callback(struct timer_list *timer) {
        struct timer_data *data = from_timer(data, timer, timer);
        data->value++;
        printk(KERN_INFO "[%s] value is = %d\n", __func__, data->value);
        if (!exiting)
                mod_timer(timer, jiffies + DELAY);
}

void __exit timer_exit(void) {
        exiting = 1;
        int ret = del_timer(&data.timer);
        printk("[%s] deleting timer..., ret = %d\n", __func__, ret);
}

[-- Attachment #1.2: Type: application/pgp-signature, Size: 832 bytes --]

[-- Attachment #2: Type: text/plain, Size: 170 bytes --]

_______________________________________________
Kernelnewbies mailing list
Kernelnewbies@kernelnewbies.org
https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies

Re: deleting timer that re-registers itself

[Greg KH]

On Tue, May 04, 2021 at 12:17:56PM -0400, Valdis Klētnieks wrote:
> On Tue, 04 May 2021 23:59:12 +0900, Hyeonggon Yoo said:
> > Does del_timer work well for timers that re-registers itself?
> > what if the timer is currently running, and del_timer is called,
> > and the running timer re-registers itself?
> 
> Minor nit: while the timer is running, there's no problem.
> When the timer has *expired* and timer_callback() is running
> is when you have a race condition.
> 
> So who's going to call del_timer()? The only thing that does that is
> timer_exit. Soo...  Apply some silly locking:
> 
> static int exiting = 0;
> 
> void timer_callback(struct timer_list *timer) {
>         struct timer_data *data = from_timer(data, timer, timer);
>         data->value++;
>         printk(KERN_INFO "[%s] value is = %d\n", __func__, data->value);
>         if (!exiting)
>                 mod_timer(timer, jiffies + DELAY);
> }
> 
> void __exit timer_exit(void) {
>         exiting = 1;
>         int ret = del_timer(&data.timer);
>         printk("[%s] deleting timer..., ret = %d\n", __func__, ret);
> }

That's mean, don't write buggy code as an example because then I'm going
to have to reject it when it gets submitted as a "real" driver :)


Hint, a "global" variable like this does not work like you expect it to
at all, sorry.

greg k-h

_______________________________________________
Kernelnewbies mailing list
Kernelnewbies@kernelnewbies.org
https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies

Re: deleting timer that re-registers itself

[Valdis Klētnieks]

[-- Attachment #1.1: Type: text/plain, Size: 234 bytes --]

On Tue, 04 May 2021 12:17:56 -0400, "Valdis KlD tnieks" said:

> void __exit timer_exit(void) {
>         exiting = 1;

-ENOCAFFEINE

That still needs a few memory barriers. See Documentation/memory_barriers.txt
for the gory details.

[-- Attachment #1.2: Type: application/pgp-signature, Size: 832 bytes --]

[-- Attachment #2: Type: text/plain, Size: 170 bytes --]

_______________________________________________
Kernelnewbies mailing list
Kernelnewbies@kernelnewbies.org
https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies

Re: deleting timer that re-registers itself

[Hyeonggon Yoo]

Valdis Klētnieks, Thank you for your kind explanation.


Okay, so 'timer is running' means it is not expired yet.
and when timer is expired (as specified in 'expires' field),
the timer_callback is called.

and there can be race condition in running timer_callback.
so locking is needed before re-registering timer.

2021년 5월 5일 (수) 오전 1:17, Valdis Klētnieks <valdis.kletnieks@vt.edu>님이 작성:
>
> On Tue, 04 May 2021 23:59:12 +0900, Hyeonggon Yoo said:
> > Does del_timer work well for timers that re-registers itself?
> > what if the timer is currently running, and del_timer is called,
> > and the running timer re-registers itself?
>
> Minor nit: while the timer is running, there's no problem.
> When the timer has *expired* and timer_callback() is running
> is when you have a race condition.
>
> So who's going to call del_timer()? The only thing that does that is
> timer_exit. Soo...  Apply some silly locking:
>
> static int exiting = 0;
>
> void timer_callback(struct timer_list *timer) {
>         struct timer_data *data = from_timer(data, timer, timer);
>         data->value++;
>         printk(KERN_INFO "[%s] value is = %d\n", __func__, data->value);
>         if (!exiting)
>                 mod_timer(timer, jiffies + DELAY);
> }
>
> void __exit timer_exit(void) {
>         exiting = 1;
>         int ret = del_timer(&data.timer);
>         printk("[%s] deleting timer..., ret = %d\n", __func__, ret);
> }

_______________________________________________
Kernelnewbies mailing list
Kernelnewbies@kernelnewbies.org
https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies

Re: deleting timer that re-registers itself

[Hyeonggon Yoo]

okay, there can be optimization when reading exiting variable.

then is memory barrier is enough as  Valdis Klētnieks said,
or spinlock is better choice?

2021년 5월 5일 (수) 오전 1:36, Hyeonggon Yoo <42.hyeyoo@gmail.com>님이 작성:
>
> Valdis Klētnieks, Thank you for your kind explanation.
>
>
> Okay, so 'timer is running' means it is not expired yet.
> and when timer is expired (as specified in 'expires' field),
> the timer_callback is called.
>
> and there can be race condition in running timer_callback.
> so locking is needed before re-registering timer.
>
> 2021년 5월 5일 (수) 오전 1:17, Valdis Klētnieks <valdis.kletnieks@vt.edu>님이 작성:
> >
> > On Tue, 04 May 2021 23:59:12 +0900, Hyeonggon Yoo said:
> > > Does del_timer work well for timers that re-registers itself?
> > > what if the timer is currently running, and del_timer is called,
> > > and the running timer re-registers itself?
> >
> > Minor nit: while the timer is running, there's no problem.
> > When the timer has *expired* and timer_callback() is running
> > is when you have a race condition.
> >
> > So who's going to call del_timer()? The only thing that does that is
> > timer_exit. Soo...  Apply some silly locking:
> >
> > static int exiting = 0;
> >
> > void timer_callback(struct timer_list *timer) {
> >         struct timer_data *data = from_timer(data, timer, timer);
> >         data->value++;
> >         printk(KERN_INFO "[%s] value is = %d\n", __func__, data->value);
> >         if (!exiting)
> >                 mod_timer(timer, jiffies + DELAY);
> > }
> >
> > void __exit timer_exit(void) {
> >         exiting = 1;
> >         int ret = del_timer(&data.timer);
> >         printk("[%s] deleting timer..., ret = %d\n", __func__, ret);
> > }

_______________________________________________
Kernelnewbies mailing list
Kernelnewbies@kernelnewbies.org
https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies

Re: deleting timer that re-registers itself

[Hyeonggon Yoo]

I think there can be a situation like:

         processA:                  processB:
0ns:  if (!exiting)
1ns:                                    exiting = 1;
2ns:                                    del_timer
3ns:  mod_timer

so spinlock seems better for this.

_______________________________________________
Kernelnewbies mailing list
Kernelnewbies@kernelnewbies.org
https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies

Re: deleting timer that re-registers itself

[Greg KH]

On Tue, May 04, 2021 at 12:35:06PM -0400, Valdis Klētnieks wrote:
> On Tue, 04 May 2021 12:17:56 -0400, "Valdis KlD tnieks" said:
> 
> > void __exit timer_exit(void) {
> >         exiting = 1;
> 
> -ENOCAFFEINE
> 
> That still needs a few memory barriers. See Documentation/memory_barriers.txt
> for the gory details.

Use a real lock, it's much simpler :)


_______________________________________________
Kernelnewbies mailing list
Kernelnewbies@kernelnewbies.org
https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies

Re: deleting timer that re-registers itself

[Hyeonggon Yoo]

Thank you Greg and Valdis! I applied simple spinlock and it's now
looks much more safe!
Below is my modified version.

timer.c

#include <linux/kernel.h>
#include <linux/module.h>
#include <linux/time.h>
#include <linux/spinlock.h>

#define MODULE_NAME "TIMER"
#define DELAY (1 * HZ)

struct timer_data {
        int value;
        spinlock_t lock;
        struct timer_list timer;
        bool isActive;
};

struct timer_data my_data = {};

void timer_callback(struct timer_list *timer) {
        struct timer_data *data = from_timer(data, timer, timer);

        data->value++;
        printk(KERN_INFO "[%s] value is = %d\n", __func__, data->value);
        spin_lock(&data->lock);
        if (data->isActive)
                mod_timer(timer, jiffies + DELAY);
        spin_unlock(&data->lock);
}

int __init timer_init(void) {
        printk("[%s] creating timer...\n", __func__);

        /* initialization */
        my_data.isActive = true;
        spin_lock_init(&my_data.lock);
        timer_setup(&my_data.timer, timer_callback, 0);

        /* register timer */
        mod_timer(&my_data.timer, jiffies + DELAY);
        return 0;
}

void __exit timer_exit(void) {
        int ret;

        spin_lock(&my_data.lock);
        my_data.isActive = false;
        ret = del_timer(&my_data.timer);
        spin_unlock(&my_data.lock);

        printk("[%s] deleting timer..., ret = %d\n", __func__, ret);
}

module_init(timer_init);
module_exit(timer_exit);

MODULE_LICENSE("GPL");

2021년 5월 5일 (수) 오전 2:02, Greg KH <greg@kroah.com>님이 작성:
>
> On Tue, May 04, 2021 at 12:35:06PM -0400, Valdis Klētnieks wrote:
> > On Tue, 04 May 2021 12:17:56 -0400, "Valdis KlD tnieks" said:
> >
> > > void __exit timer_exit(void) {
> > >         exiting = 1;
> >
> > -ENOCAFFEINE
> >
> > That still needs a few memory barriers. See Documentation/memory_barriers.txt
> > for the gory details.
>
> Use a real lock, it's much simpler :)
>

_______________________________________________
Kernelnewbies mailing list
Kernelnewbies@kernelnewbies.org
https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies