Re: [PATCH v9 04/10] mm: thp: Support allocation of anonymous multi-size THP

[Dan Carpenter <dan.carpenter@linaro.org>]

On Thu, Dec 07, 2023 at 04:12:05PM +0000, Ryan Roberts wrote:
> @@ -4176,10 +4260,15 @@ static vm_fault_t do_anonymous_page(struct vm_fault *vmf)
>  	/* Allocate our own private page. */
>  	if (unlikely(anon_vma_prepare(vma)))
>  		goto oom;
> -	folio = vma_alloc_zeroed_movable_folio(vma, vmf->address);
> +	folio = alloc_anon_folio(vmf);
> +	if (IS_ERR(folio))
> +		return 0;
>  	if (!folio)
>  		goto oom;

Returning zero is weird.  I think it should be a vm_fault_t code.

This mixing of error pointers and NULL is going to cause problems.
Normally when we have a mix of error pointers and NULL then the NULL is
not an error but instead means that the feature has been deliberately
turned off.  I'm unable to figure out what the meaning is here.

It should return one or the other, or if it's a mix then add a giant
comment explaining what they mean.

regards,
dan carpenter

>  
> +	nr_pages = folio_nr_pages(folio);
> +	addr = ALIGN_DOWN(vmf->address, nr_pages * PAGE_SIZE);
> +
>  	if (mem_cgroup_charge(folio, vma->vm_mm, GFP_KERNEL))
>  		goto oom_free_page;
>  	folio_throttle_swaprate(folio, GFP_KERNEL);