Re: [PATCH] timers: consider slack value in mod_timer()

[Yong Zhang]

On Wed, May 25, 2011 at 6:17 PM, Thomas Gleixner <tglx@xxxxxxxxxxxxx> wrote:
>> Hmmm, so the reason is for a timer whose timer->slack is not set
>> explicitly. when we recalculate expires, we will get different value
>> sometimes.
>
> No, that's not the problem.
>
>> Could you please try the attached patch(webmail will mangle it)
>
> Grrr. gmail allows usage of real mail clients, doesn't it ?

Yeah, but sometimes I can only access webmail due to some reason

>
>> diff --git a/kernel/timer.c b/kernel/timer.c
>> index fd61986..73af53c 100644
>> --- a/kernel/timer.c
>> +++ b/kernel/timer.c
>> @@ -749,6 +749,10 @@ unsigned long apply_slack(struct timer_list *timer, unsigned long expires)
>> Â Â Â unsigned long expires_limit, mask;
>> Â Â Â int bit;
>>
>> + Â Â /* no need to account slack again for a same-expire pending timer */
>> + Â Â if (timer_pending(timer) && time_after_eq(timer->expires, expires))
>> + Â Â Â Â Â Â return timer->expires;
>
> That's total crap. Assume some code sets the timer with 5 seconds for
> some purpose and after a second it wants it to fire in 50ms from now
> because some state change happened. The above will keep the original 5
> seconds timeout no matter what, so the requested 50ms timeout will
> fire about 4 seconds late.

Indeed. I forgot that case
.
>
>> Â Â Â expires_limit = expires;
>>
>> Â Â Â if (timer->slack >= 0) {
>> @@ -795,6 +799,8 @@ unsigned long apply_slack(struct timer_list *timer, unsigned long expires)
>> Â */
>> Âint mod_timer(struct timer_list *timer, unsigned long expires)
>> Â{
>> + Â Â expires = apply_slack(timer, expires);
>> +
>
> We need to analyse the problem thoroughly and not slap random changes
> into the code without knowing about the consequences. And the problem
> is mostly in the call sites because they are not aware of the slack
> effect.
>
> The sunrpc code is one of those which are affected by the slack magic
> simply because it makes the mod_timer() call basically unconditional
> even if the jiffies value is unchanged.
>
> diff --git a/net/sunrpc/xprt.c b/net/sunrpc/xprt.c
> index ce5eb68..cb0574f 100644
> --- a/net/sunrpc/xprt.c
> +++ b/net/sunrpc/xprt.c
> @@ -1053,10 +1053,12 @@ void xprt_release(struct rpc_task *task)
> Â Â Â Â Â Â Â Âxprt->ops->release_request(task);
> Â Â Â Âif (!list_empty(&req->rq_list))
> Â Â Â Â Â Â Â Âlist_del(&req->rq_list);
> - Â Â Â xprt->last_used = jiffies;
> - Â Â Â if (list_empty(&xprt->recv) && xprt_has_timer(xprt))
> - Â Â Â Â Â Â Â mod_timer(&xprt->timer,
> - Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â xprt->last_used + xprt->idle_timeout);
> + Â Â Â if (xprt->last_used = jiffies) {

Typo? s/=/!=/?

> + Â Â Â Â Â Â Â xprt->last_used = jiffies;
> + Â Â Â Â Â Â Â if (list_empty(&xprt->recv) && xprt_has_timer(xprt))
> + Â Â Â Â Â Â Â Â Â Â Â mod_timer(&xprt->timer,
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â xprt->last_used + xprt->idle_timeout);
> + Â Â Â }
> Â Â Â Âspin_unlock_bh(&xprt->transport_lock);
> Â Â Â Âif (req->rq_buffer)
> Â Â Â Â Â Â Â Âxprt->ops->buf_free(req->rq_buffer);
>
> The above patch does not solve the problem when the resulting new
> timeout is rounded up to the same expiry value after the slack is
> applied, which is not unlikely when jiffies only advanced by a small
> amount.
>
> So we must check after apply_slack() and the reason why the first
> check before apply_slack triggers very often is that auto slack only
> changes the expiry value for timeouts >= 256 jiffies.
>
> And the main caller is the networking code via
> tcp_send_delayed_ack(). The standard delay we see from there is 40ms
> (10 jiffies for HZ=250) and that falls below the 256 jiffies treshold.
>
> The patch below is a reasonable compromise between overhead and
> correctness.

Yup, I think it could smooth Sebastian's issue.

Thanks,
Yong

>
> Thanks,
>
> Â Â Â Âtglx
>
> diff --git a/kernel/timer.c b/kernel/timer.c
> index fd61986..458fd81 100644
> --- a/kernel/timer.c
> +++ b/kernel/timer.c
> @@ -749,16 +749,15 @@ unsigned long apply_slack(struct timer_list *timer, unsigned long expires)
> Â Â Â Âunsigned long expires_limit, mask;
> Â Â Â Âint bit;
>
> - Â Â Â expires_limit = expires;
> -
> Â Â Â Âif (timer->slack >= 0) {
> Â Â Â Â Â Â Â Âexpires_limit = expires + timer->slack;
> Â Â Â Â} else {
> - Â Â Â Â Â Â Â unsigned long now = jiffies;
> + Â Â Â Â Â Â Â long delta = expires - jiffies;
> +
> + Â Â Â Â Â Â Â if (delta < 256)
> + Â Â Â Â Â Â Â Â Â Â Â return expires;
>
> - Â Â Â Â Â Â Â /* No slack, if already expired else auto slack 0.4% */
> - Â Â Â Â Â Â Â if (time_after(expires, now))
> - Â Â Â Â Â Â Â Â Â Â Â expires_limit = expires + (expires - now)/256;
> + Â Â Â Â Â Â Â expires_limit = expires + (expires - now)/256;
> Â Â Â Â}
> Â Â Â Âmask = expires ^ expires_limit;
> Â Â Â Âif (mask == 0)
> @@ -795,6 +794,8 @@ unsigned long apply_slack(struct timer_list *timer, unsigned long expires)
> Â*/
> Âint mod_timer(struct timer_list *timer, unsigned long expires)
> Â{
> + Â Â Â expires = apply_slack(timer, expires);
> +
> Â Â Â Â/*
> Â Â Â Â * This is a common optimization triggered by the
> Â Â Â Â * networking code - if the timer is re-modified
> @@ -803,8 +804,6 @@ int mod_timer(struct timer_list *timer, unsigned long expires)
> Â Â Â Âif (timer_pending(timer) && timer->expires == expires)
> Â Â Â Â Â Â Â Âreturn 1;
>
> - Â Â Â expires = apply_slack(timer, expires);
> -
> Â Â Â Âreturn __mod_timer(timer, expires, false, TIMER_NOT_PINNED);
> Â}
> ÂEXPORT_SYMBOL(mod_timer);
>



-- 
Only stand for myself
--
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/