Re: [PATCH 6/10] mm/memcg: take care over pc->mem_cgroup

[Hugh Dickins]

On Wed, 22 Feb 2012, Konstantin Khlebnikov wrote:
> Hugh Dickins wrote:
> > 
> > As things stand, that would mean lock_page_cgroup() has to disable irqs
> > everywhere.  I'm not sure of the further ramifications of moving uncharge
> > to __page_cache_release() and release_pages().  I don't think a change
> > like that is out of the question, but it's certainly a bigger change
> > than I'd like to consider in this series.
> 
> Ok. I have another big question: Why we remove pages from lru at last
> put_page()?
> 
> Logically we can remove them in truncate_inode_pages_range() for file
> and in free_pages_and_swap_cache() or something at last unmap for anon.
> Pages are unreachable after that, they never become alive again.
> Reclaimer also cannot reclaim them in this state, so there no reasons for
> keeping them in lru.
> Into those two functions pages come in large batches, so we can remove them
> more effectively,
> currently they are likely to be removed right in this place, just because
> release_pages() drops
> last references, but we can do this lru remove unconditionally.

That may be a very good idea, but I'm not going to commit myself
in a hurry.

I think Kamezawa-san was involved, and has a much better grasp than
I have, of the choices of precisely when to charge and uncharge;
and why we would not have removed from lru at the point of uncharge.

There may have been lock ordering reasons, now gone away, why it could
not have been done.  Or they may just have been the overriding reason,
now going away, that memcg should not make any change to what already
was happening without memcg.

One difficulty that comes to mind, is that at the point of uncharge,
the page may be (temporarily) off lru already: what then?  We certainly
don't want the uncharge to wait until the page comes back on to lru.
But it should be possible to deal with, by just making everywhere that
puts a page back on lru check for the charge first.  Hmm, but then
what of non-memcg, where there is never any charge?  And what of
those swapin readahead pages?  I think what you were suggesting
is probably slightly different from what I went on to imagine.

Please keep this idea in mind: maybe Kamezawa will immediately point
out the fatal flaw in it, or maybe we should come back to it later -
I'm not getting deeper into it now.

> Plus it never happens in irq context, so lru_lock can be converted to
> irq-unsafe in some distant future.

I'd love that: no very strong reason, the irq-disabling just irritates
me.  But note that the irq-disabling was introduced by Andrew, not for
I/O completion reasons (those somehow followed later IIRC), but because
the lock was so contended that he didn't want the holders interrupted.
Though I've not seen such a justification used recently.

We'd also have to do something about the "rotation",
maybe Mel's separate list would help, maybe not.

Hugh
--
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/