Messages in this thread | | | Date | Fri, 2 Sep 2011 17:14:05 +0530 | Subject | Re: [PATCH 1/1] mm/backing-dev.c: Call del_timer_sync instead of del_timer | From | "kautuk.c @samsung.com" <> |
| |
Hi,
On Fri, Sep 2, 2011 at 4:51 PM, Jan Kara <jack@suse.cz> wrote: > Hello, > > On Fri 02-09-11 10:47:03, kautuk.c @samsung.com wrote: >> On Fri, Sep 2, 2011 at 3:03 AM, Andrew Morton <akpm@linux-foundation.org> wrote: >> > On Thu, 1 Sep 2011 21:27:02 +0530 >> > Kautuk Consul <consul.kautuk@gmail.com> wrote: >> > >> >> This is important for SMP scenario, to check whether the timer >> >> callback is executing on another CPU when we are deleting the >> >> timer. >> >> >> > >> > I don't see why? >> > >> >> index d6edf8d..754b35a 100644 >> >> --- a/mm/backing-dev.c >> >> +++ b/mm/backing-dev.c >> >> @@ -385,7 +385,7 @@ static int bdi_forker_thread(void *ptr) >> >> * dirty data on the default backing_dev_info >> >> */ >> >> if (wb_has_dirty_io(me) || !list_empty(&me->bdi->work_list)) { >> >> - del_timer(&me->wakeup_timer); >> >> + del_timer_sync(&me->wakeup_timer); >> >> wb_do_writeback(me, 0); >> >> } >> > >> > It isn't a use-after-free fix: bdi_unregister() safely shoots down any >> > running timer. >> > >> >> In the situation that we do a del_timer at the same time that the >> wakeup_timer_fn is >> executing on another CPU, there is one tiny possible problem: >> 1) The wakeup_timer_fn will call wake_up_process on the bdi-default thread. >> This will set the bdi-default thread's state to TASK_RUNNING. >> 2) However, the code in bdi_writeback_thread() sets the state of the >> bdi-default process >> to TASK_INTERRUPTIBLE as it intends to sleep later. >> >> If 2) happens before 1), then the bdi_forker_thread will not sleep >> inside schedule as is the intention of the bdi_forker_thread() code. > OK, I agree the code in bdi_forker_thread() might use some straightening > up wrt. task state handling but is what you decribe really an issue? Sure > the task won't go to sleep but the whole effect is that it will just loop > once more to find out there's nothing to do and then go to sleep - not a > bug deal... Or am I missing something?
Yes, you are right. I was studying the code and I found this inconsistency. Anyways, if there is NO_ACTION it will just loop and go to sleep again. I just posted this because I felt that the code was not achieving the logic that was intended in terms of sleeps and wakeups.
I am currently trying to study the other patches you have just sent.
> >> This protection is not achieved even by acquiring spinlocks before >> setting the task->state >> as the spinlock used in wakeup_timer_fn is &bdi->wb_lock whereas the code in >> bdi_forker_thread acquires &bdi_lock which is a different spin_lock. >> >> Am I correct in concluding this ? > > Honza > -- > Jan Kara <jack@suse.cz> > SUSE Labs, CR > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |