lkml.org 
[lkml]   [2011]   [Sep]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH] memcg: remove unneeded preempt_disable
    On Tue, Sep 6, 2011 at 2:58 AM, Johannes Weiner <jweiner@redhat.com> wrote:
    > On Wed, Aug 17, 2011 at 11:50:53PM -0700, Greg Thelen wrote:
    >> Both mem_cgroup_charge_statistics() and mem_cgroup_move_account() were
    >> unnecessarily disabling preemption when adjusting per-cpu counters:
    >>     preempt_disable()
    >>     __this_cpu_xxx()
    >>     __this_cpu_yyy()
    >>     preempt_enable()
    >>
    >> This change does not disable preemption and thus CPU switch is possible
    >> within these routines.  This does not cause a problem because the total
    >> of all cpu counters is summed when reporting stats.  Now both
    >> mem_cgroup_charge_statistics() and mem_cgroup_move_account() look like:
    >>     this_cpu_xxx()
    >>     this_cpu_yyy()
    >>
    >> Reported-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
    >> Signed-off-by: Greg Thelen <gthelen@google.com>
    >
    > I just noticed that both cases have preemption disabled anyway because
    > of the page_cgroup bit spinlock.
    >
    > So removing the preempt_disable() is fine but we can even keep the
    > non-atomic __this_cpu operations.
    >
    > Something like this instead?
    >
    > ---
    > From: Johannes Weiner <jweiner@redhat.com>
    > Subject: mm: memcg: remove needless recursive preemption disabling
    >
    > Callsites of mem_cgroup_charge_statistics() hold the page_cgroup bit
    > spinlock, which implies disabled preemption.
    >
    > The same goes for the explicit preemption disabling to account mapped
    > file pages in mem_cgroup_move_account().
    >
    > The explicit disabling of preemption in both cases is redundant.
    >
    > Signed-off-by: Johannes Weiner <jweiner@redhat.com>

    Looks good, thanks.

    Reviewed-by: Greg Thelen <gthelen@google.com>

    > ---
    >  mm/memcontrol.c |    6 ------
    >  1 file changed, 6 deletions(-)
    >
    > --- a/mm/memcontrol.c
    > +++ b/mm/memcontrol.c
    > @@ -618,8 +618,6 @@ static unsigned long mem_cgroup_read_eve
    >  static void mem_cgroup_charge_statistics(struct mem_cgroup *mem,
    >                                         bool file, int nr_pages)
    >  {
    > -       preempt_disable();
    > -
    >        if (file)
    >                __this_cpu_add(mem->stat->count[MEM_CGROUP_STAT_CACHE], nr_pages);
    >        else
    > @@ -634,8 +632,6 @@ static void mem_cgroup_charge_statistics
    >        }
    >
    >        __this_cpu_add(mem->stat->events[MEM_CGROUP_EVENTS_COUNT], nr_pages);
    > -
    > -       preempt_enable();
    >  }
    >
    >  unsigned long
    > @@ -2582,10 +2578,8 @@ static int mem_cgroup_move_account(struc
    >
    >        if (PageCgroupFileMapped(pc)) {
    >                /* Update mapped_file data for mem_cgroup */
    > -               preempt_disable();
    >                __this_cpu_dec(from->stat->count[MEM_CGROUP_STAT_FILE_MAPPED]);
    >                __this_cpu_inc(to->stat->count[MEM_CGROUP_STAT_FILE_MAPPED]);
    > -               preempt_enable();
    >        }
    >        mem_cgroup_charge_statistics(from, PageCgroupCache(pc), -nr_pages);
    >        if (uncharge)
    >
    --
    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: 2011-09-06 20:07    [W:0.027 / U:119.976 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site