* [PATCH v2 1/2] selftests: Provide local define of min() and max() @ 2023-08-24 20:24 Mahmoud Maatuq 2023-08-24 20:24 ` [PATCH v2 2/2] selftests/net: replace ternary operator with min()/max() Mahmoud Maatuq 2023-08-25 15:26 ` [PATCH v2 1/2] selftests: Provide local define of min() and max() Sean Christopherson 0 siblings, 2 replies; 9+ messages in thread From: Mahmoud Maatuq @ 2023-08-24 20:24 UTC (permalink / raw) To: keescook, edumazet, willemdebruijn.kernel, wad, luto, netdev, linux-kernel, kuba, shuah, pabeni, linux-kselftest, davem Cc: linux-kernel-mentees, Mahmoud Maatuq, David Laight 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v2 2/2] selftests/net: replace ternary operator with min()/max() 2023-08-24 20:24 [PATCH v2 1/2] selftests: Provide local define of min() and max() Mahmoud Maatuq @ 2023-08-24 20:24 ` Mahmoud Maatuq 2023-08-24 20:32 ` Willem de Bruijn 2023-08-25 9:32 ` David Laight 2023-08-25 15:26 ` [PATCH v2 1/2] selftests: Provide local define of min() and max() Sean Christopherson 1 sibling, 2 replies; 9+ messages in thread From: Mahmoud Maatuq @ 2023-08-24 20:24 UTC (permalink / raw) To: keescook, edumazet, willemdebruijn.kernel, wad, luto, netdev, linux-kernel, kuba, shuah, pabeni, linux-kselftest, davem Cc: linux-kernel-mentees, 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v2 2/2] selftests/net: replace ternary operator with min()/max() 2023-08-24 20:24 ` [PATCH v2 2/2] selftests/net: replace ternary operator with min()/max() Mahmoud Maatuq @ 2023-08-24 20:32 ` Willem de Bruijn 2023-08-24 21:13 ` [PATCH v2 2/2] selftests/net: replace ternary operator with min()/max() ^[^[ Mahmoud Matook 2023-08-25 9:32 ` David Laight 1 sibling, 1 reply; 9+ messages in thread From: Willem de Bruijn @ 2023-08-24 20:32 UTC (permalink / raw) To: Mahmoud Maatuq Cc: keescook, edumazet, wad, luto, netdev, linux-kernel, kuba, shuah, pabeni, linux-kselftest, davem, linux-kernel-mentees 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 > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 2/2] selftests/net: replace ternary operator with min()/max() ^[^[ 2023-08-24 20:32 ` Willem de Bruijn @ 2023-08-24 21:13 ` Mahmoud Matook 2023-08-24 21:22 ` [PATCH v2 2/2] selftests/net: replace ternary operator with min()/max() Willem de Bruijn 0 siblings, 1 reply; 9+ messages in thread From: Mahmoud Matook @ 2023-08-24 21:13 UTC (permalink / raw) To: Willem de Bruijn Cc: keescook, edumazet, wad, luto, netdev, linux-kernel, kuba, shuah, pabeni, linux-kselftest, davem, linux-kernel-mentees 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 > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 2/2] selftests/net: replace ternary operator with min()/max() 2023-08-24 21:13 ` [PATCH v2 2/2] selftests/net: replace ternary operator with min()/max() ^[^[ Mahmoud Matook @ 2023-08-24 21:22 ` Willem de Bruijn 0 siblings, 0 replies; 9+ messages in thread From: Willem de Bruijn @ 2023-08-24 21:22 UTC (permalink / raw) To: Willem de Bruijn, keescook, edumazet, wad, luto, netdev, linux-kernel, kuba, shuah, pabeni, linux-kselftest, davem, linux-kernel-mentees 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 > > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [PATCH v2 2/2] selftests/net: replace ternary operator with min()/max() 2023-08-24 20:24 ` [PATCH v2 2/2] selftests/net: replace ternary operator with min()/max() Mahmoud Maatuq 2023-08-24 20:32 ` Willem de Bruijn @ 2023-08-25 9:32 ` David Laight 1 sibling, 0 replies; 9+ messages in thread From: David Laight @ 2023-08-25 9:32 UTC (permalink / raw) To: 'Mahmoud Maatuq', keescook@chromium.org, edumazet@google.com, willemdebruijn.kernel@gmail.com, wad@chromium.org, luto@amacapital.net, netdev@vger.kernel.org, linux-kernel@vger.kernel.org, kuba@kernel.org, shuah@kernel.org, pabeni@redhat.com, linux-kselftest@vger.kernel.org, davem@davemloft.net Cc: linux-kernel-mentees@lists.linuxfoundation.org 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) ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 1/2] selftests: Provide local define of min() and max() 2023-08-24 20:24 [PATCH v2 1/2] selftests: Provide local define of min() and max() Mahmoud Maatuq 2023-08-24 20:24 ` [PATCH v2 2/2] selftests/net: replace ternary operator with min()/max() Mahmoud Maatuq @ 2023-08-25 15:26 ` Sean Christopherson 2023-08-26 9:45 ` Mahmoud Matook 1 sibling, 1 reply; 9+ messages in thread From: Sean Christopherson @ 2023-08-25 15:26 UTC (permalink / raw) To: Mahmoud Maatuq Cc: keescook, edumazet, willemdebruijn.kernel, wad, luto, netdev, linux-kernel, kuba, shuah, pabeni, linux-kselftest, davem, linux-kernel-mentees, David Laight 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. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 1/2] selftests: Provide local define of min() and max() 2023-08-25 15:26 ` [PATCH v2 1/2] selftests: Provide local define of min() and max() Sean Christopherson @ 2023-08-26 9:45 ` Mahmoud Matook 2023-08-28 15:04 ` Sean Christopherson 0 siblings, 1 reply; 9+ messages in thread From: Mahmoud Matook @ 2023-08-26 9:45 UTC (permalink / raw) To: Sean Christopherson Cc: keescook, edumazet, willemdebruijn.kernel, wad, luto, netdev, linux-kernel, kuba, shuah, pabeni, linux-kselftest, davem, linux-kernel-mentees, David Laight 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. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 1/2] selftests: Provide local define of min() and max() 2023-08-26 9:45 ` Mahmoud Matook @ 2023-08-28 15:04 ` Sean Christopherson 0 siblings, 0 replies; 9+ messages in thread From: Sean Christopherson @ 2023-08-28 15:04 UTC (permalink / raw) To: keescook, edumazet, willemdebruijn.kernel, wad, luto, netdev, linux-kernel, kuba, shuah, pabeni, linux-kselftest, davem, linux-kernel-mentees, David Laight 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. ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2023-08-28 15:05 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-08-24 20:24 [PATCH v2 1/2] selftests: Provide local define of min() and max() Mahmoud Maatuq 2023-08-24 20:24 ` [PATCH v2 2/2] selftests/net: replace ternary operator with min()/max() Mahmoud Maatuq 2023-08-24 20:32 ` Willem de Bruijn 2023-08-24 21:13 ` [PATCH v2 2/2] selftests/net: replace ternary operator with min()/max() ^[^[ Mahmoud Matook 2023-08-24 21:22 ` [PATCH v2 2/2] selftests/net: replace ternary operator with min()/max() Willem de Bruijn 2023-08-25 9:32 ` David Laight 2023-08-25 15:26 ` [PATCH v2 1/2] selftests: Provide local define of min() and max() Sean Christopherson 2023-08-26 9:45 ` Mahmoud Matook 2023-08-28 15:04 ` Sean Christopherson
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox