On Tue 06-09-11 09:41:42, kautuk.c @samsung.com wrote: > > 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; Yes, but the probability the race happens is also negligible. So you are comparing two negligible things... > > 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. So either you speak about CPU cost in amount of cycles spent - and there I still don't buy that it's clear del_timer_sync() is better than del_timer() - or you speak about latency which is a different thing. From latency POV that additional loop might be worse. But still I don't think it's clear enough to change it without any measurement... > > 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. :) You are welcome. You made me refresh my memory about some parts of kernel which is also valuable so thanks goes also to you :) Honza -- Jan Kara <jack@xxxxxxx> SUSE Labs, CR -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@xxxxxxxxx. For more info on Linux MM, see: http://www.linux-mm.org/ . Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/ Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>