[PATCH 0/2] selftests: Introduce common min()/max() and apply them in net tests

[PATCH 0/2] selftests: Introduce common min()/max() and apply them in net tests

[Mahmoud Maatuq]

The first patch introduces a common definition of 
the min() and max() macros for use in multiple files under selftests. 
This avoids the redundancy of having scattered local definitions 
and prepares the groundwork for other files to simply include kselftest.h.

The second patch then applies these new macros in the selftests/net files, 
replacing the ternary operations. This directly addresses several 
coccicheck warnings, making the code cleaner and more maintainable

Mahmoud Maatuq (2):
  selftests: Provide local define of min() and max()
  selftests/net: replace ternary operator with min()/max()

 tools/testing/selftests/kselftest.h           | 7 +++++++
 tools/testing/selftests/net/Makefile          | 2 ++
 tools/testing/selftests/net/so_txtime.c       | 7 ++++---
 tools/testing/selftests/net/udpgso_bench_tx.c | 6 +++---
 4 files changed, 16 insertions(+), 6 deletions(-)

-- 
2.34.1

[PATCH 1/2] selftests: Provide local define of min() and max()

[Mahmoud Maatuq]

to avoid manual calculation of min and max values
and fix coccinelle warnings such WARNING opportunity for min()/max()
adding one common definition that could be used in multiple files
under selftests.
there are also some defines for min/max scattered locally inside sources
under selftests.
this also prepares for cleaning up those redundant defines and include
kselftest.h instead.

Signed-off-by: Mahmoud Maatuq <mahmoudmatook.mm@gmail.com>
---
 tools/testing/selftests/kselftest.h | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/tools/testing/selftests/kselftest.h b/tools/testing/selftests/kselftest.h
index 829be379545a..e8eb7e9afbc6 100644
--- a/tools/testing/selftests/kselftest.h
+++ b/tools/testing/selftests/kselftest.h
@@ -55,6 +55,13 @@
 #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]))
 #endif
 
+#ifndef min
+# define min(x, y) ((x) < (y) ? (x) : (y))
+#endif
+#ifndef max
+# define max(x, y) ((x) < (y) ? (y) : (x))
+#endif
+
 /*
  * gcc cpuid.h provides __cpuid_count() since v4.4.
  * Clang/LLVM cpuid.h provides  __cpuid_count() since v3.4.0.
-- 
2.34.1

Re: [PATCH 1/2] selftests: Provide local define of min() and max()

[Willem de Bruijn]

Mahmoud Maatuq wrote:
> to avoid manual calculation of min and max values
> and fix coccinelle warnings such WARNING opportunity for min()/max()
> adding one common definition that could be used in multiple files
> under selftests.
> there are also some defines for min/max scattered locally inside sources
> under selftests.
> this also prepares for cleaning up those redundant defines and include
> kselftest.h instead.
> 
> Signed-off-by: Mahmoud Maatuq <mahmoudmatook.mm@gmail.com>
> ---
>  tools/testing/selftests/kselftest.h | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/tools/testing/selftests/kselftest.h b/tools/testing/selftests/kselftest.h
> index 829be379545a..e8eb7e9afbc6 100644
> --- a/tools/testing/selftests/kselftest.h
> +++ b/tools/testing/selftests/kselftest.h
> @@ -55,6 +55,13 @@
>  #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]))
>  #endif
>  
> +#ifndef min
> +# define min(x, y) ((x) < (y) ? (x) : (y))
> +#endif
> +#ifndef max
> +# define max(x, y) ((x) < (y) ? (y) : (x))
> +#endif
> +

Should this more closely follow include/linux/minmax.h, which is a lot
more strict?

I'm fine with this simpler, more relaxed, version for testing, but
calling it out for people to speak up.

Only the first two of these comments in minmax.h apply to this
userspace code.

/*
 * min()/max()/clamp() macros must accomplish three things:
 *
 * - avoid multiple evaluations of the arguments (so side-effects like
 *   "x++" happen only once) when non-constant.
 * - perform strict type-checking (to generate warnings instead of
 *   nasty runtime surprises). See the "unnecessary" pointer comparison
 *   in __typecheck().
 * - retain result as a constant expressions when called with only
 *   constant expressions (to avoid tripping VLA warnings in stack
 *   allocation usage).
 */

Note that a more strict version that includes __typecheck would
warn on the type difference between total_len and cfg_mss. Fine
with changing the type of cfg_mss in the follow-on patch to address
that.

RE: [PATCH 1/2] selftests: Provide local define of min() and max()

[David Laight]

From: Willem de Bruijn
> Sent: Sunday, August 20, 2023 4:15 PM
> 
> Mahmoud Maatuq wrote:
> > to avoid manual calculation of min and max values
> > and fix coccinelle warnings such WARNING opportunity for min()/max()
> > adding one common definition that could be used in multiple files
> > under selftests.
> > there are also some defines for min/max scattered locally inside sources
> > under selftests.
> > this also prepares for cleaning up those redundant defines and include
> > kselftest.h instead.
> >
> > Signed-off-by: Mahmoud Maatuq <mahmoudmatook.mm@gmail.com>
> > ---
> >  tools/testing/selftests/kselftest.h | 7 +++++++
> >  1 file changed, 7 insertions(+)
> >
> > diff --git a/tools/testing/selftests/kselftest.h b/tools/testing/selftests/kselftest.h
> > index 829be379545a..e8eb7e9afbc6 100644
> > --- a/tools/testing/selftests/kselftest.h
> > +++ b/tools/testing/selftests/kselftest.h
> > @@ -55,6 +55,13 @@
> >  #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]))
> >  #endif
> >
> > +#ifndef min
> > +# define min(x, y) ((x) < (y) ? (x) : (y))
> > +#endif
> > +#ifndef max
> > +# define max(x, y) ((x) < (y) ? (y) : (x))
> > +#endif
> > +
> 
> Should this more closely follow include/linux/minmax.h, which is a lot
> more strict?
> 
> I'm fine with this simpler, more relaxed, version for testing, but
> calling it out for people to speak up.
> 
> Only the first two of these comments in minmax.h apply to this
> userspace code.
> 
> /*
>  * min()/max()/clamp() macros must accomplish three things:
>  *
>  * - avoid multiple evaluations of the arguments (so side-effects like
>  *   "x++" happen only once) when non-constant.
>  * - perform strict type-checking (to generate warnings instead of
>  *   nasty runtime surprises). See the "unnecessary" pointer comparison
>  *   in __typecheck().
>  * - retain result as a constant expressions when called with only
>  *   constant expressions (to avoid tripping VLA warnings in stack
>  *   allocation usage).
>  */
> 
> Note that a more strict version that includes __typecheck would
> warn on the type difference between total_len and cfg_mss. Fine
> with changing the type of cfg_mss in the follow-on patch to address
> that.

That typecheck() is horrid.
It may well have caused more bugs due to incorrect casts that
it actually detected.

I'd suggest the version that just avoids multiple evaluations.
Or just error signed v unsigned comparisons.
See  https://lore.kernel.org/all/b4ce9dad748e489f9314a2dc95615033@AcuMS.aculab.com/
for an example patch set.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH 1/2] selftests: Provide local define of min() and max()

[Willem de Bruijn]

On Mon, Aug 21, 2023 at 9:05 AM David Laight <David.Laight@aculab.com> wrote:
>
> From: Willem de Bruijn
> > Sent: Sunday, August 20, 2023 4:15 PM
> >
> > Mahmoud Maatuq wrote:
> > > to avoid manual calculation of min and max values
> > > and fix coccinelle warnings such WARNING opportunity for min()/max()
> > > adding one common definition that could be used in multiple files
> > > under selftests.
> > > there are also some defines for min/max scattered locally inside sources
> > > under selftests.
> > > this also prepares for cleaning up those redundant defines and include
> > > kselftest.h instead.
> > >
> > > Signed-off-by: Mahmoud Maatuq <mahmoudmatook.mm@gmail.com>
> > > ---
> > >  tools/testing/selftests/kselftest.h | 7 +++++++
> > >  1 file changed, 7 insertions(+)
> > >
> > > diff --git a/tools/testing/selftests/kselftest.h b/tools/testing/selftests/kselftest.h
> > > index 829be379545a..e8eb7e9afbc6 100644
> > > --- a/tools/testing/selftests/kselftest.h
> > > +++ b/tools/testing/selftests/kselftest.h
> > > @@ -55,6 +55,13 @@
> > >  #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]))
> > >  #endif
> > >
> > > +#ifndef min
> > > +# define min(x, y) ((x) < (y) ? (x) : (y))
> > > +#endif
> > > +#ifndef max
> > > +# define max(x, y) ((x) < (y) ? (y) : (x))
> > > +#endif
> > > +
> >
> > Should this more closely follow include/linux/minmax.h, which is a lot
> > more strict?
> >
> > I'm fine with this simpler, more relaxed, version for testing, but
> > calling it out for people to speak up.
> >
> > Only the first two of these comments in minmax.h apply to this
> > userspace code.
> >
> > /*
> >  * min()/max()/clamp() macros must accomplish three things:
> >  *
> >  * - avoid multiple evaluations of the arguments (so side-effects like
> >  *   "x++" happen only once) when non-constant.
> >  * - perform strict type-checking (to generate warnings instead of
> >  *   nasty runtime surprises). See the "unnecessary" pointer comparison
> >  *   in __typecheck().
> >  * - retain result as a constant expressions when called with only
> >  *   constant expressions (to avoid tripping VLA warnings in stack
> >  *   allocation usage).
> >  */
> >
> > Note that a more strict version that includes __typecheck would
> > warn on the type difference between total_len and cfg_mss. Fine
> > with changing the type of cfg_mss in the follow-on patch to address
> > that.
>
> That typecheck() is horrid.
> It may well have caused more bugs due to incorrect casts that
> it actually detected.
>
> I'd suggest the version that just avoids multiple evaluations.
> Or just error signed v unsigned comparisons.
> See  https://lore.kernel.org/all/b4ce9dad748e489f9314a2dc95615033@AcuMS.aculab.com/
> for an example patch set.

Interesting, thanks. That is also simpler.

Also, the existing patch is no worse than the open coded code today,
so even without code to avoid multiple evaluations, I guess it's okay
to merge.

The coccinelle warnings are arguably false positives, using checks for
kernel code, but being run against userspace code that has no access
to those helpers. But fine to silence them.

RE: [PATCH 1/2] selftests: Provide local define of min() and max()

[David Laight]

...
> > That typecheck() is horrid.
> > It may well have caused more bugs due to incorrect casts that
> > it actually detected.
> >
> > I'd suggest the version that just avoids multiple evaluations.
> > Or just error signed v unsigned comparisons.
> > See  https://lore.kernel.org/all/b4ce9dad748e489f9314a2dc95615033@AcuMS.aculab.com/
> > for an example patch set.
> 
> Interesting, thanks. That is also simpler.
> 
> Also, the existing patch is no worse than the open coded code today,
> so even without code to avoid multiple evaluations, I guess it's okay
> to merge.
> 
> The coccinelle warnings are arguably false positives, using checks for
> kernel code, but being run against userspace code that has no access
> to those helpers. But fine to silence them.

You can't use is_constexpr() unless 'sizeof *(void *)' is valid.
And builtin_constant() isn't good enough for builtin_choose_expr().

That might be ok for selftests and tools, but not for generaluserspace.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH 1/2] selftests: Provide local define of min() and max()

[Mahmoud Matook]

On 08/22, David Laight wrote:

> ...
> > > That typecheck() is horrid.
> > > It may well have caused more bugs due to incorrect casts that
> > > it actually detected.
> > >
> > > I'd suggest the version that just avoids multiple evaluations.
> > > Or just error signed v unsigned comparisons.
> > > See  https://lore.kernel.org/all/b4ce9dad748e489f9314a2dc95615033@AcuMS.aculab.com/
> > > for an example patch set.
> > 
> > Interesting, thanks. That is also simpler.
> > 
> > Also, the existing patch is no worse than the open coded code today,
> > so even without code to avoid multiple evaluations, I guess it's okay
> > to merge.
> > 
> > The coccinelle warnings are arguably false positives, using checks for
> > kernel code, but being run against userspace code that has no access
> > to those helpers. But fine to silence them.
> 
> You can't use is_constexpr() unless 'sizeof *(void *)' is valid.
> And builtin_constant() isn't good enough for builtin_choose_expr().
> 
> That might be ok for selftests and tools, but not for generaluserspace.
> 
> 	David
> 
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)
I tried to use the relaxed version provided in the shared patchset link 
besides not able to use is_constexpr(), I'm not able to use
__UNIQUE_ID() also. It's definded inside include/linux/compiler-gcc.h
and it uses another macro __PASTE() which is defined inside
include/linux/compiler_types.h. 
not sure what to do next
- bring those macros definitions to able to use the relaxed version.
- if the most important point for min/max defines inside selftests is to
  avoid multiple evaluation is the below version acceptable?
  /*
  #define min(x, y) ({ \
    typeof(x) _x = (x); \
    typeof(y) _y = (y); \
    _x < _y ? _x : _y; \
})

#define max(x, y) ({ \
    typeof(x) _x = (x); \
    typeof(y) _y = (y); \
    _x > _y ? _x : _y; \
})
*/

RE: [PATCH 1/2] selftests: Provide local define of min() and max()

[David Laight]

From: Mahmoud Matook
> Sent: Wednesday, August 23, 2023 8:36 PM
...
> I tried to use the relaxed version provided in the shared patchset link
> besides not able to use is_constexpr(), I'm not able to use
> __UNIQUE_ID() also. It's definded inside include/linux/compiler-gcc.h
> and it uses another macro __PASTE() which is defined inside
> include/linux/compiler_types.h.
> not sure what to do next
>
> - bring those macros definitions to able to use the relaxed version.
> - if the most important point for min/max defines inside selftests is to
>   avoid multiple evaluation is the below version acceptable?
>
>   #define min(x, y) ({ \
>     typeof(x) _x = (x); \
>     typeof(y) _y = (y); \
>     _x < _y ? _x : _y; \
> })
> 
> #define max(x, y) ({ \
>     typeof(x) _x = (x); \
>     typeof(y) _y = (y); \
>     _x > _y ? _x : _y; \
> })

Those are a reasonable pair.

If you want a signed-ness check the:
	_Static_assert(is_signed_type(typeof(a)) == is_signed_type(typeof(b)), "min/max signednesss")
check should just drop into the above.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

[PATCH 2/2] selftests/net: replace ternary operator with min()/max()

[Mahmoud Maatuq]

Fix the following coccicheck warning:
tools/testing/selftests/net/udpgso_bench_tx.c:297:18-19: WARNING opportunity for min()
tools/testing/selftests/net/udpgso_bench_tx.c:354:27-28: WARNING opportunity for min()
tools/testing/selftests/net/so_txtime.c:129:24-26: WARNING opportunity for max()
tools/testing/selftests/net/so_txtime.c:96:30-31: WARNING opportunity for max()

Signed-off-by: Mahmoud Maatuq <mahmoudmatook.mm@gmail.com>
---
 tools/testing/selftests/net/Makefile          | 2 ++
 tools/testing/selftests/net/so_txtime.c       | 7 ++++---
 tools/testing/selftests/net/udpgso_bench_tx.c | 6 +++---
 3 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/tools/testing/selftests/net/Makefile b/tools/testing/selftests/net/Makefile
index 7f3ab2a93ed6..a06cc25489f9 100644
--- a/tools/testing/selftests/net/Makefile
+++ b/tools/testing/selftests/net/Makefile
@@ -3,6 +3,8 @@
 
 CFLAGS =  -Wall -Wl,--no-as-needed -O2 -g
 CFLAGS += -I../../../../usr/include/ $(KHDR_INCLUDES)
+# Additional include paths needed by kselftest.h
+CFLAGS += -I../
 
 TEST_PROGS := run_netsocktests run_afpackettests test_bpf.sh netdevice.sh \
 	      rtnetlink.sh xfrm_policy.sh test_blackhole_dev.sh
diff --git a/tools/testing/selftests/net/so_txtime.c b/tools/testing/selftests/net/so_txtime.c
index 2672ac0b6d1f..2936174e7de4 100644
--- a/tools/testing/selftests/net/so_txtime.c
+++ b/tools/testing/selftests/net/so_txtime.c
@@ -33,6 +33,8 @@
 #include <unistd.h>
 #include <poll.h>
 
+#include "kselftest.h"
+
 static int	cfg_clockid	= CLOCK_TAI;
 static uint16_t	cfg_port	= 8000;
 static int	cfg_variance_us	= 4000;
@@ -93,8 +95,7 @@ static void do_send_one(int fdt, struct timed_send *ts)
 		msg.msg_controllen = sizeof(control);
 
 		tdeliver = glob_tstart + ts->delay_us * 1000;
-		tdeliver_max = tdeliver_max > tdeliver ?
-			       tdeliver_max : tdeliver;
+		tdeliver_max = max(tdeliver_max, tdeliver);
 
 		cm = CMSG_FIRSTHDR(&msg);
 		cm->cmsg_level = SOL_SOCKET;
@@ -126,7 +127,7 @@ static void do_recv_one(int fdr, struct timed_send *ts)
 		error(1, 0, "read: %dB", ret);
 
 	tstop = (gettime_ns(cfg_clockid) - glob_tstart) / 1000;
-	texpect = ts->delay_us >= 0 ? ts->delay_us : 0;
+	texpect = max(ts->delay_us, 0);
 
 	fprintf(stderr, "payload:%c delay:%lld expected:%lld (us)\n",
 			rbuf[0], (long long)tstop, (long long)texpect);
diff --git a/tools/testing/selftests/net/udpgso_bench_tx.c b/tools/testing/selftests/net/udpgso_bench_tx.c
index 477392715a9a..e92a7a753959 100644
--- a/tools/testing/selftests/net/udpgso_bench_tx.c
+++ b/tools/testing/selftests/net/udpgso_bench_tx.c
@@ -25,7 +25,7 @@
 #include <sys/types.h>
 #include <unistd.h>
 
-#include "../kselftest.h"
+#include "kselftest.h"
 
 #ifndef ETH_MAX_MTU
 #define ETH_MAX_MTU 0xFFFFU
@@ -294,7 +294,7 @@ static int send_udp(int fd, char *data)
 	total_len = cfg_payload_len;
 
 	while (total_len) {
-		len = total_len < cfg_mss ? total_len : cfg_mss;
+		len = min(total_len, cfg_mss);
 
 		ret = sendto(fd, data, len, cfg_zerocopy ? MSG_ZEROCOPY : 0,
 			     cfg_connected ? NULL : (void *)&cfg_dst_addr,
@@ -351,7 +351,7 @@ static int send_udp_sendmmsg(int fd, char *data)
 			error(1, 0, "sendmmsg: exceeds max_nr_msg");
 
 		iov[i].iov_base = data + off;
-		iov[i].iov_len = cfg_mss < left ? cfg_mss : left;
+		iov[i].iov_len = min(cfg_mss, left);
 
 		mmsgs[i].msg_hdr.msg_iov = iov + i;
 		mmsgs[i].msg_hdr.msg_iovlen = 1;
-- 
2.34.1

Re: [PATCH 2/2] selftests/net: replace ternary operator with min()/max()

[Willem de Bruijn]

Mahmoud Maatuq wrote:
> Fix the following coccicheck warning:
> tools/testing/selftests/net/udpgso_bench_tx.c:297:18-19: WARNING opportunity for min()
> tools/testing/selftests/net/udpgso_bench_tx.c:354:27-28: WARNING opportunity for min()
> tools/testing/selftests/net/so_txtime.c:129:24-26: WARNING opportunity for max()
> tools/testing/selftests/net/so_txtime.c:96:30-31: WARNING opportunity for max()
> 
> Signed-off-by: Mahmoud Maatuq <mahmoudmatook.mm@gmail.com>
> ---
>  tools/testing/selftests/net/Makefile          | 2 ++
>  tools/testing/selftests/net/so_txtime.c       | 7 ++++---
>  tools/testing/selftests/net/udpgso_bench_tx.c | 6 +++---
>  3 files changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/tools/testing/selftests/net/Makefile b/tools/testing/selftests/net/Makefile
> index 7f3ab2a93ed6..a06cc25489f9 100644
> --- a/tools/testing/selftests/net/Makefile
> +++ b/tools/testing/selftests/net/Makefile
> @@ -3,6 +3,8 @@
>  
>  CFLAGS =  -Wall -Wl,--no-as-needed -O2 -g
>  CFLAGS += -I../../../../usr/include/ $(KHDR_INCLUDES)
> +# Additional include paths needed by kselftest.h
> +CFLAGS += -I../

Why this, instead of the existing include with relative path?
  

Re: [PATCH 2/2] selftests/net: replace ternary operator with min()/max()

[Mahmoud Matook]

On 08/20, Willem de Bruijn wrote:

> Mahmoud Maatuq wrote:
> > Fix the following coccicheck warning:
> > tools/testing/selftests/net/udpgso_bench_tx.c:297:18-19: WARNING opportunity for min()
> > tools/testing/selftests/net/udpgso_bench_tx.c:354:27-28: WARNING opportunity for min()
> > tools/testing/selftests/net/so_txtime.c:129:24-26: WARNING opportunity for max()
> > tools/testing/selftests/net/so_txtime.c:96:30-31: WARNING opportunity for max()
> > 
> > Signed-off-by: Mahmoud Maatuq <mahmoudmatook.mm@gmail.com>
> > ---
> >  tools/testing/selftests/net/Makefile          | 2 ++
> >  tools/testing/selftests/net/so_txtime.c       | 7 ++++---
> >  tools/testing/selftests/net/udpgso_bench_tx.c | 6 +++---
> >  3 files changed, 9 insertions(+), 6 deletions(-)
> > 
> > diff --git a/tools/testing/selftests/net/Makefile b/tools/testing/selftests/net/Makefile
> > index 7f3ab2a93ed6..a06cc25489f9 100644
> > --- a/tools/testing/selftests/net/Makefile
> > +++ b/tools/testing/selftests/net/Makefile
> > @@ -3,6 +3,8 @@
> >  
> >  CFLAGS =  -Wall -Wl,--no-as-needed -O2 -g
> >  CFLAGS += -I../../../../usr/include/ $(KHDR_INCLUDES)
> > +# Additional include paths needed by kselftest.h
> > +CFLAGS += -I../
> 
> Why this, instead of the existing include with relative path?
>
no big reason other than that version was added in a previous patch that was accepted 
https://lore.kernel.org/all/168972782004.15840.17484255346823026.git-patchwork-notify@kernel.org/#r