From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751641AbdFOPfj (ORCPT ); Thu, 15 Jun 2017 11:35:39 -0400 Received: from mx1.redhat.com ([209.132.183.28]:53648 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750737AbdFOPfh (ORCPT ); Thu, 15 Jun 2017 11:35:37 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 48FED7F6A1 Authentication-Results: ext-mx01.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx01.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=jglisse@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com 48FED7F6A1 Date: Thu, 15 Jun 2017 11:35:34 -0400 From: Jerome Glisse To: Balbir Singh Cc: linux-kernel@vger.kernel.org, linux-mm@kvack.org, John Hubbard , David Nellans , Johannes Weiner , Michal Hocko , Vladimir Davydov , cgroups@vger.kernel.org Subject: Re: [HMM-CDM 3/5] mm/memcontrol: allow to uncharge page without using page->lru field Message-ID: <20170615153533.GA3837@redhat.com> References: <20170614201144.9306-1-jglisse@redhat.com> <20170614201144.9306-4-jglisse@redhat.com> <20170615133128.2fe2c33f@firefly.ozlabs.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20170615133128.2fe2c33f@firefly.ozlabs.ibm.com> User-Agent: Mutt/1.7.1 (2016-10-04) X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.25]); Thu, 15 Jun 2017 15:35:37 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Jun 15, 2017 at 01:31:28PM +1000, Balbir Singh wrote: > On Wed, 14 Jun 2017 16:11:42 -0400 > Jérôme Glisse wrote: > > > HMM pages (private or public device pages) are ZONE_DEVICE page and > > thus you can not use page->lru fields of those pages. This patch > > re-arrange the uncharge to allow single page to be uncharge without > > modifying the lru field of the struct page. > > > > There is no change to memcontrol logic, it is the same as it was > > before this patch. > > > > Signed-off-by: Jérôme Glisse > > Cc: Johannes Weiner > > Cc: Michal Hocko > > Cc: Vladimir Davydov > > Cc: cgroups@vger.kernel.org > > --- > > mm/memcontrol.c | 168 +++++++++++++++++++++++++++++++------------------------- > > 1 file changed, 92 insertions(+), 76 deletions(-) > > > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > > index e3fe4d0..b93f5fe 100644 > > --- a/mm/memcontrol.c > > +++ b/mm/memcontrol.c > > @@ -5509,48 +5509,102 @@ void mem_cgroup_cancel_charge(struct page *page, struct mem_cgroup *memcg, > > cancel_charge(memcg, nr_pages); > > } > > > > -static void uncharge_batch(struct mem_cgroup *memcg, unsigned long pgpgout, > > - unsigned long nr_anon, unsigned long nr_file, > > - unsigned long nr_kmem, unsigned long nr_huge, > > - unsigned long nr_shmem, struct page *dummy_page) > > +struct uncharge_gather { > > + struct mem_cgroup *memcg; > > + unsigned long pgpgout; > > + unsigned long nr_anon; > > + unsigned long nr_file; > > + unsigned long nr_kmem; > > + unsigned long nr_huge; > > + unsigned long nr_shmem; > > + struct page *dummy_page; > > +}; > > + > > +static inline void uncharge_gather_clear(struct uncharge_gather *ug) > > { > > - unsigned long nr_pages = nr_anon + nr_file + nr_kmem; > > + memset(ug, 0, sizeof(*ug)); > > +} > > + > > +static void uncharge_batch(const struct uncharge_gather *ug) > > +{ > > Can we pass page as an argument so that we can do check events on the page? Well it is what dummy page is for, i wanted to keep code as close to existing to make it easier for people to see that there was no logic change with that patch. [...] > > +static void uncharge_page(struct page *page, struct uncharge_gather *ug) > > +{ > > + VM_BUG_ON_PAGE(PageLRU(page), page); > > + VM_BUG_ON_PAGE(!PageHWPoison(page) && page_count(page), page); > > + > > + if (!page->mem_cgroup) > > + return; > > + > > + /* > > + * Nobody should be changing or seriously looking at > > + * page->mem_cgroup at this point, we have fully > > + * exclusive access to the page. > > + */ > > + > > + if (ug->memcg != page->mem_cgroup) { > > + if (ug->memcg) { > > + uncharge_batch(ug); > > What is ug->dummy_page set to at this point? ug->dummy_page is assigned below So at begining ug->memcg is NULL and so is ug->dummy_page, after first call to uncharge_page() if ug->memcg isn't NULL then ug->dummy_page points to a valid page. So uncharge_batch() can never be call with dummy_page == NULL same as before this patch. [...] > > static void uncharge_list(struct list_head *page_list) > > { > > - struct mem_cgroup *memcg = NULL; > > - unsigned long nr_shmem = 0; > > - unsigned long nr_anon = 0; > > - unsigned long nr_file = 0; > > - unsigned long nr_huge = 0; > > - unsigned long nr_kmem = 0; > > - unsigned long pgpgout = 0; > > + struct uncharge_gather ug; > > struct list_head *next; > > - struct page *page; > > + > > + uncharge_gather_clear(&ug); > > > > /* > > * Note that the list can be a single page->lru; hence the > > @@ -5558,57 +5612,16 @@ static void uncharge_list(struct list_head *page_list) > > */ > > next = page_list->next; > > do { > > + struct page *page; > > + > > Nit pick > > VM_WARN_ON(is_zone_device_page(page)); Yeah probably good thing to add. I will add it as part of the other memcontrol patch as i want to keep this one about moving stuff around with no logic change. Thanks for reviewing Jérôme