On 19 Mar 2021, at 19:37, Yang Shi wrote:
> On Thu, Mar 18, 2021 at 5:52 PM Zi Yan <zi.yan@xxxxxxxx> wrote:
>>
>> From: Zi Yan <ziy@xxxxxxxxxx>
>>
>> We did not have a direct user interface of splitting the compound page
>> backing a THP and there is no need unless we want to expose the THP
>> implementation details to users. Make <debugfs>/split_huge_pages accept
>> a new command to do that.
>>
>> By writing "<pid>,<vaddr_start>,<vaddr_end>" to
>> <debugfs>/split_huge_pages, THPs within the given virtual address range
>> from the process with the given pid are split. It is used to test
>> split_huge_page function. In addition, a selftest program is added to
>> tools/testing/selftests/vm to utilize the interface by splitting
>> PMD THPs and PTE-mapped THPs.
>>
>> This does not change the old behavior, i.e., writing 1 to the interface
>> to split all THPs in the system.
>>
>> Changelog:
>>
>> From v5:
>> 1. Skipped special VMAs and other fixes. (suggested by Yang Shi)
>
> Looks good to me. Reviewed-by: Yang Shi <shy828301@xxxxxxxxx>
>
> Some nits below:
>
>>
>> From v4:
>> 1. Fixed the error code return issue, spotted by kernel test robot
>> <lkp@xxxxxxxxx>.
>>
>> From v3:
>> 1. Factored out split huge pages in the given pid code to a separate
>> function.
>> 2. Added the missing put_page for not split pages.
>> 3. pr_debug -> pr_info, make reading results simpler.
>>
>> From v2:
>> 1. Reused existing <debugfs>/split_huge_pages interface. (suggested by
>> Yang Shi)
>>
>> From v1:
>> 1. Removed unnecessary calling to vma_migratable, spotted by kernel test
>> robot <lkp@xxxxxxxxx>.
>> 2. Dropped the use of find_mm_struct and code it directly, since there
>> is no need for the permission check in that function and the function
>> is only available when migration is on.
>> 3. Added some comments in the selftest program to clarify how PTE-mapped
>> THPs are formed.
>>
>> Signed-off-by: Zi Yan <ziy@xxxxxxxxxx>
>> ---
>> mm/huge_memory.c | 143 +++++++-
>> tools/testing/selftests/vm/.gitignore | 1 +
>> tools/testing/selftests/vm/Makefile | 1 +
>> .../selftests/vm/split_huge_page_test.c | 318 ++++++++++++++++++
>> 4 files changed, 456 insertions(+), 7 deletions(-)
>> 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 bff92dea5ab3..9bf9bc489228 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>
>> @@ -2922,16 +2923,14 @@ static struct shrinker deferred_split_shrinker = {
>> };
>>
>> #ifdef CONFIG_DEBUG_FS
>> -static int split_huge_pages_set(void *data, u64 val)
>> +static void split_huge_pages_all(void)
>> {
>> struct zone *zone;
>> struct page *page;
>> unsigned long pfn, max_zone_pfn;
>> unsigned long total = 0, split = 0;
>>
>> - if (val != 1)
>> - return -EINVAL;
>> -
>> + pr_info("Split all THPs\n");
>> for_each_populated_zone(zone) {
>> max_zone_pfn = zone_end_pfn(zone);
>> for (pfn = zone->zone_start_pfn; pfn < max_zone_pfn; pfn++) {
>> @@ -2959,11 +2958,141 @@ static int split_huge_pages_set(void *data, u64 val)
>> }
>>
>> pr_info("%lu of %lu THP split\n", split, total);
>> +}
>>
>> - return 0;
>> +static int split_huge_pages_pid(int pid, unsigned long vaddr_start,
>> + unsigned long vaddr_end)
>> +{
>> + int ret = 0;
>> + struct task_struct *task;
>> + struct mm_struct *mm;
>> + unsigned long total = 0, split = 0;
>> + unsigned long addr;
>> +
>> + vaddr_start &= PAGE_MASK;
>> + vaddr_end &= PAGE_MASK;
>> +
>> + /* Find the task_struct from pid */
>> + rcu_read_lock();
>> + task = find_task_by_vpid(pid);
>> + if (!task) {
>> + rcu_read_unlock();
>> + ret = -ESRCH;
>> + goto out;
>> + }
>> + get_task_struct(task);
>> + rcu_read_unlock();
>> +
>> + /* Find the mm_struct */
>> + mm = get_task_mm(task);
>> + put_task_struct(task);
>> +
>> + if (!mm) {
>> + ret = -EINVAL;
>> + goto out;
>> + }
>> +
>> + pr_info("Split huge pages in pid: %d, vaddr: [0x%lx - 0x%lx]\n",
>> + pid, vaddr_start, vaddr_end);
>> +
>> + mmap_read_lock(mm);
>> + /*
>> + * 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) {
>> + struct vm_area_struct *vma = find_vma(mm, addr);
>> + unsigned int follflags;
>> + struct page *page;
>> +
>> + if (!vma || addr < vma->vm_start)
>> + break;
>> +
>> + /* skip special VMA and hugetlb VMA */
>> + if (vma_is_special_huge(vma) || is_vm_hugetlb_page(vma)) {
>
> VM_IO vma should be skipped as well. And you may extract this into a helper?
Sure. Any name suggestion? :)
>> + addr = vma->vm_end;
>> + continue;
>> + }
>> +
>> + /* FOLL_DUMP to ignore special (like zero) pages */
>> + follflags = FOLL_GET | FOLL_DUMP;
>> + page = follow_page(vma, addr, follflags);
>> +
>> + if (IS_ERR(page))
>> + continue;
>> + if (!page)
>> + continue;
>> +
>> + if (!is_transparent_hugepage(page))
>> + goto next;
>> +
>> + total++;
>> + if (!can_split_huge_page(compound_head(page), NULL))
>> + goto next;
>> +
>> + if (!trylock_page(page))
>> + goto next;
>> +
>> + if (!split_huge_page(page))
>> + split++;
>> +
>> + unlock_page(page);
>> +next:
>> + put_page(page);
>> + }
>> + mmap_read_unlock(mm);
>> + mmput(mm);
>> +
>> + pr_info("%lu of %lu THP split\n", split, total);
>> +
>> +out:
>> + return ret;
>> }
>> -DEFINE_DEBUGFS_ATTRIBUTE(split_huge_pages_fops, NULL, split_huge_pages_set,
>> - "%llu\n");
>> +
>> +static ssize_t split_huge_pages_write(struct file *file, const char __user *buf,
>> + size_t count, loff_t *ppops)
>> +{
>> + static DEFINE_MUTEX(split_debug_mutex);
>> + ssize_t ret;
>> + char input_buf[80]; /* hold pid, start_vaddr, end_vaddr */
>
> Why not move buf len macro in the following patch to this patch? Then
> you don't have to change the length again.
Sure.
Thanks for the comments.
--
Best Regards,
Yan Zi
Attachment:
signature.asc
Description: OpenPGP digital signature
