Add flags for temporary compound pages
Subsystems (6)
A message that is not in the archive (sent off-list or not yet received): [email protected]
RFC PATCH 1/3 Add flags for temporary compound pages
This chunk of the patch just adds the necessary page flags to support what I call a temporary compound page. Temporary compound pages provide us with the ability to turn a chunk of pages into a proper THP at some point after pulling them off the free list, but also allow us to fault in single 4K pages from the chunk.
--- include/linux/gfp.h | 5 +++++ 1 file changed, 5 insertions(+)
include/linux/gfp.h +5 -0
@@ -35,6 +35,7 @@ struct vm_area_struct;#define ___GFP_NO_KSWAPD 0x400000u#define ___GFP_OTHER_NODE 0x800000u#define ___GFP_WRITE 0x1000000u +#define ___GFP_COMP_TEMP 0x2000000u/* If the above are modified, __GFP_BITS_SHIFT may need updating */ /*
@@ -77,6 +78,7 @@ struct vm_area_struct;#define __GFP_NORETRY ((__force gfp_t)___GFP_NORETRY)/* See above */ #define __GFP_MEMALLOC ((__force gfp_t)___GFP_MEMALLOC)/* Allow access to emergency reserves */ #define __GFP_COMP ((__force gfp_t)___GFP_COMP)/* Add compound page metadata */ +#define __GFP_COMP_TEMP ((__force gfp_t)___GFP_COMP_TEMP)/* Treat as temp compound page */ #define __GFP_ZERO ((__force gfp_t)___GFP_ZERO)/* Return zeroed page on success */ #define __GFP_NOMEMALLOC ((__force gfp_t)___GFP_NOMEMALLOC)/* Don't use emergency reserves. * This takes precedence over the
@@ -121,6 +123,9 @@ struct vm_area_struct;#define GFP_TRANSHUGE (GFP_HIGHUSER_MOVABLE | __GFP_COMP | \ __GFP_NOMEMALLOC | __GFP_NORETRY | __GFP_NOWARN | \ __GFP_NO_KSWAPD) +#define GFP_TEMP_TRANSHUGE (GFP_HIGHUSER_MOVABLE | __GFP_COMP_TEMP | \ + __GFP_NOMEMALLOC | __GFP_NORETRY | \ + __GFP_NOWARN | __GFP_NO_KSWAPD)#ifdef CONFIG_NUMA #define GFP_THISNODE (__GFP_THISNODE | __GFP_NOWARN | __GFP_NORETRY)
Signature · 8 lines
1.7.12.4 -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
RFC PATCH 2/3 Add tunable to control THP behavior
In reply to a message not in the archiveThis part of the patch adds a tunable to /sys/kernel/mm/transparent_hugepage called threshold. This threshold determines how many pages a user must fault in from a single node before a temporary compound page is turned into a THP.
--- include/linux/huge_mm.h | 2 ++ include/linux/mm_types.h | 1 + mm/huge_memory.c | 30 ++++++++++++++++++++++++++++++ 3 files changed, 33 insertions(+)
include/linux/huge_mm.h +2 -0
@@ -177,6 +177,8 @@ static inline struct page *compound_trans_head(struct page *page)extern int do_huge_pmd_numa_page (struct mm_struct *mm,struct vm_area_struct *vma,unsigned long addr,pmd_t pmd,pmd_t *pmdp); +extern int thp_threshold_check (void ); +#else /* CONFIG_TRANSPARENT_HUGEPAGE */ #define HPAGE_PMD_SHIFT ({ BUILD_BUG(); 0; })#define HPAGE_PMD_MASK ({ BUILD_BUG(); 0; })
include/linux/mm_types.h +1 -0
@@ -408,6 +408,7 @@ struct mm_struct {#endif #ifdef CONFIG_TRANSPARENT_HUGEPAGE pgtable_t pmd_huge_pte;/* protected by page_table_lock */ +int thp_threshold;#endif #ifdef CONFIG_CPUMASK_OFFSTACK struct cpumask cpumask_allocation;
mm/huge_memory.c +30 -0
@@ -44,6 +44,9 @@ unsigned long transparent_hugepage_flags __read_mostly = (1 <<TRANSPARENT_HUGEPAGE_DEFRAG_KHUGEPAGED_FLAG )| (1 <<TRANSPARENT_HUGEPAGE_USE_ZERO_PAGE_FLAG ); +/* default to 1 page threshold for handing out thps; maintains old behavior */ +static int transparent_hugepage_threshold =1 ; +/* default scan 8*512 pte (or vmas) every 30 second */ static unsigned int khugepaged_pages_to_scan__read_mostly =HPAGE_PMD_NR *8 ;static unsigned int khugepaged_pages_collapsed;
@@ -237,6 +240,11 @@ static struct shrinker huge_zero_page_shrinker = { .seeks = DEFAULT_SEEKS, }; +int thp_threshold_check () +{ +return transparent_hugepage_threshold; +} +#ifdef CONFIG_SYSFS static ssize_t double_flag_show(struct kobject *kobj,
@@ -376,6 +384,27 @@ static ssize_t use_zero_page_store(struct kobject *kobj, }static struct kobj_attribute use_zero_page_attr =__ATTR (use_zero_page,0644 , use_zero_page_show, use_zero_page_store); +static ssize_t threshold_show (struct kobject *kobj, +struct kobj_attribute *attr,char *buf) +{ +return sprintf (buf,"%d\n" , transparent_hugepage_threshold); +} +static ssize_t threshold_store (struct kobject *kobj, +struct kobj_attribute *attr, +const char *buf,size_t count) +{ +int err, value; + + err =kstrtoint (buf,10 , &value); +if (err || value <1 || value >HPAGE_PMD_NR ) +return -EINVAL ; + + transparent_hugepage_threshold = value; + +return count; +} +static struct kobj_attribute threshold_attr = +__ATTR (threshold,0644 , threshold_show, threshold_store);#ifdef CONFIG_DEBUG_VM static ssize_t debug_cow_show (struct kobject *kobj,struct kobj_attribute *attr,char *buf)
@@ -397,6 +426,7 @@ static struct kobj_attribute debug_cow_attr =static struct attribute *hugepage_attr[] = { &enabled_attr.attr, &defrag_attr.attr, + &threshold_attr.attr, &use_zero_page_attr.attr, #ifdef CONFIG_DEBUG_VM &debug_cow_attr.attr,
Signature · 8 lines
1.7.12.4 -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Re: [RFC PATCH 2/3] Add tunable to control THP behavior
Quoted 4 lines · Alex Thorlton
This part of the patch adds a tunable to
/sys/kernel/mm/transparent_hugepagecalled threshold. This threshold determines how many pages a user must fault in from a single node before a temporary compound page is turned into a THP.
Is there a setting that will turn off the must-be-the-same-node behavior? There are workloads where TLB matters more than cross-node traffic (or where all the pages are hopelessly shared between nodes, but hugepages are still useful).
--Andy
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Re: [RFC PATCH 2/3] Add tunable to control THP behavior
Quoted 4 lines
Is there a setting that will turn off the must-be-the-same-node behavior? There are workloads where TLB matters more than cross-node traffic (or where all the pages are hopelessly shared between nodes, but hugepages are still useful).
That's pretty much how THPs already behave in the kernel, so if you want to allow THPs to be handed out to one node, but referenced from many others, you'd just set the threshold to 1, and let the existing code take over.
As for the must-be-the-same-node behavior: I'd actually say it's more like a "must have so much on one node" behavior, in that, if you set the threshold to 16, for example, 16 4K pages must be faulted in on the same node, in the same contiguous 2M chunk, before a THP will be created. What happens after that THP is created is out of our control, it could be referenced from anywhere.
The idea here is that we can tune things so that jobs that behave poorly with THP on will not be given THPs, but the jobs that like THPs can still get them. Granted, there are still issues with this approach, but I think it's a bit better than just handing out a THP because we touched one byte in a 2M chunk.
- Alex
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Re: [RFC PATCH 2/3] Add tunable to control THP behavior
Quoted 11 lines · Alex Thorlton
Quoted 5 lines
Is there a setting that will turn off the must-be-the-same-node behavior? There are workloads where TLB matters more than cross-node traffic (or where all the pages are hopelessly shared between nodes, but hugepages are still useful).
That's pretty much how THPs already behave in the kernel, so if you want to allow THPs to be handed out to one node, but referenced from many others, you'd just set the threshold to 1, and let the existing code take over.
Right. I like that behavior for my workload. (Although I currently allocate huge pages -- when I wrote that code, THP interacted so badly with pagecache that it was a non-starter. I think it's fixed now, though.)
Quoted 7 lines
As for the must-be-the-same-node behavior: I'd actually say it's more like a "must have so much on one node" behavior, in that, if you set the threshold to 16, for example, 16 4K pages must be faulted in on the same node, in the same contiguous 2M chunk, before a THP will be created. What happens after that THP is created is out of our control, it could be referenced from anywhere.
In that case, I guess I misunderstood your description. Are saying that, once any node accesses this many pages in the potential THP, then the whole THP will be mapped?
--Andy
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Re: [RFC PATCH 2/3] Add tunable to control THP behavior
Quoted 4 lines
Right. I like that behavior for my workload. (Although I currently allocate huge pages -- when I wrote that code, THP interacted so badly with pagecache that it was a non-starter. I think it's fixed now, though.)
In that case, it's probably best to just stick with current behavior, and leave the threshold at 1, unless we implement something like I discuss below.
Quoted 3 lines
In that case, I guess I misunderstood your description. Are saying that, once any node accesses this many pages in the potential THP, then the whole THP will be mapped?
Well, right now, this patch completely gives up on mapping a THP if two different nodes take a page from our chunk before the threshold is hit, so I think you're mostly understanding it correctly.
One thing we could consider is adding an option to map the THP on the node with the *most* references to the potential THP, instead of giving up on mapping the THP when multiple nodes reference it. That might be a good middle ground, but I can see some performance issues coming into play there if the threshold is set too high, since we'll have to move all the pages in the chunk to the node that hit the threshold.
- Alex
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Re: [RFC PATCH 2/3] Add tunable to control THP behavior
In reply to Alex Thorltonmm/huge_memory.c unsigned long transparent_hugepage_flags __read_mostly = · line 48
Quoted 12 lines · Alex Thorlton
This part of the patch adds a tunable to
/sys/kernel/mm/transparent_hugepagecalled threshold. This threshold determines how many pages a user must fault in from a single node before a temporary compound page is turned into a THP.+++
b/mm/huge_memory.cmm/huge_memory.c +2
-0@@ -44,6 +44,9 @@ unsigned long transparent_hugepage_flags __read_mostly = (1 <<TRANSPARENT_HUGEPAGE_DEFRAG_KHUGEPAGED_FLAG )| (1 <<TRANSPARENT_HUGEPAGE_USE_ZERO_PAGE_FLAG ); +/* default to 1 page threshold for handing out thps; maintains old behavior */ +static int transparent_hugepage_threshold =1 ;
I assume the motivation for writing all this code is that "1" was not a good value in your tests.
That makes me wonder, why should 1 be the default value with your patches?
If there is a better value, why should we not use that?
What is the upside of using a better value?
What is the downside?
Is there a value that would to bound the downside, so it is almost always smaller than the upside?
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Re: [RFC PATCH 2/3] Add tunable to control THP behavior
Quoted 11 lines · Rik van Riel
Quoted 10 lines · Alex Thorlton
This part of the patch adds a tunable to
/sys/kernel/mm/transparent_hugepagecalled threshold. This threshold determines how many pages a user must fault in from a single node before a temporary compound page is turned into a THP.+++
b/mm/huge_memory.cmm/huge_memory.c +0
-0@@ -44,6 +44,9 @@ unsigned long transparent_hugepage_flags __read_mostly = (1 <<TRANSPARENT_HUGEPAGE_DEFRAG_KHUGEPAGED_FLAG )| (1 <<TRANSPARENT_HUGEPAGE_USE_ZERO_PAGE_FLAG );
Quoted 5 lines
mm/huge_memory.c unsigned long transparent_hugepage_flags __read_mostly = · line 48
Quoted 2 lines
mm/huge_memory.c +2
-0+/* default to 1 page threshold for handing out thps; maintains old behavior */ +static int transparent_hugepage_threshold =1 ;I assume the motivation for writing all this code is that "1" was not a good value in your tests.
Yes, that's correct.
Quoted 2 lines
That makes me wonder, why should 1 be the default value with your patches?
The main reason I set the default to 1 was because the majority of jobs aren't hurt by the existing THP behavior. I figured it would be best to default to having things behave the same as they do now, but provide the option to increase the threshold on systems that run jobs that could be adversely affected by the current behavior.
Quoted 5 lines
If there is a better value, why should we not use that?
What is the upside of using a better value?
What is the downside?
The problem here is that what the "better" value is can vary greatly depending on how a particular task allocates memory. Setting the threshold too high can negatively affect the performance of jobs that behave well with the current behavior, setting it too low won't yield a performance increase for the jobs that are hurt by the current behavior. With some more thorough testing, I'm sure that we could arrive at a value that will help out jobs which behave poorly under current conditions, while having a minimal effect on jobs that already perform well. At this point, I'm looking more to ensure that everybody likes this approach to solving the problem before putting the finishing touches on the patches, and doing testing to find a good middle ground.
Quoted 2 lines
Is there a value that would to bound the downside, so it is almost always smaller than the upside?
Again, the problem here is that, to find a good value, we have to know quite a bit about why a particular value is bad for a particular job. While, as stated above, I think we can probably find a good middle ground to use as a default, in the end it will be the job of individual sysadmins to determine what value works best for their particular applications, and tune things accordingly.
- Alex
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
RFC PATCH 3/3 Change THP behavior
In reply to a message not in the archiveThis patch implements the functionality we're really going for here. It adds the decision making behavior to determine when to grab a temporary compound page, and whether or not to fault in single pages or to turn the temporary page into a THP. This one is rather large, might split it up a bit more for later versions
I've left most of my comments in here just to provide people with some insight into what I may have been thinking when I chose to do something in a certain way. These will probably be trimmed down in later versions of the patch.
--- include/linux/huge_mm.h | 6 + include/linux/mm_types.h | 13 +++ kernel/fork.c | 1 + mm/huge_memory.c | 283 +++++++++++++++++++++++++++++++++++++++++++++++ mm/internal.h | 1 + mm/memory.c | 29 ++++- mm/page_alloc.c | 66 ++++++++++- 7 files changed, 392 insertions(+), 7 deletions(-)
include/linux/huge_mm.h +6 -0
@@ -5,6 +5,12 @@ extern int do_huge_pmd_anonymous_page(struct mm_struct *mm,struct vm_area_struct *vma, unsigned long address, pmd_t *pmd, unsigned int flags); +extern struct temp_hugepage *find_pmd_mm_freelist (struct mm_struct *mm, +pmd_t *pmd); +extern int do_huge_pmd_temp_page (struct mm_struct *mm, +struct vm_area_struct *vma, +unsigned long address,pmd_t *pmd, +unsigned int flags);extern int copy_huge_pmd (struct mm_struct *dst_mm,struct mm_struct *src_mm,pmd_t *dst_pmd,pmd_t *src_pmd,unsigned long addr,struct vm_area_struct *vma);
include/linux/mm_types.h +13 -0
@@ -322,6 +322,17 @@ struct mm_rss_stat {atomic_long_t count[NR_MM_COUNTERS ]; }; +#ifdef CONFIG_TRANSPARENT_HUGEPAGE +struct temp_hugepage { +pmd_t *pmd; +struct page *page; +spinlock_t temp_hugepage_lock; +int node;/* node id of the first page in the chunk */ +int ref_count;/* number of pages faulted in from the chunk */ +struct list_head list;/* pointers to next/prev chunks */ +}; +#endif +struct kioctx_table ;struct mm_struct {struct vm_area_struct * mmap;/* list of VMAs */
@@ -408,7 +419,9 @@ struct mm_struct {#endif #ifdef CONFIG_TRANSPARENT_HUGEPAGE pgtable_t pmd_huge_pte;/* protected by page_table_lock */ +spinlock_t thp_list_lock;/* lock to protect thp_temp_list */ int thp_threshold; +struct list_head thp_temp_list;/* list of 512 page chunks for THPs */ #endif #ifdef CONFIG_CPUMASK_OFFSTACK struct cpumask cpumask_allocation;
kernel/fork.c +1 -0
@@ -816,6 +816,7 @@ struct mm_struct *dup_mm(struct task_struct *tsk)#ifdef CONFIG_TRANSPARENT_HUGEPAGE mm->pmd_huge_pte =NULL ; +INIT_LIST_HEAD (&mm->thp_temp_list);#endif #ifdef CONFIG_NUMA_BALANCING mm->first_nid =NUMA_PTE_SCAN_INIT ;
mm/huge_memory.c +283 -0
295 lines not on this page · raw message
mm/internal.h +1 -0
@@ -98,6 +98,7 @@ extern pmd_t *mm_find_pmd(struct mm_struct *mm, unsigned long address);*/ extern void __free_pages_bootmem (struct page *page,unsigned int order);extern void prep_compound_page (struct page *page,unsigned long order); +extern void make_compound_page (struct page *page,unsigned long order);#ifdef CONFIG_MEMORY_FAILURE extern bool is_free_buddy_page (struct page *page);#endif
mm/memory.c +23 -6
@@ -3764,13 +3764,30 @@ retry: pmd =pmd_alloc (mm, pud, address);if (!pmd)return VM_FAULT_OOM ; -if (pmd_none (*pmd) &&transparent_hugepage_enabled (vma)) { +if (transparent_hugepage_enabled (vma)) {int ret =VM_FAULT_FALLBACK ; -if (!vma->vm_ops) - ret =do_huge_pmd_anonymous_page (mm, vma, address, - pmd, flags); -if (!(ret &VM_FAULT_FALLBACK )) -return ret; +/* +* This is a temporary location for this code, just to get things +* up and running. I'll come up with a better way to handle this +* later +*/ +if (!mm->thp_threshold) + mm->thp_threshold =thp_threshold_check (); +if (!mm->thp_temp_list.next && !mm->thp_temp_list.prev) +INIT_LIST_HEAD (&mm->thp_temp_list); +if (mm->thp_threshold >1 ) { +if (!vma->vm_ops) + ret =do_huge_pmd_temp_page (mm, vma, address, + pmd, flags); +if (!(ret &VM_FAULT_FALLBACK )) +return ret; + }else if (pmd_none (*pmd)) { +if (!vma->vm_ops) + ret =do_huge_pmd_anonymous_page (mm, vma, address, + pmd, flags); +if (!(ret &VM_FAULT_FALLBACK )) +return ret; + } }else {pmd_t orig_pmd = *pmd;int ret;
mm/page_alloc.c +65 -1
78 lines not on this page · raw message
Signature · 7 lines
1.7.12.4 -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
RE: [RFC PATCH 3/3] Change THP behavior
Quoted 48 lines · Alex Thorlton
mm/huge_memory.c +48
-0+/* +* now that we've done the accounting work, we check to see if +* we've exceeded our threshold +*/ +if (temp_thp->ref_count >= mm->thp_threshold) { +pmd_t pmd_entry; +pgtable_t pgtable; + +/* +* we'll do all of the following beneath the big ptl for now +* this will need to be modified to work with the split ptl +*/ +spin_lock (&mm->page_table_lock); + +/* +* once we get past the lock we have to make sure that somebody +* else hasn't already turned this guy into a THP, if they have, +* then the page we need is already faulted in as part of the THP +* they created +*/ +if (PageTransHuge (temp_thp->page)) { +spin_unlock (&mm->page_table_lock); +return 0 ; + } + + pgtable =pte_alloc_one (mm, haddr); +if (unlikely (!pgtable)) { +spin_unlock (&mm->page_table_lock); +return VM_FAULT_OOM ; + } + +/* might wanna move this? */ +__SetPageUptodate (temp_thp->page); + +/* turn the pages into one compound page */ +make_compound_page (temp_thp->page,HPAGE_PMD_ORDER ); + +/* set up the pmd */ + pmd_entry =mk_huge_pmd (temp_thp->page, vma->vm_page_prot); + pmd_entry =maybe_pmd_mkwrite (pmd_mkdirty (pmd_entry), vma); + +/* remap the new page since we cleared the mappings */ +page_add_anon_rmap (temp_thp->page, vma, address); + +/* deposit the thp */ +pgtable_trans_huge_deposit (mm, pmd, pgtable); + +set_pmd_at (mm, haddr, pmd, pmd_entry);
mm/huge_memory.c int do_huge_pmd_anonymous_page(struct mm_struct *mm, struct vm_area_struct *vma, · line 1078
Quoted 10 lines
mm/huge_memory.c +10
-0+add_mm_counter (mm,MM_ANONPAGES ,HPAGE_PMD_NR - mm->thp_threshold +1 ); +/* mm->nr_ptes++; */ + +/* delete the reference to this compound page from our list */ +spin_lock (&mm->thp_list_lock); +list_del (&temp_thp->list); +spin_unlock (&mm->thp_list_lock); + +spin_unlock (&mm->page_table_lock); +return 0 ;
Hm. I think this part is not correct: you collapse temp thp page into real one only for current procees. What will happen if a process with temp thp pages was forked?
And I don't think this problem is an easy one. khugepaged can't collapse pages with page->_count != 1 for the same reason: to make it properly you need to take mmap_sem for all processes and collapse all pages at once. And if a page is pinned, we also can't collapse.
Sorry, I don't think the whole idea has much potential. :(
Signature · 7 lines
Kirill A. Shutemov -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Re: [RFC PATCH 3/3] Change THP behavior
Quoted 3 lines
Hm. I think this part is not correct: you collapse temp thp page into real one only for current procees. What will happen if a process with temp thp pages was forked?
That's a scenario that I hadn't yet addressed, but definitely something I'll consider going forward. I think we can come up with a way to appropriately handle that situation.
Quoted 4 lines
And I don't think this problem is an easy one. khugepaged can't collapse pages with
page->_count!= 1 for the same reason: to make it properly you need to takemmap_semfor all processes and collapse all pages at once. And if a page is pinned, we also can't collapse.
Again, a few things here that I hadn't taken into account. I'll look for a way to address these concerns.
Quoted 1 line
Sorry, I don't think the whole idea has much potential. :(
I understand that there are some issues with this initial pass at the idea, but I think we can get things corrected and come up with a workable solution here. When it comes down to it, there are relatively few options to correct the issue that I'm focusing on here, and this one seems to be the most appropriate one that we've tried so far. We've looked at disabling THP on a per-cpuset/per-process basis, and that has been met with fairly strong resistance, for good reason. I'll take another pass at this and hopefully be able to address some of your concerns with the idea.
Thanks for taking the time to look the patch over!
- Alex
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Re: [RFC PATCH 3/3] Change THP behavior
In reply to Alex ThorltonI know almost nothing about thp, unlikely I understand this patch correctly...
But, afaics, the main idea is that until we have mm->thp_threshold faults we install the tail pages of temp_hugepage->page as a normal anonymous page, then we actually add the Head/Tail metadata and add the necessary huge_pmd/etc.
I simply can't understand how this can work until make_compound_page() is called. Just for example, what happens after sys_munmap() ? If nothing else, who will find and free temp_hugepage connected to this area? Or, what if sys_mremap() moves this vma? find_pmd_mm_freelist() won't find the right temp_thp after that? Or split_vma, etc.
And make_compound_page() itself looks suspicious,
Quoted 15 lines · Alex Thorlton
mm/page_alloc.c +14
-0+void make_compound_page (struct page *page,unsigned long order) +{ +int i, max_count =0 , max_mapcount =0 ; +int nr_pages =1 << order; + +set_compound_page_dtor (page, free_compound_page); +set_compound_order (page, order); + +__SetPageHead (page); + +/* +* we clear all the mappings here, so we have to remember to set +* them back up! +*/
mm/page_alloc.c void prep_compound_page(struct page *page, unsigned long order) · line 405
Quoted 10 lines
mm/page_alloc.c +10
-0+ page->mapping =NULL ; + + max_count = (int )atomic_read (&page->_count); + max_mapcount = (int )atomic_read (&page->_mapcount); + +for (i =1 ; i < nr_pages; i++) { +int cur_count, cur_mapcount; +struct page *p = page + i; + p->flags =0 ;/* this seems dumb */ +__SetPageTail (p);
Just for example, what if put_page(p) or get_page(p) is called after __SetPageTail() ?
Afaics, this page was already visible to, say, get_user_pages() and it can have external references.
In fact everything else looks suspicious too but let me repeat that I do not really understand this code. So please do not count this as review, but perhaps the changelog should tell more to explain what this patch actually does and how this all should work?
Oleg.
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>