lkml.org 
[lkml]   [2010]   [Oct]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: [PATCH] memcg: lock-free clear page writeback (Was Re: [PATCH 04/10] memcg: disable local interrupts in lock_page_cgroup()
    From
    Hi Kame,

    On Thu, Oct 7, 2010 at 3:24 PM, KAMEZAWA Hiroyuki
    <kamezawa.hiroyu@jp.fujitsu.com> wrote:
    > Greg, I think clear_page_writeback() will not require _any_ locks with this patch.
    > But set_page_writeback() requires it...
    > (Maybe adding a special function for clear_page_writeback() is better rather than
    >  adding some complex to switch() in update_page_stat())
    >
    > ==
    > From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
    >
    > Now, at page information accounting, we do lock_page_cgroup() if pc->mem_cgroup
    > points to a cgroup where someone is moving charges from.
    >
    > At supporing dirty-page accounting, one of troubles is writeback bit.
    > In general, writeback can be cleared via IRQ context. To update writeback bit
    > with lock_page_cgroup() in safe way, we'll have to disable IRQ.
    > ....or do something.
    >
    > This patch waits for completion of writeback under lock_page() and do
    > lock_page_cgroup() in safe way. (We never got end_io via IRQ context.)
    >
    > By this, writeback-accounting will never see race with account_move() and
    > it can trust pc->mem_cgroup always _without_ any lock.
    >
    > Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
    > ---
    >  mm/memcontrol.c |   18 ++++++++++++++++++
    >  1 file changed, 18 insertions(+)
    >
    > Index: mmotm-0928/mm/memcontrol.c
    > ===================================================================
    > --- mmotm-0928.orig/mm/memcontrol.c
    > +++ mmotm-0928/mm/memcontrol.c
    > @@ -2183,17 +2183,35 @@ static void __mem_cgroup_move_account(st
    >  /*
    >  * check whether the @pc is valid for moving account and call
    >  * __mem_cgroup_move_account()
    > + * Don't call this under pte_lock etc...we'll do lock_page() and wait for
    > + * the end of I/O.
    >  */
    >  static int mem_cgroup_move_account(struct page_cgroup *pc,
    >                struct mem_cgroup *from, struct mem_cgroup *to, bool uncharge)
    >  {
    >        int ret = -EINVAL;
    > +
    > +       /*
    > +        * We move severl flags and accounting information here. So we need to
    > +        * avoid the races with update_stat routines. For most of routines,
    > +        * lock_page_cgroup() is enough for avoiding race. But we need to take
    > +        * care of IRQ context. If flag updates comes from IRQ context, This
    > +        * "move account" will be racy (and cause deadlock in lock_page_cgroup())
    > +        *
    > +        * Now, the only race we have is Writeback flag. We wait for it cleared
    > +        * before starting our jobs.
    > +        */
    > +
    > +       lock_page(pc->page);
    > +       wait_on_page_writeback(pc->page);
    > +
    >        lock_page_cgroup(pc);
    >        if (PageCgroupUsed(pc) && pc->mem_cgroup == from) {
    >                __mem_cgroup_move_account(pc, from, to, uncharge);
    >                ret = 0;
    >        }
    >        unlock_page_cgroup(pc);
    > +       unlock_page(pc->page);
    >        /*
    >         * check events
    >         */
    >
    >

    Looks good to me.
    But let me ask a question.
    Why do only move_account need this logic?
    Is deadlock candidate is only this place?
    How about mem_cgroup_prepare_migration?

    unmap_and_move
    lock_page
    mem_cgroup_prepare_migration
    lock_page_cgroup
    ...
    softirq happen
    lock_page_cgroup


    If race happens only where move_account and writeback, please describe
    it as comment.
    It would help to review the code in future.

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

    \
     
     \ /
      Last update: 2010-10-08 01:37    [W:4.030 / U:0.160 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site