Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1666194 > unrolled thread
| Started by | Jérôme Glisse <jglisse@redhat.com> |
|---|---|
| First post | 2017-06-14 22:20 +0200 |
| Last post | 2017-06-15 05:20 +0200 |
| Articles | 4 — 3 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
[HMM-CDM 4/5] mm/memcontrol: support MEMORY_DEVICE_PRIVATE and MEMORY_DEVICE_PUBLIC Jérôme Glisse <jglisse@redhat.com> - 2017-06-14 22:20 +0200
Re: [HMM-CDM 4/5] mm/memcontrol: support MEMORY_DEVICE_PRIVATE and MEMORY_DEVICE_PUBLIC Balbir Singh <bsingharora@gmail.com> - 2017-06-15 03:50 +0200
Re: [HMM-CDM 4/5] mm/memcontrol: support MEMORY_DEVICE_PRIVATE and MEMORY_DEVICE_PUBLIC Jerome Glisse <jglisse@redhat.com> - 2017-06-15 04:10 +0200
Re: [HMM-CDM 4/5] mm/memcontrol: support MEMORY_DEVICE_PRIVATE and MEMORY_DEVICE_PUBLIC Balbir Singh <bsingharora@gmail.com> - 2017-06-15 05:20 +0200
| From | Jérôme Glisse <jglisse@redhat.com> |
|---|---|
| Date | 2017-06-14 22:20 +0200 |
| Subject | [HMM-CDM 4/5] mm/memcontrol: support MEMORY_DEVICE_PRIVATE and MEMORY_DEVICE_PUBLIC |
| Message-ID | <tSmVH-1k9-7@gated-at.bofh.it> |
HMM pages (private or public device pages) are ZONE_DEVICE page and
thus need special handling when it comes to lru or refcount. This
patch make sure that memcontrol properly handle those when it face
them. Those pages are use like regular pages in a process address
space either as anonymous page or as file back page. So from memcg
point of view we want to handle them like regular page for now at
least.
Signed-off-by: Jérôme Glisse <jglisse@redhat.com>
Cc: Johannes Weiner <hannes@cmpxchg.org>
Cc: Michal Hocko <mhocko@kernel.org>
Cc: Vladimir Davydov <vdavydov.dev@gmail.com>
Cc: cgroups@vger.kernel.org
---
kernel/memremap.c | 2 ++
mm/memcontrol.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++-----
2 files changed, 55 insertions(+), 5 deletions(-)
diff --git a/kernel/memremap.c b/kernel/memremap.c
index da74775..584984c 100644
--- a/kernel/memremap.c
+++ b/kernel/memremap.c
@@ -479,6 +479,8 @@ void put_zone_device_private_or_public_page(struct page *page)
__ClearPageActive(page);
__ClearPageWaiters(page);
+ mem_cgroup_uncharge(page);
+
page->pgmap->page_free(page, page->pgmap->data);
}
else if (!count)
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index b93f5fe..171b638 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -4391,12 +4391,13 @@ enum mc_target_type {
MC_TARGET_NONE = 0,
MC_TARGET_PAGE,
MC_TARGET_SWAP,
+ MC_TARGET_DEVICE,
};
static struct page *mc_handle_present_pte(struct vm_area_struct *vma,
unsigned long addr, pte_t ptent)
{
- struct page *page = vm_normal_page(vma, addr, ptent);
+ struct page *page = _vm_normal_page(vma, addr, ptent, true);
if (!page || !page_mapped(page))
return NULL;
@@ -4407,13 +4408,20 @@ static struct page *mc_handle_present_pte(struct vm_area_struct *vma,
if (!(mc.flags & MOVE_FILE))
return NULL;
}
- if (!get_page_unless_zero(page))
+ if (is_device_public_page(page)) {
+ /*
+ * MEMORY_DEVICE_PUBLIC means ZONE_DEVICE page and which have a
+ * refcount of 1 when free (unlike normal page)
+ */
+ if (!page_ref_add_unless(page, 1, 1))
+ return NULL;
+ } else if (!get_page_unless_zero(page))
return NULL;
return page;
}
-#ifdef CONFIG_SWAP
+#if defined(CONFIG_SWAP) || defined(CONFIG_DEVICE_PRIVATE)
static struct page *mc_handle_swap_pte(struct vm_area_struct *vma,
pte_t ptent, swp_entry_t *entry)
{
@@ -4422,6 +4430,23 @@ static struct page *mc_handle_swap_pte(struct vm_area_struct *vma,
if (!(mc.flags & MOVE_ANON) || non_swap_entry(ent))
return NULL;
+
+ /*
+ * Handle MEMORY_DEVICE_PRIVATE which are ZONE_DEVICE page belonging to
+ * a device and because they are not accessible by CPU they are store
+ * as special swap entry in the CPU page table.
+ */
+ if (is_device_private_entry(ent)) {
+ page = device_private_entry_to_page(ent);
+ /*
+ * MEMORY_DEVICE_PRIVATE means ZONE_DEVICE page and which have
+ * a refcount of 1 when free (unlike normal page)
+ */
+ if (!page_ref_add_unless(page, 1, 1))
+ return NULL;
+ return page;
+ }
+
/*
* Because lookup_swap_cache() updates some statistics counter,
* we call find_get_page() with swapper_space directly.
@@ -4582,6 +4607,8 @@ static int mem_cgroup_move_account(struct page *page,
* 2(MC_TARGET_SWAP): if the swap entry corresponding to this pte is a
* target for charge migration. if @target is not NULL, the entry is stored
* in target->ent.
+ * 3(MC_TARGET_DEVICE): like MC_TARGET_PAGE but page is MEMORY_DEVICE_PUBLIC
+ * or MEMORY_DEVICE_PRIVATE (so ZONE_DEVICE page and thus not on the lru).
*
* Called with pte lock held.
*/
@@ -4610,6 +4637,9 @@ static enum mc_target_type get_mctgt_type(struct vm_area_struct *vma,
*/
if (page->mem_cgroup == mc.from) {
ret = MC_TARGET_PAGE;
+ if (is_device_private_page(page) ||
+ is_device_public_page(page))
+ ret = MC_TARGET_DEVICE;
if (target)
target->page = page;
}
@@ -4669,6 +4699,11 @@ static int mem_cgroup_count_precharge_pte_range(pmd_t *pmd,
ptl = pmd_trans_huge_lock(pmd, vma);
if (ptl) {
+ /*
+ * Note their can not be MC_TARGET_DEVICE for now as we do not
+ * support transparent huge page with MEMORY_DEVICE_PUBLIC or
+ * MEMORY_DEVICE_PRIVATE but this might change.
+ */
if (get_mctgt_type_thp(vma, addr, *pmd, NULL) == MC_TARGET_PAGE)
mc.precharge += HPAGE_PMD_NR;
spin_unlock(ptl);
@@ -4884,6 +4919,14 @@ static int mem_cgroup_move_charge_pte_range(pmd_t *pmd,
putback_lru_page(page);
}
put_page(page);
+ } else if (target_type == MC_TARGET_DEVICE) {
+ page = target.page;
+ if (!mem_cgroup_move_account(page, true,
+ mc.from, mc.to)) {
+ mc.precharge -= HPAGE_PMD_NR;
+ mc.moved_charge += HPAGE_PMD_NR;
+ }
+ put_page(page);
}
spin_unlock(ptl);
return 0;
@@ -4895,12 +4938,16 @@ static int mem_cgroup_move_charge_pte_range(pmd_t *pmd,
pte = pte_offset_map_lock(vma->vm_mm, pmd, addr, &ptl);
for (; addr != end; addr += PAGE_SIZE) {
pte_t ptent = *(pte++);
+ bool device = false;
swp_entry_t ent;
if (!mc.precharge)
break;
switch (get_mctgt_type(vma, addr, ptent, &target)) {
+ case MC_TARGET_DEVICE:
+ device = true;
+ /* fall through */
case MC_TARGET_PAGE:
page = target.page;
/*
@@ -4911,7 +4958,7 @@ static int mem_cgroup_move_charge_pte_range(pmd_t *pmd,
*/
if (PageTransCompound(page))
goto put;
- if (isolate_lru_page(page))
+ if (!device && isolate_lru_page(page))
goto put;
if (!mem_cgroup_move_account(page, false,
mc.from, mc.to)) {
@@ -4919,7 +4966,8 @@ static int mem_cgroup_move_charge_pte_range(pmd_t *pmd,
/* we uncharge from mc.from later. */
mc.moved_charge++;
}
- putback_lru_page(page);
+ if (!device)
+ putback_lru_page(page);
put: /* get_mctgt_type() gets the page */
put_page(page);
break;
--
2.9.3
[toc] | [next] | [standalone]
| From | Balbir Singh <bsingharora@gmail.com> |
|---|---|
| Date | 2017-06-15 03:50 +0200 |
| Subject | Re: [HMM-CDM 4/5] mm/memcontrol: support MEMORY_DEVICE_PRIVATE and MEMORY_DEVICE_PUBLIC |
| Message-ID | <tSs54-4pz-7@gated-at.bofh.it> |
| In reply to | #1666194 |
On Wed, 14 Jun 2017 16:11:43 -0400
Jérôme Glisse <jglisse@redhat.com> wrote:
> HMM pages (private or public device pages) are ZONE_DEVICE page and
> thus need special handling when it comes to lru or refcount. This
> patch make sure that memcontrol properly handle those when it face
> them. Those pages are use like regular pages in a process address
> space either as anonymous page or as file back page. So from memcg
> point of view we want to handle them like regular page for now at
> least.
>
> Signed-off-by: Jérôme Glisse <jglisse@redhat.com>
> Cc: Johannes Weiner <hannes@cmpxchg.org>
> Cc: Michal Hocko <mhocko@kernel.org>
> Cc: Vladimir Davydov <vdavydov.dev@gmail.com>
> Cc: cgroups@vger.kernel.org
> ---
> kernel/memremap.c | 2 ++
> mm/memcontrol.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++-----
> 2 files changed, 55 insertions(+), 5 deletions(-)
>
> diff --git a/kernel/memremap.c b/kernel/memremap.c
> index da74775..584984c 100644
> --- a/kernel/memremap.c
> +++ b/kernel/memremap.c
> @@ -479,6 +479,8 @@ void put_zone_device_private_or_public_page(struct page *page)
> __ClearPageActive(page);
> __ClearPageWaiters(page);
>
> + mem_cgroup_uncharge(page);
> +
A zone device page could have a mem_cgroup charge if
1. The old page was charged to a cgroup and the new page from ZONE_DEVICE then
gets the charge that we need to drop here
And should not be charged
2. If the driver allowed mmap based allocation (these pages are not on LRU
Since put_zone_device_private_or_public_page() is called from release_pages(),
I think the assumption is that 2 is not a problem? I've not tested the mmap
bits yet.
> page->pgmap->page_free(page, page->pgmap->data);
> }
> else if (!count)
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index b93f5fe..171b638 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -4391,12 +4391,13 @@ enum mc_target_type {
> MC_TARGET_NONE = 0,
> MC_TARGET_PAGE,
> MC_TARGET_SWAP,
> + MC_TARGET_DEVICE,
> };
>
> static struct page *mc_handle_present_pte(struct vm_area_struct *vma,
> unsigned long addr, pte_t ptent)
> {
> - struct page *page = vm_normal_page(vma, addr, ptent);
> + struct page *page = _vm_normal_page(vma, addr, ptent, true);
>
> if (!page || !page_mapped(page))
> return NULL;
> @@ -4407,13 +4408,20 @@ static struct page *mc_handle_present_pte(struct vm_area_struct *vma,
> if (!(mc.flags & MOVE_FILE))
> return NULL;
> }
> - if (!get_page_unless_zero(page))
> + if (is_device_public_page(page)) {
> + /*
> + * MEMORY_DEVICE_PUBLIC means ZONE_DEVICE page and which have a
> + * refcount of 1 when free (unlike normal page)
> + */
> + if (!page_ref_add_unless(page, 1, 1))
> + return NULL;
> + } else if (!get_page_unless_zero(page))
> return NULL;
>
> return page;
> }
>
> -#ifdef CONFIG_SWAP
> +#if defined(CONFIG_SWAP) || defined(CONFIG_DEVICE_PRIVATE)
> static struct page *mc_handle_swap_pte(struct vm_area_struct *vma,
> pte_t ptent, swp_entry_t *entry)
> {
> @@ -4422,6 +4430,23 @@ static struct page *mc_handle_swap_pte(struct vm_area_struct *vma,
>
> if (!(mc.flags & MOVE_ANON) || non_swap_entry(ent))
> return NULL;
> +
> + /*
> + * Handle MEMORY_DEVICE_PRIVATE which are ZONE_DEVICE page belonging to
> + * a device and because they are not accessible by CPU they are store
> + * as special swap entry in the CPU page table.
> + */
> + if (is_device_private_entry(ent)) {
> + page = device_private_entry_to_page(ent);
> + /*
> + * MEMORY_DEVICE_PRIVATE means ZONE_DEVICE page and which have
> + * a refcount of 1 when free (unlike normal page)
> + */
> + if (!page_ref_add_unless(page, 1, 1))
> + return NULL;
> + return page;
> + }
> +
> /*
> * Because lookup_swap_cache() updates some statistics counter,
> * we call find_get_page() with swapper_space directly.
> @@ -4582,6 +4607,8 @@ static int mem_cgroup_move_account(struct page *page,
> * 2(MC_TARGET_SWAP): if the swap entry corresponding to this pte is a
> * target for charge migration. if @target is not NULL, the entry is stored
> * in target->ent.
> + * 3(MC_TARGET_DEVICE): like MC_TARGET_PAGE but page is MEMORY_DEVICE_PUBLIC
> + * or MEMORY_DEVICE_PRIVATE (so ZONE_DEVICE page and thus not on the lru).
> *
> * Called with pte lock held.
> */
> @@ -4610,6 +4637,9 @@ static enum mc_target_type get_mctgt_type(struct vm_area_struct *vma,
> */
> if (page->mem_cgroup == mc.from) {
> ret = MC_TARGET_PAGE;
> + if (is_device_private_page(page) ||
> + is_device_public_page(page))
> + ret = MC_TARGET_DEVICE;
> if (target)
> target->page = page;
> }
> @@ -4669,6 +4699,11 @@ static int mem_cgroup_count_precharge_pte_range(pmd_t *pmd,
>
> ptl = pmd_trans_huge_lock(pmd, vma);
> if (ptl) {
> + /*
> + * Note their can not be MC_TARGET_DEVICE for now as we do not
there
> + * support transparent huge page with MEMORY_DEVICE_PUBLIC or
> + * MEMORY_DEVICE_PRIVATE but this might change.
I am trying to remind myself why THP and MEMORY_DEVICE_* pages don't work well
together today, the driver could allocate a THP size set of pages and migrate it.
There are patches to do THP migration, not upstream yet. Could you remind me
of any other limitations?
Balbir Singh
[toc] | [prev] | [next] | [standalone]
| From | Jerome Glisse <jglisse@redhat.com> |
|---|---|
| Date | 2017-06-15 04:10 +0200 |
| Subject | Re: [HMM-CDM 4/5] mm/memcontrol: support MEMORY_DEVICE_PRIVATE and MEMORY_DEVICE_PUBLIC |
| Message-ID | <tSsoq-4ML-13@gated-at.bofh.it> |
| In reply to | #1666360 |
On Thu, Jun 15, 2017 at 11:41:59AM +1000, Balbir Singh wrote:
> On Wed, 14 Jun 2017 16:11:43 -0400
> Jérôme Glisse <jglisse@redhat.com> wrote:
>
> > HMM pages (private or public device pages) are ZONE_DEVICE page and
> > thus need special handling when it comes to lru or refcount. This
> > patch make sure that memcontrol properly handle those when it face
> > them. Those pages are use like regular pages in a process address
> > space either as anonymous page or as file back page. So from memcg
> > point of view we want to handle them like regular page for now at
> > least.
> >
> > Signed-off-by: Jérôme Glisse <jglisse@redhat.com>
> > Cc: Johannes Weiner <hannes@cmpxchg.org>
> > Cc: Michal Hocko <mhocko@kernel.org>
> > Cc: Vladimir Davydov <vdavydov.dev@gmail.com>
> > Cc: cgroups@vger.kernel.org
> > ---
> > kernel/memremap.c | 2 ++
> > mm/memcontrol.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++-----
> > 2 files changed, 55 insertions(+), 5 deletions(-)
> >
> > diff --git a/kernel/memremap.c b/kernel/memremap.c
> > index da74775..584984c 100644
> > --- a/kernel/memremap.c
> > +++ b/kernel/memremap.c
> > @@ -479,6 +479,8 @@ void put_zone_device_private_or_public_page(struct page *page)
> > __ClearPageActive(page);
> > __ClearPageWaiters(page);
> >
> > + mem_cgroup_uncharge(page);
> > +
>
> A zone device page could have a mem_cgroup charge if
>
> 1. The old page was charged to a cgroup and the new page from ZONE_DEVICE then
> gets the charge that we need to drop here
>
> And should not be charged
>
> 2. If the driver allowed mmap based allocation (these pages are not on LRU
>
>
> Since put_zone_device_private_or_public_page() is called from release_pages(),
> I think the assumption is that 2 is not a problem? I've not tested the mmap
> bits yet.
Well that is one of the big question. Do we care about memory cgroup despite
page not being on lru and thus not being reclaimable through the usual path ?
I believe we do want to keep charging ZONE_DEVICE page against memory cgroup
so that userspace limit are enforced. This is important especialy for device
private when migrating back to system memory due to CPU page fault. We do not
want the migration back to fail because of memory cgroup limit.
Hence why i do want to charge ZONE_DEVICE page just like regular page. If we
have people that run into OOM because of this then we can start thinking about
how to account those pages slightly differently inside the memory cgroup.
For now i believe we do want this patch.
[...]
> > @@ -4610,6 +4637,9 @@ static enum mc_target_type get_mctgt_type(struct vm_area_struct *vma,
> > */
> > if (page->mem_cgroup == mc.from) {
> > ret = MC_TARGET_PAGE;
> > + if (is_device_private_page(page) ||
> > + is_device_public_page(page))
> > + ret = MC_TARGET_DEVICE;
> > if (target)
> > target->page = page;
> > }
> > @@ -4669,6 +4699,11 @@ static int mem_cgroup_count_precharge_pte_range(pmd_t *pmd,
> >
> > ptl = pmd_trans_huge_lock(pmd, vma);
> > if (ptl) {
> > + /*
> > + * Note their can not be MC_TARGET_DEVICE for now as we do not
> there
> > + * support transparent huge page with MEMORY_DEVICE_PUBLIC or
> > + * MEMORY_DEVICE_PRIVATE but this might change.
>
> I am trying to remind myself why THP and MEMORY_DEVICE_* pages don't work well
> together today, the driver could allocate a THP size set of pages and migrate it.
> There are patches to do THP migration, not upstream yet. Could you remind me
> of any other limitations?
No there is nothing that would be problematic AFAICT. Persistent memory already
use huge page so we should be in the clear. But i would rather enable that as
a separate patchset alltogether and have proper testing specificaly for such
scenario.
Jérôme
[toc] | [prev] | [next] | [standalone]
| From | Balbir Singh <bsingharora@gmail.com> |
|---|---|
| Date | 2017-06-15 05:20 +0200 |
| Subject | Re: [HMM-CDM 4/5] mm/memcontrol: support MEMORY_DEVICE_PRIVATE and MEMORY_DEVICE_PUBLIC |
| Message-ID | <tStua-5uX-3@gated-at.bofh.it> |
| In reply to | #1666374 |
On Wed, 14 Jun 2017 22:04:55 -0400
Jerome Glisse <jglisse@redhat.com> wrote:
> On Thu, Jun 15, 2017 at 11:41:59AM +1000, Balbir Singh wrote:
> > On Wed, 14 Jun 2017 16:11:43 -0400
> > Jérôme Glisse <jglisse@redhat.com> wrote:
> >
> > > HMM pages (private or public device pages) are ZONE_DEVICE page and
> > > thus need special handling when it comes to lru or refcount. This
> > > patch make sure that memcontrol properly handle those when it face
> > > them. Those pages are use like regular pages in a process address
> > > space either as anonymous page or as file back page. So from memcg
> > > point of view we want to handle them like regular page for now at
> > > least.
> > >
> > > Signed-off-by: Jérôme Glisse <jglisse@redhat.com>
> > > Cc: Johannes Weiner <hannes@cmpxchg.org>
> > > Cc: Michal Hocko <mhocko@kernel.org>
> > > Cc: Vladimir Davydov <vdavydov.dev@gmail.com>
> > > Cc: cgroups@vger.kernel.org
> > > ---
> > > kernel/memremap.c | 2 ++
> > > mm/memcontrol.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++-----
> > > 2 files changed, 55 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/kernel/memremap.c b/kernel/memremap.c
> > > index da74775..584984c 100644
> > > --- a/kernel/memremap.c
> > > +++ b/kernel/memremap.c
> > > @@ -479,6 +479,8 @@ void put_zone_device_private_or_public_page(struct page *page)
> > > __ClearPageActive(page);
> > > __ClearPageWaiters(page);
> > >
> > > + mem_cgroup_uncharge(page);
> > > +
> >
> > A zone device page could have a mem_cgroup charge if
> >
> > 1. The old page was charged to a cgroup and the new page from ZONE_DEVICE then
> > gets the charge that we need to drop here
> >
> > And should not be charged
> >
> > 2. If the driver allowed mmap based allocation (these pages are not on LRU
> >
> >
> > Since put_zone_device_private_or_public_page() is called from release_pages(),
> > I think the assumption is that 2 is not a problem? I've not tested the mmap
> > bits yet.
>
> Well that is one of the big question. Do we care about memory cgroup despite
> page not being on lru and thus not being reclaimable through the usual path ?
>
> I believe we do want to keep charging ZONE_DEVICE page against memory cgroup
> so that userspace limit are enforced. This is important especialy for device
> private when migrating back to system memory due to CPU page fault. We do not
> want the migration back to fail because of memory cgroup limit.
>
> Hence why i do want to charge ZONE_DEVICE page just like regular page. If we
> have people that run into OOM because of this then we can start thinking about
> how to account those pages slightly differently inside the memory cgroup.
>
> For now i believe we do want this patch.
>
Yes, we do need the patch, I was trying to check if we'll end up trying to uncharge
a page that is not charged, just double checking
>
> [...]
>
> > > @@ -4610,6 +4637,9 @@ static enum mc_target_type get_mctgt_type(struct vm_area_struct *vma,
> > > */
> > > if (page->mem_cgroup == mc.from) {
> > > ret = MC_TARGET_PAGE;
> > > + if (is_device_private_page(page) ||
> > > + is_device_public_page(page))
> > > + ret = MC_TARGET_DEVICE;
> > > if (target)
> > > target->page = page;
> > > }
> > > @@ -4669,6 +4699,11 @@ static int mem_cgroup_count_precharge_pte_range(pmd_t *pmd,
> > >
> > > ptl = pmd_trans_huge_lock(pmd, vma);
> > > if (ptl) {
> > > + /*
> > > + * Note their can not be MC_TARGET_DEVICE for now as we do not
> > there
> > > + * support transparent huge page with MEMORY_DEVICE_PUBLIC or
> > > + * MEMORY_DEVICE_PRIVATE but this might change.
> >
> > I am trying to remind myself why THP and MEMORY_DEVICE_* pages don't work well
> > together today, the driver could allocate a THP size set of pages and migrate it.
> > There are patches to do THP migration, not upstream yet. Could you remind me
> > of any other limitations?
>
> No there is nothing that would be problematic AFAICT. Persistent memory already
> use huge page so we should be in the clear. But i would rather enable that as
> a separate patchset alltogether and have proper testing specificaly for such
> scenario.
Agreed
Balbir Singh.
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web