kill unnecessary bdi wakeups + cleanups
in [Kernel]
|
Prev: [PATCHv5 09/15] writeback: restructure bdi forker loop a little
Next: kill unnecessary SB sync wake-ups + cleanups
From: Artem Bityutskiy on 3 Aug 2010 10:20 On Tue, 2010-08-03 at 14:27 +0200, Jens Axboe wrote: > On 2010-07-25 13:29, Artem Bityutskiy wrote: > > Hi, > > > > here is v6 of the patch series which clean-ups bdi threads and substantially > > lessens amount of unnecessary kernel wake-ups, which is very important on > > battery-powered devices. > > > > This patch-set is also available at: > > git://git.infradead.org/users/dedekind/misc-2.6.git flushers_v6 > > Thanks Artem, for sticking around long enough to get this into > shape. I have finally merged it. > > > 1. Use 'spin_lock_bh' for the 'bdi->wb_lock' (changed patch N12) > > I'd rather not, question is how to avoid it. Either just wakeup the > default thread, or punt the lock-and-check bdi->wb.task to a thread. Jens, here are my quick thoughts, will come back to this tomorrow. The spin_lock_bh(&bdi->wb_lock) in 'wakeup_timer_fn()' is needed: a) to make sure the forker thread does not decide to kill the bdi thread at the same time, which could cause an oops on 'wake_up_process(bdi->wb.task)'. b) to make sure the forker thread does not decide to spawn a bdi thread at the same time, in which case we could lose a wake-up. I without the "_bh" suffix lockdep complains with a warning. Cannot cite the complained, but it is a fair warning about a possible deadlock if the timer function interrupts the CPU while it is already holding the spinlock, or something like that. The easiest way to address it was to use "_bh". The only way to avoid "_bh" I see right now is to not 'bdi->wb_lock' at all in 'wakeup_timer_fn()'. In this case we cannot touch 'bdi->wb.task' because it can become NULL at any point of time. Your first suggestion ("just wakeup the default thread") will work only if we add a new BDI_wakeup_thread or something like that. Not sure it is worth it. The second suggestion ("punt the lock-and-check bdi->wb.task to a thread") is vague. "A thread" - this must be the forker thread, what else could that be? So basically this is the same as the first suggestion - we set a flag in 'bdi->wb.state' and wake up the forker, which should wake up the bdi thread? -- Best Regards, Artem Bityutskiy (Артём Битюцкий) -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Jens Axboe on 4 Aug 2010 07:40 On 2010-08-03 14:47, Jens Axboe wrote: > On 2010-08-03 14:37, Artem Bityutskiy wrote: >> On Tue, 2010-08-03 at 14:27 +0200, Jens Axboe wrote: >>> On 2010-07-25 13:29, Artem Bityutskiy wrote: >>>> Hi, >>>> >>>> here is v6 of the patch series which clean-ups bdi threads and substantially >>>> lessens amount of unnecessary kernel wake-ups, which is very important on >>>> battery-powered devices. >>>> >>>> This patch-set is also available at: >>>> git://git.infradead.org/users/dedekind/misc-2.6.git flushers_v6 >>> >>> Thanks Artem, for sticking around long enough to get this into >>> shape. I have finally merged it. >> >> Thanks, but >> >>>> 1. Use 'spin_lock_bh' for the 'bdi->wb_lock' (changed patch N12) >>> >>> I'd rather not, question is how to avoid it. Either just wakeup the >>> default thread, or punt the lock-and-check bdi->wb.task to a thread. >> >> you merged this change, do you want me to send a separate patch which >> undo the 'spin_lock_bh' change? I'll think about how to avoid this and >> come back. > > Yes, it's not a huge thing, but it would be nice to get rid of. So I > figured it was better to merge it and not have you respin the series yet > again. There is a spinlock bug in the current code, you nest _bh locks on lock but not always on unlock. I fixed it up as per the below: diff --git a/mm/backing-dev.c b/mm/backing-dev.c index 0b8ee66..08d3575 100644 --- a/mm/backing-dev.c +++ b/mm/backing-dev.c @@ -415,7 +415,8 @@ static int bdi_forker_thread(void *ptr) break; } - spin_lock_bh(&bdi->wb_lock); + spin_lock(&bdi->wb_lock); + /* * If there is no work to do and the bdi thread was * inactive long enough - kill it. The wb_lock is taken @@ -432,7 +433,7 @@ static int bdi_forker_thread(void *ptr) action = KILL_THREAD; break; } - spin_unlock_bh(&bdi->wb_lock); + spin_unlock(&bdi->wb_lock); } spin_unlock_bh(&bdi_lock); -- Jens Axboe Confidentiality Notice: This e-mail message, its contents and any attachments to it are confidential to the intended recipient, and may contain information that is privileged and/or exempt from disclosure under applicable law. If you are not the intended recipient, please immediately notify the sender and destroy the original e-mail message and any attachments (and any copies that may have been made) from your system or otherwise. Any unauthorized use, copying, disclosure or distribution of this information is strictly prohibited. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Artem Bityutskiy on 5 Aug 2010 05:40
On Wed, 2010-08-04 at 13:34 +0200, Jens Axboe wrote: > On 2010-08-03 14:47, Jens Axboe wrote: > > On 2010-08-03 14:37, Artem Bityutskiy wrote: > >> On Tue, 2010-08-03 at 14:27 +0200, Jens Axboe wrote: > >>> On 2010-07-25 13:29, Artem Bityutskiy wrote: > >>>> Hi, > >>>> > >>>> here is v6 of the patch series which clean-ups bdi threads and substantially > >>>> lessens amount of unnecessary kernel wake-ups, which is very important on > >>>> battery-powered devices. > >>>> > >>>> This patch-set is also available at: > >>>> git://git.infradead.org/users/dedekind/misc-2.6.git flushers_v6 > >>> > >>> Thanks Artem, for sticking around long enough to get this into > >>> shape. I have finally merged it. > >> > >> Thanks, but > >> > >>>> 1. Use 'spin_lock_bh' for the 'bdi->wb_lock' (changed patch N12) > >>> > >>> I'd rather not, question is how to avoid it. Either just wakeup the > >>> default thread, or punt the lock-and-check bdi->wb.task to a thread. > >> > >> you merged this change, do you want me to send a separate patch which > >> undo the 'spin_lock_bh' change? I'll think about how to avoid this and > >> come back. > > > > Yes, it's not a huge thing, but it would be nice to get rid of. So I > > figured it was better to merge it and not have you respin the series yet > > again. > > There is a spinlock bug in the current code, you nest _bh locks on lock > but not always on unlock. I fixed it up as per the below: Right, sorry, to be frank I never used _bh spinlock versions before, and just did not know the nesting trick. Thanks for fixing. Also, I'm still not sure whether I should get rid of this _bh or not. I wrote my thoughts to you in another e-mail. -- Best Regards, Artem Bityutskiy (Артём Битюцкий) -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ |