On Fri, Jun 3, 2022 at 5:40 PM Zach O'Keefe <zokeefe@xxxxxxxxxx> wrote:
>
> Add a gfp_t flags member to struct collapse_control that allows contexts
> to specify their own allocation semantics. This decouples the
> allocation semantics from
> /sys/kernel/mm/transparent_hugepage/khugepaged/defrag.
>
> khugepaged updates this member for every hugepage processed, since the
> sysfs setting might change at any time.
>
> Signed-off-by: Zach O'Keefe <zokeefe@xxxxxxxxxx>
> ---
> mm/khugepaged.c | 21 +++++++++++++--------
> 1 file changed, 13 insertions(+), 8 deletions(-)
>
> diff --git a/mm/khugepaged.c b/mm/khugepaged.c
> index 38488d114073..ba722347bebd 100644
> --- a/mm/khugepaged.c
> +++ b/mm/khugepaged.c
> @@ -92,6 +92,9 @@ struct collapse_control {
>
> /* Last target selected in khugepaged_find_target_node() */
> int last_target_node;
> +
> + /* gfp used for allocation and memcg charging */
> + gfp_t gfp;
> };
>
> /**
> @@ -994,15 +997,14 @@ static bool __collapse_huge_page_swapin(struct mm_struct *mm,
> return true;
> }
>
> -static int alloc_charge_hpage(struct page **hpage, struct mm_struct *mm,
> +static int alloc_charge_hpage(struct mm_struct *mm, struct page **hpage,
Why did you have to reverse the order of mm and hpage? It seems
pointless and you could save a couple of changed lines.
> struct collapse_control *cc)
> {
> - gfp_t gfp = alloc_hugepage_khugepaged_gfpmask() | __GFP_THISNODE;
> int node = khugepaged_find_target_node(cc);
>
> - if (!khugepaged_alloc_page(hpage, gfp, node))
> + if (!khugepaged_alloc_page(hpage, cc->gfp, node))
> return SCAN_ALLOC_HUGE_PAGE_FAIL;
> - if (unlikely(mem_cgroup_charge(page_folio(*hpage), mm, gfp)))
> + if (unlikely(mem_cgroup_charge(page_folio(*hpage), mm, cc->gfp)))
> return SCAN_CGROUP_CHARGE_FAIL;
> count_memcg_page_event(*hpage, THP_COLLAPSE_ALLOC);
> return SCAN_SUCCEED;
> @@ -1032,7 +1034,7 @@ static void collapse_huge_page(struct mm_struct *mm, unsigned long address,
> */
> mmap_read_unlock(mm);
>
> - result = alloc_charge_hpage(hpage, mm, cc);
> + result = alloc_charge_hpage(mm, hpage, cc);
> if (result != SCAN_SUCCEED)
> goto out_nolock;
>
> @@ -1613,7 +1615,7 @@ static void collapse_file(struct mm_struct *mm, struct file *file,
> VM_BUG_ON(!IS_ENABLED(CONFIG_READ_ONLY_THP_FOR_FS) && !is_shmem);
> VM_BUG_ON(start & (HPAGE_PMD_NR - 1));
>
> - result = alloc_charge_hpage(hpage, mm, cc);
> + result = alloc_charge_hpage(mm, hpage, cc);
> if (result != SCAN_SUCCEED)
> goto out;
>
> @@ -2037,8 +2039,7 @@ static void khugepaged_scan_file(struct mm_struct *mm, struct file *file,
> }
> #else
> static void khugepaged_scan_file(struct mm_struct *mm, struct file *file,
> - pgoff_t start, struct page **hpage,
> - struct collapse_control *cc)
> + pgoff_t start, struct collapse_control *cc)
Why was the !CONFIG_SHMEM version definition changed, but CONFIG_SHMEM
version was not?
> {
> BUILD_BUG();
> }
> @@ -2121,6 +2122,9 @@ static unsigned int khugepaged_scan_mm_slot(unsigned int pages,
> if (unlikely(khugepaged_test_exit(mm)))
> goto breakouterloop;
>
> + /* reset gfp flags since sysfs settings might change */
> + cc->gfp = alloc_hugepage_khugepaged_gfpmask() |
> + __GFP_THISNODE;
> VM_BUG_ON(khugepaged_scan.address < hstart ||
> khugepaged_scan.address + HPAGE_PMD_SIZE >
> hend);
> @@ -2255,6 +2259,7 @@ static int khugepaged(void *none)
> struct mm_slot *mm_slot;
> struct collapse_control cc = {
> .last_target_node = NUMA_NO_NODE,
> + /* .gfp set later */
Seems pointless to me.
> };
>
> set_freezable();
> --
> 2.36.1.255.ge46751e96f-goog
>
