On Wed, Aug 6, 2025 at 10:18 PM Joshua Hahn <joshua.hah...@gmail.com> wrote:
>
> On Wed, 30 Jul 2025 19:53:01 +0530 Pranav Tyagi <pranav.tyag...@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.hah...@gmail.com>
>
> > Signed-off-by: Pranav Tyagi <pranav.tyag...@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)
Hi Joshua,
Thanks for the detailed review and I sincerely apologize for the delayed
response.
You’re correct — __auto_type mainly provides compiler optimizations when the
argument is a variably modified type and in this case the compiler already
knows the size of the arguments at compile time. The motivation here is more
about readability and consistency, as __auto_type is already used in several
places within selftests. This patch has since been picked up into the
mm-unstable branch.
I also looked at the swap() definition in include/linux/minmax.h, which is
indeed more widely used across subsystems. That version may offer tangible
compile-time improvements in cases involving variable argument types and I
plan to follow up with a patch there.
Thanks again for the thorough review.
Regards
Pranav Tyagi
