Re: [PATCH v4 2/2] mm: huge_memory: debugfs for file-backed THP split.
On Wed, Mar 17, 2021 at 8:00 AM Zi Yan wrote: > > On 16 Mar 2021, at 19:18, Yang Shi wrote: > > > On Mon, Mar 15, 2021 at 1:34 PM Zi Yan wrote: > >> > >> From: Zi Yan > >> > >> Further extend /split_huge_pages to accept > >> ",," for file-backed THP split tests since > >> tmpfs may have file backed by THP that mapped nowhere. > >> > >> Update selftest program to test file-backed THP split too. > >> > >> Suggested-by: Kirill A. Shutemov > >> Signed-off-by: Zi Yan > >> --- > >> mm/huge_memory.c | 95 ++- > >> .../selftests/vm/split_huge_page_test.c | 79 ++- > >> 2 files changed, 166 insertions(+), 8 deletions(-) > >> > >> diff --git a/mm/huge_memory.c b/mm/huge_memory.c > >> index 3bfee54e2cd0..da91ee97d944 100644 > >> --- a/mm/huge_memory.c > >> +++ b/mm/huge_memory.c > >> @@ -3043,12 +3043,72 @@ static int split_huge_pages_pid(int pid, unsigned > >> long vaddr_start, > >> return ret; > >> } > >> > >> +static int split_huge_pages_in_file(const char *file_path, pgoff_t > >> off_start, > >> + pgoff_t off_end) > >> +{ > >> + struct filename *file; > >> + struct file *candidate; > >> + struct address_space *mapping; > >> + int ret = -EINVAL; > >> + pgoff_t off_cur; > >> + unsigned long total = 0, split = 0; > >> + > >> + file = getname_kernel(file_path); > >> + if (IS_ERR(file)) > >> + return ret; > >> + > >> + candidate = file_open_name(file, O_RDONLY, 0); > >> + if (IS_ERR(candidate)) > >> + goto out; > >> + > >> + pr_info("split file-backed THPs in file: %s, offset: [0x%lx - > >> 0x%lx]\n", > >> +file_path, off_start, off_end); > >> + > >> + mapping = candidate->f_mapping; > >> + > >> + for (off_cur = off_start; off_cur < off_end;) { > >> + struct page *fpage = pagecache_get_page(mapping, off_cur, > >> + FGP_ENTRY | FGP_HEAD, 0); > >> + > >> + if (xa_is_value(fpage) || !fpage) { > > > > Why do you have FGP_ENTRY? It seems it would return page instead of > > NULL if page is value. So I think you could remove FGP_ENTRY and > > xa_is_value() check as well. > > The comment on FGP_ENTRY says “If there is a shadow/swap/DAX entry, return > it instead of allocating a new page to replace it”. I do not think we > want to allocate new pages here. I mostly follow the use of > pagecache_get_page() > in shmem_getpage_gfp without swapin or allocating new pages. Yes, you are correct. I overlooked that. > > > > >> + off_cur += PAGE_SIZE; > >> + continue; > >> + } > >> + > >> + if (!is_transparent_hugepage(fpage)) { > >> + off_cur += PAGE_SIZE; > >> + goto next; > >> + } > >> + total++; > >> + off_cur = fpage->index + thp_size(fpage); > >> + > >> + if (!trylock_page(fpage)) > >> + goto next; > >> + > >> + if (!split_huge_page(fpage)) > >> + split++; > >> + > >> + unlock_page(fpage); > >> +next: > >> + put_page(fpage); > >> + } > >> + > >> + filp_close(candidate, NULL); > >> + ret = 0; > >> + > >> + pr_info("%lu of %lu file-backed THP split\n", split, total); > >> +out: > >> + putname(file); > >> + return ret; > >> +} > >> + > >> static ssize_t split_huge_pages_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 */ > >> + /* hold pid, start_vaddr, end_vaddr or file_path, off_start, > >> off_end */ > >> + char input_buf[MAX_INPUT]; > > > > I didn't find where MAX_INPUT is defined in your patch. Just saw > > include/uapi/linux/limits.h have it defined. Is it the one you really > > refer to? > > Yeah, I want to use 255 as the max input size and find MAX_INPUT. From your > comment, > I think it is better to define a MACRO here explicitly. > > > >> int pid; > >> unsigned long vaddr_start, vaddr_end; > >> > >> @@ -3058,11 +3118,40 @@ static ssize_t split_huge_pages_write(struct file > >> *file, const char __user *buf, > >> > >> ret = -EFAULT; > >> > >> - memset(input_buf, 0, 80); > >> + memset(input_buf, 0, MAX_INPUT); > >> if (copy_from_user(input_buf, buf, min_t(size_t, count, 80))) > >> goto out; > >> > >> - input_buf[79] = '\0'; > >> + input_buf[MAX_INPUT - 1] = '\0'; > >> + > >> + if (input_buf[0] == '/') { > >> + char *tok; > >> + char *buf = input_buf; > >> + char file_path[MAX_INPUT]; > >>
Re: [PATCH v4 2/2] mm: huge_memory: debugfs for file-backed THP split.
On 16 Mar 2021, at 19:18, Yang Shi wrote: > On Mon, Mar 15, 2021 at 1:34 PM Zi Yan wrote: >> >> From: Zi Yan >> >> Further extend /split_huge_pages to accept >> ",," for file-backed THP split tests since >> tmpfs may have file backed by THP that mapped nowhere. >> >> Update selftest program to test file-backed THP split too. >> >> Suggested-by: Kirill A. Shutemov >> Signed-off-by: Zi Yan >> --- >> mm/huge_memory.c | 95 ++- >> .../selftests/vm/split_huge_page_test.c | 79 ++- >> 2 files changed, 166 insertions(+), 8 deletions(-) >> >> diff --git a/mm/huge_memory.c b/mm/huge_memory.c >> index 3bfee54e2cd0..da91ee97d944 100644 >> --- a/mm/huge_memory.c >> +++ b/mm/huge_memory.c >> @@ -3043,12 +3043,72 @@ static int split_huge_pages_pid(int pid, unsigned >> long vaddr_start, >> return ret; >> } >> >> +static int split_huge_pages_in_file(const char *file_path, pgoff_t >> off_start, >> + pgoff_t off_end) >> +{ >> + struct filename *file; >> + struct file *candidate; >> + struct address_space *mapping; >> + int ret = -EINVAL; >> + pgoff_t off_cur; >> + unsigned long total = 0, split = 0; >> + >> + file = getname_kernel(file_path); >> + if (IS_ERR(file)) >> + return ret; >> + >> + candidate = file_open_name(file, O_RDONLY, 0); >> + if (IS_ERR(candidate)) >> + goto out; >> + >> + pr_info("split file-backed THPs in file: %s, offset: [0x%lx - >> 0x%lx]\n", >> +file_path, off_start, off_end); >> + >> + mapping = candidate->f_mapping; >> + >> + for (off_cur = off_start; off_cur < off_end;) { >> + struct page *fpage = pagecache_get_page(mapping, off_cur, >> + FGP_ENTRY | FGP_HEAD, 0); >> + >> + if (xa_is_value(fpage) || !fpage) { > > Why do you have FGP_ENTRY? It seems it would return page instead of > NULL if page is value. So I think you could remove FGP_ENTRY and > xa_is_value() check as well. The comment on FGP_ENTRY says “If there is a shadow/swap/DAX entry, return it instead of allocating a new page to replace it”. I do not think we want to allocate new pages here. I mostly follow the use of pagecache_get_page() in shmem_getpage_gfp without swapin or allocating new pages. > >> + off_cur += PAGE_SIZE; >> + continue; >> + } >> + >> + if (!is_transparent_hugepage(fpage)) { >> + off_cur += PAGE_SIZE; >> + goto next; >> + } >> + total++; >> + off_cur = fpage->index + thp_size(fpage); >> + >> + if (!trylock_page(fpage)) >> + goto next; >> + >> + if (!split_huge_page(fpage)) >> + split++; >> + >> + unlock_page(fpage); >> +next: >> + put_page(fpage); >> + } >> + >> + filp_close(candidate, NULL); >> + ret = 0; >> + >> + pr_info("%lu of %lu file-backed THP split\n", split, total); >> +out: >> + putname(file); >> + return ret; >> +} >> + >> static ssize_t split_huge_pages_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 */ >> + /* hold pid, start_vaddr, end_vaddr or file_path, off_start, off_end >> */ >> + char input_buf[MAX_INPUT]; > > I didn't find where MAX_INPUT is defined in your patch. Just saw > include/uapi/linux/limits.h have it defined. Is it the one you really > refer to? Yeah, I want to use 255 as the max input size and find MAX_INPUT. From your comment, I think it is better to define a MACRO here explicitly. >> int pid; >> unsigned long vaddr_start, vaddr_end; >> >> @@ -3058,11 +3118,40 @@ static ssize_t split_huge_pages_write(struct file >> *file, const char __user *buf, >> >> ret = -EFAULT; >> >> - memset(input_buf, 0, 80); >> + memset(input_buf, 0, MAX_INPUT); >> if (copy_from_user(input_buf, buf, min_t(size_t, count, 80))) >> goto out; >> >> - input_buf[79] = '\0'; >> + input_buf[MAX_INPUT - 1] = '\0'; >> + >> + if (input_buf[0] == '/') { >> + char *tok; >> + char *buf = input_buf; >> + char file_path[MAX_INPUT]; >> + pgoff_t off_start = 0, off_end = 0; >> + size_t input_len = strlen(input_buf); >> + >> + tok = strsep(&buf, ","); >> + if (tok) { >> + strncpy(file_path, tok, MAX_INPUT); >> + } else { >> + ret = -EINVAL; >> + goto out; >> +
Re: [PATCH v4 2/2] mm: huge_memory: debugfs for file-backed THP split.
On Mon, Mar 15, 2021 at 1:34 PM Zi Yan wrote: > > From: Zi Yan > > Further extend /split_huge_pages to accept > ",," for file-backed THP split tests since > tmpfs may have file backed by THP that mapped nowhere. > > Update selftest program to test file-backed THP split too. > > Suggested-by: Kirill A. Shutemov > Signed-off-by: Zi Yan > --- > mm/huge_memory.c | 95 ++- > .../selftests/vm/split_huge_page_test.c | 79 ++- > 2 files changed, 166 insertions(+), 8 deletions(-) > > diff --git a/mm/huge_memory.c b/mm/huge_memory.c > index 3bfee54e2cd0..da91ee97d944 100644 > --- a/mm/huge_memory.c > +++ b/mm/huge_memory.c > @@ -3043,12 +3043,72 @@ static int split_huge_pages_pid(int pid, unsigned > long vaddr_start, > return ret; > } > > +static int split_huge_pages_in_file(const char *file_path, pgoff_t off_start, > + pgoff_t off_end) > +{ > + struct filename *file; > + struct file *candidate; > + struct address_space *mapping; > + int ret = -EINVAL; > + pgoff_t off_cur; > + unsigned long total = 0, split = 0; > + > + file = getname_kernel(file_path); > + if (IS_ERR(file)) > + return ret; > + > + candidate = file_open_name(file, O_RDONLY, 0); > + if (IS_ERR(candidate)) > + goto out; > + > + pr_info("split file-backed THPs in file: %s, offset: [0x%lx - > 0x%lx]\n", > +file_path, off_start, off_end); > + > + mapping = candidate->f_mapping; > + > + for (off_cur = off_start; off_cur < off_end;) { > + struct page *fpage = pagecache_get_page(mapping, off_cur, > + FGP_ENTRY | FGP_HEAD, 0); > + > + if (xa_is_value(fpage) || !fpage) { Why do you have FGP_ENTRY? It seems it would return page instead of NULL if page is value. So I think you could remove FGP_ENTRY and xa_is_value() check as well. > + off_cur += PAGE_SIZE; > + continue; > + } > + > + if (!is_transparent_hugepage(fpage)) { > + off_cur += PAGE_SIZE; > + goto next; > + } > + total++; > + off_cur = fpage->index + thp_size(fpage); > + > + if (!trylock_page(fpage)) > + goto next; > + > + if (!split_huge_page(fpage)) > + split++; > + > + unlock_page(fpage); > +next: > + put_page(fpage); > + } > + > + filp_close(candidate, NULL); > + ret = 0; > + > + pr_info("%lu of %lu file-backed THP split\n", split, total); > +out: > + putname(file); > + return ret; > +} > + > static ssize_t split_huge_pages_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 */ > + /* hold pid, start_vaddr, end_vaddr or file_path, off_start, off_end > */ > + char input_buf[MAX_INPUT]; I didn't find where MAX_INPUT is defined in your patch. Just saw include/uapi/linux/limits.h have it defined. Is it the one you really refer to? > int pid; > unsigned long vaddr_start, vaddr_end; > > @@ -3058,11 +3118,40 @@ static ssize_t split_huge_pages_write(struct file > *file, const char __user *buf, > > ret = -EFAULT; > > - memset(input_buf, 0, 80); > + memset(input_buf, 0, MAX_INPUT); > if (copy_from_user(input_buf, buf, min_t(size_t, count, 80))) > goto out; > > - input_buf[79] = '\0'; > + input_buf[MAX_INPUT - 1] = '\0'; > + > + if (input_buf[0] == '/') { > + char *tok; > + char *buf = input_buf; > + char file_path[MAX_INPUT]; > + pgoff_t off_start = 0, off_end = 0; > + size_t input_len = strlen(input_buf); > + > + tok = strsep(&buf, ","); > + if (tok) { > + strncpy(file_path, tok, MAX_INPUT); > + } else { > + ret = -EINVAL; > + goto out; > + } > + > + ret = sscanf(buf, "0x%lx,0x%lx", &off_start, &off_end); > + if (ret != 2) { > + pr_info("ret: %ld\n", ret); > + ret = -EINVAL; > + goto out; > + } > + ret = split_huge_pages_in_file(file_path, off_start, off_end); > + if (!ret) > + ret = input_len; > + > + goto out; > + } > + > ret = sscanf(input_buf, "%d,0x%lx,0x%lx", &pid, &vaddr_start, > &vaddr_end); > if (ret == 1 && pid == 1) { > split
[PATCH v4 2/2] mm: huge_memory: debugfs for file-backed THP split.
From: Zi Yan Further extend /split_huge_pages to accept ",," for file-backed THP split tests since tmpfs may have file backed by THP that mapped nowhere. Update selftest program to test file-backed THP split too. Suggested-by: Kirill A. Shutemov Signed-off-by: Zi Yan --- mm/huge_memory.c | 95 ++- .../selftests/vm/split_huge_page_test.c | 79 ++- 2 files changed, 166 insertions(+), 8 deletions(-) diff --git a/mm/huge_memory.c b/mm/huge_memory.c index 3bfee54e2cd0..da91ee97d944 100644 --- a/mm/huge_memory.c +++ b/mm/huge_memory.c @@ -3043,12 +3043,72 @@ static int split_huge_pages_pid(int pid, unsigned long vaddr_start, return ret; } +static int split_huge_pages_in_file(const char *file_path, pgoff_t off_start, + pgoff_t off_end) +{ + struct filename *file; + struct file *candidate; + struct address_space *mapping; + int ret = -EINVAL; + pgoff_t off_cur; + unsigned long total = 0, split = 0; + + file = getname_kernel(file_path); + if (IS_ERR(file)) + return ret; + + candidate = file_open_name(file, O_RDONLY, 0); + if (IS_ERR(candidate)) + goto out; + + pr_info("split file-backed THPs in file: %s, offset: [0x%lx - 0x%lx]\n", +file_path, off_start, off_end); + + mapping = candidate->f_mapping; + + for (off_cur = off_start; off_cur < off_end;) { + struct page *fpage = pagecache_get_page(mapping, off_cur, + FGP_ENTRY | FGP_HEAD, 0); + + if (xa_is_value(fpage) || !fpage) { + off_cur += PAGE_SIZE; + continue; + } + + if (!is_transparent_hugepage(fpage)) { + off_cur += PAGE_SIZE; + goto next; + } + total++; + off_cur = fpage->index + thp_size(fpage); + + if (!trylock_page(fpage)) + goto next; + + if (!split_huge_page(fpage)) + split++; + + unlock_page(fpage); +next: + put_page(fpage); + } + + filp_close(candidate, NULL); + ret = 0; + + pr_info("%lu of %lu file-backed THP split\n", split, total); +out: + putname(file); + return ret; +} + static ssize_t split_huge_pages_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 */ + /* hold pid, start_vaddr, end_vaddr or file_path, off_start, off_end */ + char input_buf[MAX_INPUT]; int pid; unsigned long vaddr_start, vaddr_end; @@ -3058,11 +3118,40 @@ static ssize_t split_huge_pages_write(struct file *file, const char __user *buf, ret = -EFAULT; - memset(input_buf, 0, 80); + memset(input_buf, 0, MAX_INPUT); if (copy_from_user(input_buf, buf, min_t(size_t, count, 80))) goto out; - input_buf[79] = '\0'; + input_buf[MAX_INPUT - 1] = '\0'; + + if (input_buf[0] == '/') { + char *tok; + char *buf = input_buf; + char file_path[MAX_INPUT]; + pgoff_t off_start = 0, off_end = 0; + size_t input_len = strlen(input_buf); + + tok = strsep(&buf, ","); + if (tok) { + strncpy(file_path, tok, MAX_INPUT); + } else { + ret = -EINVAL; + goto out; + } + + ret = sscanf(buf, "0x%lx,0x%lx", &off_start, &off_end); + if (ret != 2) { + pr_info("ret: %ld\n", ret); + ret = -EINVAL; + goto out; + } + ret = split_huge_pages_in_file(file_path, off_start, off_end); + if (!ret) + ret = input_len; + + goto out; + } + ret = sscanf(input_buf, "%d,0x%lx,0x%lx", &pid, &vaddr_start, &vaddr_end); if (ret == 1 && pid == 1) { split_huge_pages_all(); diff --git a/tools/testing/selftests/vm/split_huge_page_test.c b/tools/testing/selftests/vm/split_huge_page_test.c index 9f33ddbb3182..0202702f7eda 100644 --- a/tools/testing/selftests/vm/split_huge_page_test.c +++ b/tools/testing/selftests/vm/split_huge_page_test.c @@ -7,11 +7,13 @@ #define _GNU_SOURCE #include #include +#include #include #include #include #include #include +#include #include #include @@ -24,6 +26,9 @@ uint64_t pmd_pagesize; #define SMAP_PATH "/proc/self/smaps" #define INPUT_MAX 80 +#define PID_FMT "%d,0x%lx,0x%lx" +#define PATH_FMT "%s,0x%lx,0x%lx" + #define P