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

[PATCH v2 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>
Suggested-by: David Laight <David.Laight@aculab.com>
---
changes in v2:
redefine min/max in a more strict way to avoid 
signedness mismatch and multiple evaluation.
is_signed_type() moved from selftests/kselftest_harness.h 
to selftests/kselftest.h.
---
 tools/testing/selftests/kselftest.h         | 24 +++++++++++++++++++++
 tools/testing/selftests/kselftest_harness.h |  2 --
 2 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/kselftest.h b/tools/testing/selftests/kselftest.h
index 829be379545a..93d029471cc9 100644
--- a/tools/testing/selftests/kselftest.h
+++ b/tools/testing/selftests/kselftest.h
@@ -55,6 +55,30 @@
 #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]))
 #endif
 
+#ifndef is_signed_type
+#define is_signed_type(var)       (!!(((__typeof__(var))(-1)) < (__typeof__(var))1))
+#endif
+
+#ifndef min
+#define min(x, y) ({ \
+	_Static_assert(is_signed_type(typeof(x)) == is_signed_type(typeof(y)), \
+	       "min: signedness mismatch"); \
+	typeof(x) _x = (x); \
+	typeof(y) _y = (y); \
+	_x < _y ? _x : _y; \
+})
+#endif
+
+#ifndef max
+#define max(x, y) ({ \
+	_Static_assert(is_signed_type(typeof(x)) == is_signed_type(typeof(y)), \
+	       "max: signedness mismatch"); \
+	typeof(x) _x = (x); \
+	typeof(y) _y = (y); \
+	_x > _y ? _x : _y; \
+})
+#endif
+
 /*
  * gcc cpuid.h provides __cpuid_count() since v4.4.
  * Clang/LLVM cpuid.h provides  __cpuid_count() since v3.4.0.
diff --git a/tools/testing/selftests/kselftest_harness.h b/tools/testing/selftests/kselftest_harness.h
index 5fd49ad0c696..e4e310815226 100644
--- a/tools/testing/selftests/kselftest_harness.h
+++ b/tools/testing/selftests/kselftest_harness.h
@@ -699,8 +699,6 @@
 	if (_metadata->passed && _metadata->step < 253) \
 		_metadata->step++;
 
-#define is_signed_type(var)       (!!(((__typeof__(var))(-1)) < (__typeof__(var))1))
-
 #define __EXPECT(_expected, _expected_str, _seen, _seen_str, _t, _assert) do { \
 	/* Avoid multiple evaluation of the cases */ \
 	__typeof__(_expected) __exp = (_expected); \
-- 
2.34.1

[PATCH v2 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>
Suggested-by: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
---
changes in v2:
cast var cfg_mss to int to avoid static assertion
after providing a stricter version of min() that does signedness checking.
---
 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..6bd32a312901 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, (int)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 v2 2/2] selftests/net: replace ternary operator with min()/max()

[Willem de Bruijn]

On Thu, Aug 24, 2023 at 4:25 PM Mahmoud Maatuq
<mahmoudmatook.mm@gmail.com> 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>
> Suggested-by: Willem de Bruijn <willemdebruijn.kernel@gmail.com>

I did not suggest this change.

> ---
> changes in v2:
> cast var cfg_mss to int to avoid static assertion
> after providing a stricter version of min() that does signedness checking.
> ---
>  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 does kselftest.h need this? It does not include anything else?

I'd just add #include "../kselftests.h" to so_txtime.c and remove the
path change from udpgso_bench_tx.c

Fine with this approach. Just don't understand the comment

>
>  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..6bd32a312901 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, (int)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 v2 2/2] selftests/net: replace ternary operator with min()/max()^[^[

[Mahmoud Matook]

On 08/24, Willem de Bruijn wrote:

> On Thu, Aug 24, 2023 at 4:25 PM Mahmoud Maatuq
> <mahmoudmatook.mm@gmail.com> 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>
> > Suggested-by: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
> 
> I did not suggest this change.
> 
the suggestion i meant here is about the following part
"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."
as mentioned here https://lore.kernel.org/all/CAF=yD-+6cWTiDgpsu=hUV+OvzDFRaT2ZUmtQo9qTrCB9i-+7ng@mail.gmail.com/
I tried to change the type of cfg_mss but it creates a different warn
at udpgso_bench_tx.c:354 between cfg_mss and left that's way i casted
instead.
apology if i misunderstood your point.
> > ---
> > changes in v2:
> > cast var cfg_mss to int to avoid static assertion
> > after providing a stricter version of min() that does signedness checking.
> > ---
> >  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 does kselftest.h need this? It does not include anything else?
> 
> I'd just add #include "../kselftests.h" to so_txtime.c and remove the
> path change from udpgso_bench_tx.c
> 
> Fine with this approach. Just don't understand the comment
>

the comment here is wrong and it should be changed to include path
needed to include kselftest.h or to be deleted

> >
> >  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..6bd32a312901 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, (int)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 v2 2/2] selftests/net: replace ternary operator with min()/max()

[Willem de Bruijn]

On Thu, Aug 24, 2023 at 5:13 PM Mahmoud Matook
<mahmoudmatook.mm@gmail.com> wrote:
>
> On 08/24, Willem de Bruijn wrote:
>
> > On Thu, Aug 24, 2023 at 4:25 PM Mahmoud Maatuq
> > <mahmoudmatook.mm@gmail.com> 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>
> > > Suggested-by: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
> >
> > I did not suggest this change.
> >
> the suggestion i meant here is about the following part
> "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."
> as mentioned here https://lore.kernel.org/all/CAF=yD-+6cWTiDgpsu=hUV+OvzDFRaT2ZUmtQo9qTrCB9i-+7ng@mail.gmail.com/
> I tried to change the type of cfg_mss but it creates a different warn
> at udpgso_bench_tx.c:354 between cfg_mss and left that's way i casted
> instead.
> apology if i misunderstood your point.

Thanks. It's fine to avoid changing the type or cast if it does not
set of any alarms.

I think Suggested-by is for when the main idea of the patch is
someone's suggestion. Not a big deal, but please drop in v3. It's your
hard work and yours only. I'll add my Reviewed-by.



> > > ---
> > > changes in v2:
> > > cast var cfg_mss to int to avoid static assertion
> > > after providing a stricter version of min() that does signedness checking.
> > > ---
> > >  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 does kselftest.h need this? It does not include anything else?
> >
> > I'd just add #include "../kselftests.h" to so_txtime.c and remove the
> > path change from udpgso_bench_tx.c
> >
> > Fine with this approach. Just don't understand the comment
> >
>
> the comment here is wrong and it should be changed to include path
> needed to include kselftest.h or to be deleted
>
> > >
> > >  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..6bd32a312901 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, (int)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 v2 2/2] selftests/net: replace ternary operator with min()/max()

[David Laight]

From: Mahmoud Maatuq
> Sent: 24 August 2023 21:24
....
>  		tdeliver = glob_tstart + ts->delay_us * 1000;
> -		tdeliver_max = tdeliver_max > tdeliver ?
> -			       tdeliver_max : tdeliver;
> +		tdeliver_max = max(tdeliver_max, tdeliver);

That was spectacularly horrid...
What is wrong with using:
	if (tdeliver > tdeliver_max)
		tdeliver_max = tdeliver;
That is pretty obviously calculating the upper bound.
Even the version with max() needs extra parsing by the human reader.

(The only issue is whether it reads better with the if condition
reversed.)

	David

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

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

[Sean Christopherson]

On Fri, Aug 25, 2023, 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>
> Suggested-by: David Laight <David.Laight@aculab.com>
> ---
> changes in v2:
> redefine min/max in a more strict way to avoid 
> signedness mismatch and multiple evaluation.
> is_signed_type() moved from selftests/kselftest_harness.h 
> to selftests/kselftest.h.
> ---
>  tools/testing/selftests/kselftest.h         | 24 +++++++++++++++++++++

Heh, reminds me of https://xkcd.com/927.

All of these #defines are available in tools/include/linux/kernel.h, and it's
trivially easy for selftests to add all of tools/include to their include path.
I don't see any reason for the selftests framework to define yet another version,
just fix the individual tests.

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

[Mahmoud Matook]

On 08/25, Sean Christopherson wrote:

> On Fri, Aug 25, 2023, 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>
> > Suggested-by: David Laight <David.Laight@aculab.com>
> > ---
> > changes in v2:
> > redefine min/max in a more strict way to avoid 
> > signedness mismatch and multiple evaluation.
> > is_signed_type() moved from selftests/kselftest_harness.h 
> > to selftests/kselftest.h.
> > ---
> >  tools/testing/selftests/kselftest.h         | 24 +++++++++++++++++++++
> 
> Heh, reminds me of https://xkcd.com/927.
> 
> All of these #defines are available in tools/include/linux/kernel.h, and it's
> trivially easy for selftests to add all of tools/include to their include path.
> I don't see any reason for the selftests framework to define yet another version,
> just fix the individual tests.

giving the reviews seems that patchset is useless.
still a confusing point for me; after adding tools/include to the
include path of selftes how we can differentaite between  #include
<linux/kernel.h> that under tools/include and one under usr/include.

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

[Sean Christopherson]

On Sat, Aug 26, 2023, Mahmoud Matook wrote:
> On 08/25, Sean Christopherson wrote:
> 
> > On Fri, Aug 25, 2023, 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>
> > > Suggested-by: David Laight <David.Laight@aculab.com>
> > > ---
> > > changes in v2:
> > > redefine min/max in a more strict way to avoid 
> > > signedness mismatch and multiple evaluation.
> > > is_signed_type() moved from selftests/kselftest_harness.h 
> > > to selftests/kselftest.h.
> > > ---
> > >  tools/testing/selftests/kselftest.h         | 24 +++++++++++++++++++++
> > 
> > Heh, reminds me of https://xkcd.com/927.
> > 
> > All of these #defines are available in tools/include/linux/kernel.h, and it's
> > trivially easy for selftests to add all of tools/include to their include path.
> > I don't see any reason for the selftests framework to define yet another version,
> > just fix the individual tests.
> 
> giving the reviews seems that patchset is useless.
> still a confusing point for me; after adding tools/include to the
> include path of selftes how we can differentaite between  #include
> <linux/kernel.h> that under tools/include and one under usr/include.

AFAIK, it's up to the individual selftest (or it's "local" framework) to declare
the tools/include path before usr/include, e.g. see tools/testing/selftests/kvm/Makefile.

The whole setup is definitely a bit kludgy, but IMO it's better than conditionally
providing selftests specific fallbacks and potentially ending up with multiple
definitions of min/max within a single test.