Messages in this thread | | | Date | Fri, 18 Jul 2014 11:07:19 -0400 | From | Johannes Weiner <> | Subject | Re: [patch] mm: memcontrol: use page lists for uncharge batching |
| |
On Thu, Jul 17, 2014 at 05:29:36PM +0200, Michal Hocko wrote: > On Mon 07-07-14 14:55:58, Johannes Weiner wrote: > > Pages are now uncharged at release time, and all sources of batched > > uncharges operate on lists of pages. Directly use those lists, and > > get rid of the per-task batching state. > > > > This also batches statistics accounting, in addition to the res > > counter charges, to reduce IRQ-disabling and re-enabling. > > It is probably worth noticing that there is a higher chance of missing > threshold events now when we can accumulate huge number of uncharges > during munmaps. I do not think this is earth shattering and the overall > improvement is worth it but changelog should mention it.
Does this actually matter, though? We might deliver events a few pages later than before, but as I read the threshold code, once invoked it catches up from the last delivered threshold to the new usage. So we shouldn't *miss* any events.
> > Signed-off-by: Johannes Weiner <hannes@cmpxchg.org> > > With the follow up fix from > http://marc.info/?l=linux-mm&m=140552814228135&w=2 > > Acked-by: Michal Hocko <mhocko@suse.cz>
Thanks!
> > +static void uncharge_list(struct list_head *page_list) > > +{ > > + struct mem_cgroup *memcg = NULL; > > + unsigned long nr_memsw = 0; > > + unsigned long nr_anon = 0; > > + unsigned long nr_file = 0; > > + unsigned long nr_huge = 0; > > + unsigned long pgpgout = 0; > > + unsigned long nr_mem = 0; > > + struct list_head *next; > > + struct page *page; > > + > > + next = page_list->next; > > + do { > > I would use list_for_each_entry here which would also save list_empty > check in mem_cgroup_uncharge_list
list_for_each_entry() wouldn't work for the singleton list where we pass in page->lru. That's why it's a do-while that always does the first page before checking whether it looped back to the list head.
Do we need a comment for that? I'm not convinced, there are only two callsites, and the one that passes the singleton page->lru is right below this function.
| |