Re: [PATCH 1/1] mm/backing-dev.c: Call del_timer_sync instead of del_timer

[kautuk.c @samsung.com]

Hi,

On Mon, Sep 5, 2011 at 9:35 PM, Jan Kara <jack@xxxxxxx> wrote:
>  Hi,
>
> On Mon 05-09-11 20:06:04, kautuk.c @samsung.com wrote:
>> >  OK, I don't care much whether we have there del_timer() or
>> > del_timer_sync(). Let me just say that the race you are afraid of is
>> > probably not going to happen in practice so I'm not sure it's valid to be
>> > afraid of CPU cycles being burned needlessly. The timer is armed when an
>> > dirty inode is first attached to default bdi's dirty list. Then the default
>> > bdi flusher thread would have to be woken up so that following happens:
>> >        CPU1                            CPU2
>> >  timer fires -> wakeup_timer_fn()
>> >                                        bdi_forker_thread()
>> >                                          del_timer(&me->wakeup_timer);
>> >                                          wb_do_writeback(me, 0);
>> >                                          ...
>> >                                          set_current_state(TASK_INTERRUPTIBLE);
>> >  wake_up_process(default_backing_dev_info.wb.task);
>> >
>> >  Especially wb_do_writeback() is going to take a long time so just that
>> > single thing makes the race unlikely. Given del_timer_sync() is slightly
>> > more costly than del_timer() even for unarmed timer, it is questionable
>> > whether (chance race happens * CPU spent in extra loop) > (extra CPU spent
>> > in del_timer_sync() * frequency that code is executed in
>> > bdi_forker_thread())...
>> >
>>
>> Ok, so this means that we can compare the following 2 paths of code:
>> i)   One extra iteration of the bdi_forker_thread loop, versus
>> ii)  The amount of time it takes for the del_timer_sync to wait till the
>> timer_fn on the other CPU finishes executing + schedule resulting in a
>> guaranteed sleep.
>  No, ii) is going to be as rare. But instead you should compare i) against:
> iii) The amount of time it takes del_timer_sync() to check whether the
> timer_fn is running on a different CPU (which is work del_timer() doesn't
> do).

The amount of time it takes del_timer_sync to check the timer_fn should be
negligible.
In fact, try_to_del_timer_sync differs from del_timer_sync in only
that it performs
an additional check:
if (base->running_timer == timer)
    goto out;

>
>  We are going to spend time in iii) each and every time
> if (wb_has_dirty_io(me) || !list_empty(&me->bdi->work_list))
>  evaluates to true.

The amount of time spent on this every time will not matter much, as
the task will
still be preemptible. However, if you notice that in most of the
bdi_forker_thread
loop, we disable preemption due to taking a spinlock so an additional loop there
might be more costly.

>
>  Now frequency of i) and iii) happening is hard to evaluate so it's not
> clear what's going to be better. Certainly I don't think such evaluation is
> worth my time...
>

Ok. Anyways, thanks for explaining all this to me.
I really appreciate your time. :)

>                                                                Honza
> --
> Jan Kara <jack@xxxxxxx>
> SUSE Labs, CR
>
--
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/