Re: [PATCH 2/2] mm: compaction: Minimise the time IRQs are disabledwhile isolating pages for migration

[Minchan Kim]

On Wed, Mar 2, 2011 at 1:19 AM, Andrea Arcangeli <aarcange@xxxxxxxxxx> wrote:
> On Wed, Mar 02, 2011 at 12:35:58AM +0900, Minchan Kim wrote:
>> On Tue, Mar 01, 2011 at 01:49:25PM +0900, KAMEZAWA Hiroyuki wrote:
>> > On Tue, 1 Mar 2011 13:11:46 +0900
>> > Minchan Kim <minchan.kim@xxxxxxxxx> wrote:
>> >
>> > > On Tue, Mar 01, 2011 at 08:42:09AM +0900, KAMEZAWA Hiroyuki wrote:
>> > > > On Mon, 28 Feb 2011 10:18:27 +0000
>> > > > Mel Gorman <mel@xxxxxxxxx> wrote:
>> > > >
>> > > > > > BTW, can't we drop disable_irq() from all lru_lock related codes ?
>> > > > > >
>> > > > >
>> > > > > I don't think so - at least not right now. Some LRU operations such as LRU
>> > > > > pagevec draining are run from IPI which is running from an interrupt so
>> > > > > minimally spin_lock_irq is necessary.
>> > > > >
>> > > >
>> > > > pagevec draining is done by workqueue(schedule_on_each_cpu()).
>> > > > I think only racy case is just lru rotation after writeback.
>> > >
>> > > put_page still need irq disable.
>> > >
>> >
>> > Aha..ok. put_page() removes a page from LRU via __page_cache_release().
>> > Then, we may need to remove a page from LRU under irq context.
>> > Hmm...
>>
>> But as __page_cache_release's comment said, normally vm doesn't release page in
>> irq context. so it would be rare.
>> If we can remove it, could we change all of spin_lock_irqsave with spin_lock?
>> If it is right, I think it's very desirable to reduce irq latency.
>>
>> How about this? It's totally a quick implementation and untested.
>> I just want to hear opinions of you guys if the work is valuable or not before
>> going ahead.
>
> pages freed from irq shouldn't be PageLRU.

Hmm..
As looking code, it seems to be no problem and I didn't see the any
comment about such rule. It should have been written down in
__page_cache_release.
Just out of curiosity.
What kinds of problem happen if we release lru page in irq context?

>
> deferring freeing to workqueue doesn't look ok. firewall loads runs
> only from irq and this will cause some more work and a delay in the
> freeing. I doubt it's worhwhile especially for the lru_lock.
>

As you said, if it is for decreasing lock contention in SMP to deliver
overall better performance, maybe we need to check again how much it
helps.
If it doesn't help much, could we remove irq_save/restore of lru_lock?
Do you know any benchmark to prove it had a benefit at that time or
any thread discussing about that in lkml?

Thanks, Andrea.

-- 
Kind regards,
Minchan Kim
--
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/