I agree with you. I will write a separate new test. In the meanwhile I think it should be okay to prepare and send just one patch. Thanks Bimmy -----Original Message----- From: Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx> Sent: Thursday, September 24, 2020 1:22 PM To: Pujari, Bimmy <bimmy.pujari@xxxxxxxxx> Cc: bpf <bpf@xxxxxxxxxxxxxxx>; Networking <netdev@xxxxxxxxxxxxxxx>; mchehab@xxxxxxxxxx; Alexei Starovoitov <ast@xxxxxxxxxx>; Daniel Borkmann <daniel@xxxxxxxxxxxxx>; Martin Lau <kafai@xxxxxx>; Maciej Żenczykowski <maze@xxxxxxxxxx>; Nikravesh, Ashkan <ashkan.nikravesh@xxxxxxxxx> Subject: Re: [PATCH bpf-next v3 2/2] selftests/bpf: Verifying real time helper function On Wed, Sep 23, 2020 at 7:26 PM <bimmy.pujari@xxxxxxxxx> wrote: > > From: Bimmy Pujari <bimmy.pujari@xxxxxxxxx> > > Test xdping measures RTT from xdp using monotonic time helper. > Extending xdping test to use real time helper function in order to > verify this helper function. > > Signed-off-by: Bimmy Pujari <bimmy.pujari@xxxxxxxxx> > --- This is exactly the use of REALTIME clock that I was arguing against, and yet you are actually creating an example of how to use it for such case. CLOCK_REALTIME should not be used to measuring time elapsed (not within the same machine, at least), there are strictly better alternatives. So if you want to write a test for a new helper (assuming everyone else thinks it's a good idea), then do just that - write a separate minimal test that tests just your new functionality. Don't couple it with a massive XDP program. And also don't create unnecessarily almost 400 lines of code churn. > .../testing/selftests/bpf/progs/xdping_kern.c | 183 +---------------- > .../testing/selftests/bpf/progs/xdping_kern.h | 193 ++++++++++++++++++ > .../bpf/progs/xdping_realtime_kern.c | 4 + > tools/testing/selftests/bpf/test_xdping.sh | 44 +++- > 4 files changed, 235 insertions(+), 189 deletions(-) create mode > 100644 tools/testing/selftests/bpf/progs/xdping_kern.h > create mode 100644 > tools/testing/selftests/bpf/progs/xdping_realtime_kern.c > [...]
