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.nakry...@gmail.com> Sent: Thursday, September 24, 2020 1:22 PM To: Pujari, Bimmy <bimmy.puj...@intel.com> Cc: bpf <b...@vger.kernel.org>; Networking <netdev@vger.kernel.org>; mche...@kernel.org; Alexei Starovoitov <a...@kernel.org>; Daniel Borkmann <dan...@iogearbox.net>; Martin Lau <ka...@fb.com>; Maciej Żenczykowski <m...@google.com>; Nikravesh, Ashkan <ashkan.nikrav...@intel.com> 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.puj...@intel.com> wrote: > > From: Bimmy Pujari <bimmy.puj...@intel.com> > > 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.puj...@intel.com> > --- 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 > [...]
