Re: [PATCH rc4-mm2 2/2] posix-timers: use try_to_del_timer_sync()

[Oleg Nesterov]

George Anzinger wrote:
>
> Oleg Nesterov wrote:
> >
> > This patch removes timer_active/set_timer_inactive which plays with
> > timer_list's internals in favour of using try_to_del_timer_sync(),
> > which was introduced in the previous patch.
>
> Is there a particular reason for this, like it does not work, for example, or
> are you just trying to clean up code?

It's a cleanup, I think that current code is correct.

> If this currently works, please leave it alone.

Ok.

> We also note that this code is the subject of a patch to the RT patch to cover
> the same issue when softirqs are run from threads and therefor allow
> posix_timer_fn to be preempted.  (That fix being mainly to expand usage from
> just SMP to SMP || SOFTIRQ_PREEMPT.)

I guess you are talking about this patch:
http://marc.theaimsgroup.com/?l=linux-kernel&m=111566867218576

> Also, I think that del_timer_sync and friends need to be turned on if soft_irq
> is preemptable.

I agree completely.

> + * For RT the timer call backs are preemptable.  This means that folks
> + * trying to delete timers may run into timers that are "active" for
> + * long times.  To help out with this we provide a wake up function to
> + * wake up a caller who wants waking when a timer clears the call back.
> + * This is the same sort of thing that the del_timer_sync does, but we
> + * need (in the HRT case) to cover two lists and not just the one.
> + */
> +#ifdef CONFIG_PREEMPT_SOFTIRQS
> +#include <linux/wait.h>
> +static DECLARE_WAIT_QUEUE_HEAD(timer_wake_queue);
> +#define wake_timer_waiters() wake_up(&timer_wake_queue)
> +#define wait_for_timer(timer) wait_event(timer_wake_queue, !timer_active(timer))

I'm not an expert at all, so I may be wrong, but I don't think
it's a good idea.

I think it is bad if __run_timers() could be preempted while
->running_timer != NULL. This will interact badly with __mod_timer,
del_timer_sync. I think that __run_timers() should do:

	set_running_timer(base, timer);
	preempt_disable();
	spin_unlock_irq(&base->lock);

	timer->function();

	set_running_timer(base, NULL);
	preempt_enable();
	spin_lock_irq(&base->lock);

What do you think?

Oleg.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/