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(, ",");
>> + 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(, ",");
> + if (tok) {
> + strncpy(file_path, tok, MAX_INPUT);
> + } else {
> + ret = -EINVAL;
> + goto out;
> + }
> +
> + ret = sscanf(buf, "0x%lx,0x%lx", _start, _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", , _start,
> _end);
> if (ret == 1 && pid == 1) {
> split_huge_pages_all();
> diff
