Re: [PATCH] selftests/mm: use __auto_type in swap() macro

[Joshua Hahn <joshua.hahnjy@gmail.com>]

On Wed, 30 Jul 2025 19:53:01 +0530 Pranav Tyagi <pranav.tyagi03@gmail.com> wrote:

Hi Pranav,

Thank you for this patch! Sorry for the late response, thank you for bumping
the patch. I just have a few comments about the commit message.

> Replace typeof() with __auto_type in the swap() macro in uffd-stress.c.
> __auto_type was introduced in GCC 4.9 and reduces the compile time for
> all compilers. No functional changes intended.

From what I understand, the compiler optimizations from typeof() --> __auto_type
applies when the argument is a variably modified type. It seems like this
internal definition of swap() is only used within selftests/mm/uffd-stress.c,
and in particular is only used twice, in these two lines:

/* prepare next bounce */
swap(area_src, area_dst);

swap(area_src_alias, area_dst_alias);

Where area_src, area_dst, area_src_alias, area_dst_alias are all char * which
the compiler knows the size of at compile time. Given this, I wonder if there
are any compile time speedups.

But this is just my understanding, based on a quick look at the code. Please
feel free to correct me, if I am incorrectly understanding your changes or if
my understanding of __auto_type is incorrect : -)

With that said, I think the main benefit that we get from using __auto_type
has more to do with readability. Since __auto_type can only be used to
declare local variables (as we are doing here), maybe we can rephrase the
commit to be about improving readability, and not compile time?

Again, please let me know if I am overlooking something.

One other thought -- while this internal definition of swap() is confined to
selftests/mm/uffd-stress.c, there is another identical definition in
include/linux/minmax.h that may be used more widely across not only mm
stresstests, but across subsystems as well. Without having taken a deeper
look into this, perhaps this version of swap is indeed called on some data
structures with variable type, and we might be able to see some tangible
compile time improvements?

In any case, this change looks good to me, but maybe a new commit message
that can more closely describe the effects would be better : -) Once you add
those changes, please feel free to add my review tag for the mm selftest change:

Reviewed-by: Joshua Hahn <joshua.hahnjy@gmail.com>

> Signed-off-by: Pranav Tyagi <pranav.tyagi03@gmail.com>
> ---
>  tools/testing/selftests/mm/uffd-stress.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/testing/selftests/mm/uffd-stress.c b/tools/testing/selftests/mm/uffd-stress.c
> index 40af7f67c407..c0f64df5085c 100644
> --- a/tools/testing/selftests/mm/uffd-stress.c
> +++ b/tools/testing/selftests/mm/uffd-stress.c
> @@ -51,7 +51,7 @@ static char *zeropage;
>  pthread_attr_t attr;
>  
>  #define swap(a, b) \
> -	do { typeof(a) __tmp = (a); (a) = (b); (b) = __tmp; } while (0)
> +	do { __auto_type __tmp = (a); (a) = (b); (b) = __tmp; } while (0)
>  
>  const char *examples =
>  	"# Run anonymous memory test on 100MiB region with 99999 bounces:\n"
> -- 
> 2.49.0

Sent using hkml (https://github.com/sjp38/hackermail)