On 16 Nov 2020, at 11:06, Kirill A. Shutemov wrote: > On Wed, Nov 11, 2020 at 03:40:03PM -0500, Zi Yan wrote: >> From: Zi Yan <[email protected]> >> >> Huge pages in the process with the given pid and virtual address range >> are split. It is used to test split huge page function. In addition, >> a testing program is added to tools/testing/selftests/vm to utilize the >> interface by splitting PMD THPs. >> >> Signed-off-by: Zi Yan <[email protected]> >> --- >> mm/huge_memory.c | 98 +++++++++++ >> mm/internal.h | 1 + >> mm/migrate.c | 2 +- >> tools/testing/selftests/vm/Makefile | 1 + >> .../selftests/vm/split_huge_page_test.c | 161 ++++++++++++++++++ >> 5 files changed, 262 insertions(+), 1 deletion(-) >> create mode 100644 tools/testing/selftests/vm/split_huge_page_test.c >> >> diff --git a/mm/huge_memory.c b/mm/huge_memory.c >> index 207ebca8c654..c4fead5ead31 100644 >> --- a/mm/huge_memory.c >> +++ b/mm/huge_memory.c >> @@ -7,6 +7,7 @@ >> >> #include <linux/mm.h> >> #include <linux/sched.h> >> +#include <linux/sched/mm.h> >> #include <linux/sched/coredump.h> >> #include <linux/sched/numa_balancing.h> >> #include <linux/highmem.h> >> @@ -2935,10 +2936,107 @@ static int split_huge_pages_set(void *data, u64 val) >> DEFINE_DEBUGFS_ATTRIBUTE(split_huge_pages_fops, NULL, split_huge_pages_set, >> "%llu\n"); >> >> +static ssize_t split_huge_pages_in_range_pid_write(struct file *file, >> + const char __user *buf, size_t count, loff_t *ppops) >> +{ >> + static DEFINE_MUTEX(mutex); >> + ssize_t ret; >> + char input_buf[80]; /* hold pid, start_vaddr, end_vaddr */ >> + int pid; >> + unsigned long vaddr_start, vaddr_end, addr; >> + nodemask_t task_nodes; >> + struct mm_struct *mm; >> + >> + ret = mutex_lock_interruptible(&mutex); >> + if (ret) >> + return ret; >> + >> + ret = -EFAULT; >> + >> + memset(input_buf, 0, 80); >> + if (copy_from_user(input_buf, buf, min_t(size_t, count, 80))) >> + goto out; >> + >> + input_buf[80] = '\0'; > > Hm. Out-of-buffer access?
Sorry. Will fix it.
>
>> + ret = sscanf(input_buf, "%d,%lx,%lx", &pid, &vaddr_start, &vaddr_end);
>
> Why hex without 0x prefix?
No particular reason. Let me add the prefix.
>
>> + if (ret != 3) {
>> + ret = -EINVAL;
>> + goto out;
>> + }
>> + vaddr_start &= PAGE_MASK;
>> + vaddr_end &= PAGE_MASK;
>> +
>> + ret = strlen(input_buf);
>> + pr_debug("split huge pages in pid: %d, vaddr: [%lx - %lx]\n",
>> + pid, vaddr_start, vaddr_end);
>> +
>> + mm = find_mm_struct(pid, &task_nodes);
>
> I don't follow why you need nodemask.
I don’t need it. I just reuse the find_mm_struct function from
mm/migrate.c.
>
>> + if (IS_ERR(mm)) {
>> + ret = -EINVAL;
>> + goto out;
>> + }
>> +
>> + mmap_read_lock(mm);
>> + for (addr = vaddr_start; addr < vaddr_end;) {
>> + struct vm_area_struct *vma = find_vma(mm, addr);
>> + unsigned int follflags;
>> + struct page *page;
>> +
>> + if (!vma || addr < vma->vm_start || !vma_migratable(vma))
>> + break;
>> +
>> + /* FOLL_DUMP to ignore special (like zero) pages */
>> + follflags = FOLL_GET | FOLL_DUMP;
>> + page = follow_page(vma, addr, follflags);
>> +
>> + if (IS_ERR(page))
>> + break;
>> + if (!page)
>> + break;
>> +
>> + if (!is_transparent_hugepage(page))
>> + goto next;
>> +
>> + if (!can_split_huge_page(page, NULL))
>> + goto next;
>> +
>> + if (!trylock_page(page))
>> + goto next;
>> +
>> + addr += page_size(page) - PAGE_SIZE;
>
> Who said it was mapped as huge? mremap() allows to construct an PTE page
> table that filled with PTE-mapped THPs, each of them distinct.
I forgot about this. I was trying to be smart to skip the rest of
subpages if we split a THP. I will increase addr always by PAGE_SIZE
to handle this situation.
>> +
>> + /* reset addr if split fails */
>> + if (split_huge_page(page))
>> + addr -= (page_size(page) - PAGE_SIZE);
>> +
>> + unlock_page(page);
>> +next:
>> + /* next page */
>> + addr += page_size(page);
>
> Isn't it the second time if split_huge_page() succeed.
If split_huge_page() succeeds, page_size(page) would be PAGE_SIZE
and addr was increased by THP size - PAGE_SIZE above, so addr now should
be at the end of the original THP.
Anyway, I will change the code to something like:
/*
* always increase addr by PAGE_SIZE, since we could have a PTE page
* table filled with PTE-mapped THPs, each of which is distinct.
*/
for (addr = vaddr_start; addr < vaddr_end; addr += PAGE_SIZE) {
...
if (!trylock_page(page))
continue;
split_huge_page(page);
unlock_page(page);
put_page(page);
}
mmap_read_unlock(mm);
Thanks for reviewing the patch.
—
Best Regards,
Yan Zi
signature.asc
Description: OpenPGP digital signature
