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

On 14/12/2023 11:30, Dan Carpenter wrote:

On Thu, Dec 14, 2023 at 10:54:19AM +0000, Ryan Roberts wrote:

quoted

On 13/12/2023 07:21, Dan Carpenter wrote:

quoted

On Thu, Dec 07, 2023 at 04:12:05PM +0000, Ryan Roberts wrote:

quoted

@@ -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.

It's the same pattern that the existing code a little further down this function
already implements:

	vmf->pte = pte_offset_map_lock(vma->vm_mm, vmf->pmd, addr, &vmf->ptl);
	if (!vmf->pte)
		goto release;

If we fail to map/lock the pte (due to a race), then we return 0 to allow user
space to rerun the faulting instruction and cause the fault to happen again. The
above code ends up calling "return ret;" and ret is 0.

Ah, okay.  Thanks!

quoted

quoted

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.

There are 3 conditions that the function can return:

 - folio successfully allocated
 - folio failed to be allocated due to OOM
 - fault needs to be tried again due to losing race

Previously only the first 2 conditions were possible and they were indicated by
NULL/not-NULL. The new 3rd condition is only possible when THP is compile-time
enabled. So it keeps the logic simpler to keep the NULL/not-NULL distinction for
the first 2, and use the error code for the final one.

There are IS_ERR() and IS_ERR_OR_NULL() variants so I assume a pattern where you
can have pointer, error or NULL is somewhat common already?

People are confused by this a lot so I have written a blog about it:

https://staticthinking.wordpress.com/2022/08/01/mixing-error-pointers-and-null/

Nice; thanks for the pointer :)

The IS_ERR_OR_NULL() function should be used like this:

int blink_leds()
{
	led = get_leds();
	if (IS_ERR_OR_NULL(led))
		return PTR_ERR(led);  <-- NULL means zero/success
	return led->blink();
}

In the case of alloc_anon_folio(), I would be tempted to create a
wrapper around it where NULL becomes ERR_PTR(-ENOMEM).  But this is
obviously fast path code and I haven't benchmarked it.

Adding a comment is the other option.

I'll add a comment; as you say this is a fast path, and I'm actively being
burned in similar places (on another series I'm working on) where an additional
check is regressing performance significantly so not keen on risking it here.

Andrew, I'll fold in the David's suggested ifdef improvement at the same time.
Would you prefer an additional patch to squash in, or a whole new version of the
series to swap out with the existing patches in mm-unstable?

regards,
dan carpenter

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel