Re: [PATCH V5 7/8] memcg: don't account root memcg page stats if only root exists

On Fri, Aug 02, 2013 at 12:32:17PM +0800, Sha Zhengju wrote:

On Fri, Aug 2, 2013 at 12:20 AM, Johannes Weiner [off-list ref] wrote:

quoted

On Thu, Aug 01, 2013 at 08:00:07PM +0800, Sha Zhengju wrote:

quoted

@@ -6303,6 +6360,49 @@ mem_cgroup_css_online(struct cgroup *cont)
      }

      error = memcg_init_kmem(memcg, &mem_cgroup_subsys);
+     if (!error) {
+             if (!mem_cgroup_in_use()) {
+                     /* I'm the first non-root memcg, move global stats to root memcg.
+                      * Memcg creating is serialized by cgroup locks(cgroup_mutex),
+                      * so the mem_cgroup_in_use() checking is safe.
+                      *
+                      * We use global_page_state() to get global page stats, but
+                      * because of the optimized inc/dec functions in SMP while
+                      * updating each zone's stats, We may lose some numbers
+                      * in a stock(zone->pageset->vm_stat_diff) which brings some
+                      * inaccuracy. But places where kernel use these page stats to
+                      * steer next decision e.g. dirty page throttling or writeback
+                      * also use global_page_state(), so here it's enough too.
+                      */
+                     spin_lock(&root_mem_cgroup->pcp_counter_lock);
+                     root_mem_cgroup->stats_base.count[MEM_CGROUP_STAT_FILE_MAPPED] =
+                                             global_page_state(NR_FILE_MAPPED);
+                     root_mem_cgroup->stats_base.count[MEM_CGROUP_STAT_FILE_DIRTY] =
+                                             global_page_state(NR_FILE_DIRTY);
+                     root_mem_cgroup->stats_base.count[MEM_CGROUP_STAT_WRITEBACK] =
+                                             global_page_state(NR_WRITEBACK);
+                     spin_unlock(&root_mem_cgroup->pcp_counter_lock);
+             }

If inaccuracies in these counters are okay, why do we go through an
elaborate locking scheme that sprinkles memcg callbacks everywhere
just to be 100% reliable in the rare case somebody moves memory
between cgroups?

IMO they are not the same thing. Moving between cgroups may happen
many times, if we ignore the inaccuracy between moving, then the
cumulative inaccuracies caused by this back and forth behavior can
become very large.

But the transferring above occurs only once since we do it only when
the first non-root memcg creating, so the error is at most
zone->pageset->stat_threshold. This threshold depends on the amount of
zone memory and  the number of processors, and its maximum value is
125, so I thought using global_page_state() is enough. Of course we
can add the stock to seek for greater perfection, but there are also
possibilities of inaccuracy because of racy.

File pages may get unmapped/dirtied/put under writeback/finish
writeback between reading the stats and arming the inuse keys (before
which you are not collecting any percpu deltas).

The error is not from the percpu inaccuracies but because you don't
snapshot the counters and start accounting changes atomically wrt
ongoing counter modificatcions.  This means the error is unbounded.