diff mbox series

[bpf-next,v7,2/2] selftests/bpf: Selftest for real time helper

Message ID 20201001020504.18151-2-bimmy.pujari@intel.com (mailing list archive)
State Rejected
Headers show
Series [bpf-next,v7,1/2] bpf: Add bpf_ktime_get_real_ns | expand

Commit Message

Pujari, Bimmy Oct. 1, 2020, 2:05 a.m. UTC
From: Bimmy Pujari <bimmy.pujari@intel.com>

Add test validating that bpf_ktime_get_real_ns works fine.

Signed-off-by: Bimmy Pujari <bimmy.pujari@intel.com>
---
 .../selftests/bpf/prog_tests/ktime_real.c     | 42 +++++++++++++++++++
 .../bpf/progs/test_ktime_get_real_ns.c        | 36 ++++++++++++++++
 2 files changed, 78 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/ktime_real.c
 create mode 100644 tools/testing/selftests/bpf/progs/test_ktime_get_real_ns.c

Comments

Alexei Starovoitov Oct. 1, 2020, 5:35 a.m. UTC | #1
On Wed, Sep 30, 2020 at 07:05:04PM -0700, bimmy.pujari@intel.com wrote:
> +SEC("realtime_helper")
> +int realtime_helper_test(struct __sk_buff *skb)
> +{
> +	unsigned long long *lasttime;
> +	unsigned long long curtime;
> +	int key = 0;
> +	int err = 0;
> +
> +	lasttime = bpf_map_lookup_elem(&time_map, &key);
> +	if (!lasttime)
> +		goto err;
> +
> +	curtime = bpf_ktime_get_real_ns();
> +	if (curtime <= *lasttime) {
> +		err = 1;
> +		goto err;
> +	}
> +	*lasttime = curtime;

so the test is doing exactly what comment in patch 1 is saying not to do.
I'm sorry but Andrii's comments are correct. If the authors of the patch
cannot make it right we should not add this helper to general audience.
Just because POSIX allows it it doesn't mean that it did the good choice.
Pujari, Bimmy Oct. 1, 2020, 9:52 p.m. UTC | #2
Thanks everyone for putting your valuable time to review these patches. Can any one from you suggest the best way to test this function in selftest?

Regards
Bimmy

-----Original Message-----
From: Alexei Starovoitov <alexei.starovoitov@gmail.com> 
Sent: Wednesday, September 30, 2020 10:35 PM
To: Pujari, Bimmy <bimmy.pujari@intel.com>
Cc: bpf@vger.kernel.org; netdev@vger.kernel.org; mchehab@kernel.org; ast@kernel.org; daniel@iogearbox.net; kafai@fb.com; maze@google.com; Nikravesh, Ashkan <ashkan.nikravesh@intel.com>; Alvarez, Daniel A <daniel.a.alvarez@intel.com>
Subject: Re: [PATCH bpf-next v7 2/2] selftests/bpf: Selftest for real time helper

On Wed, Sep 30, 2020 at 07:05:04PM -0700, bimmy.pujari@intel.com wrote:
> +SEC("realtime_helper")
> +int realtime_helper_test(struct __sk_buff *skb) {
> +	unsigned long long *lasttime;
> +	unsigned long long curtime;
> +	int key = 0;
> +	int err = 0;
> +
> +	lasttime = bpf_map_lookup_elem(&time_map, &key);
> +	if (!lasttime)
> +		goto err;
> +
> +	curtime = bpf_ktime_get_real_ns();
> +	if (curtime <= *lasttime) {
> +		err = 1;
> +		goto err;
> +	}
> +	*lasttime = curtime;

so the test is doing exactly what comment in patch 1 is saying not to do.
I'm sorry but Andrii's comments are correct. If the authors of the patch cannot make it right we should not add this helper to general audience.
Just because POSIX allows it it doesn't mean that it did the good choice.
Alexei Starovoitov Oct. 1, 2020, 10:50 p.m. UTC | #3
On Thu, Oct 1, 2020 at 2:52 PM Pujari, Bimmy <bimmy.pujari@intel.com> wrote:
>
> Thanks everyone for putting your valuable time to review these patches. Can any one from you suggest the best way to test this function in selftest?

Don't bother. This helper is no go.

Please trim your replies next time and do not top post.
Maciej Żenczykowski Oct. 5, 2020, 5:36 p.m. UTC | #4
> Don't bother. This helper is no go.

I disagree on the 'no go' -- I do think we should have this helper.

The problem is it should only be used in some limited circumstances -
for example when processing packets coming in off the wire with real
times in them...  For example for something like measuring delay of
ntp frames.  This is of course testable but annoying (create a fake
ntp frame with gettimeofday injected timestamp in it, run it through
bpf, see what processing delay it reports).

Lets not make bpf even harder to use then it already is...
David Ahern Oct. 7, 2020, 4:33 p.m. UTC | #5
On 10/5/20 10:36 AM, Maciej Żenczykowski wrote:
>> Don't bother. This helper is no go.
> 
> I disagree on the 'no go' -- I do think we should have this helper.

+1

> 
> Lets not make bpf even harder to use then it already is...
> 

Logging is done using time of day; that is not a posix mistake but a
real need. Users do not file complaints based on a server's boot time or
some random monotonic time; they report a problem based on time-of-day.
Allowing bpf programs to timeofday makes it easier to troubleshoot and
correlate kernel side events to userspace logs.
diff mbox series

Patch

diff --git a/tools/testing/selftests/bpf/prog_tests/ktime_real.c b/tools/testing/selftests/bpf/prog_tests/ktime_real.c
new file mode 100644
index 000000000000..85235f2786b2
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/ktime_real.c
@@ -0,0 +1,42 @@ 
+// SPDX-License-Identifier: GPL-2.0
+#include <test_progs.h>
+#include <network_helpers.h>
+
+static void *time_thread(void *arg)
+{
+	__u32 duration, retval;
+	int err, prog_fd = *(u32 *) arg;
+
+	err = bpf_prog_test_run(prog_fd, 10000, &pkt_v4, sizeof(pkt_v4),
+				NULL, NULL, &retval, &duration);
+	CHECK(err || retval, "",
+	      "err %d errno %d retval %d duration %d\n",
+	      err, errno, retval, duration);
+	pthread_exit(arg);
+}
+
+void test_ktime_real(void)
+{
+	const char *file = "./test_ktime_get_real_ns.o";
+	struct bpf_object *obj = NULL;
+	pthread_t thread_id;
+	int prog_fd;
+	int err = 0;
+	void *ret;
+
+	err = bpf_prog_load(file, BPF_PROG_TYPE_CGROUP_SKB, &obj, &prog_fd);
+	if (CHECK_FAIL(err)) {
+		printf("test_ktime_get_real_ns:bpf_prog_load errno %d\n", errno);
+		goto close_prog;
+	}
+
+	if (CHECK_FAIL(pthread_create(&thread_id, NULL,
+				      &time_thread, &prog_fd)))
+		goto close_prog;
+
+	if (CHECK_FAIL(pthread_join(thread_id, &ret) ||
+				    ret != (void *)&prog_fd))
+		goto close_prog;
+close_prog:
+	bpf_object__close(obj);
+}
diff --git a/tools/testing/selftests/bpf/progs/test_ktime_get_real_ns.c b/tools/testing/selftests/bpf/progs/test_ktime_get_real_ns.c
new file mode 100644
index 000000000000..37132c59de61
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/test_ktime_get_real_ns.c
@@ -0,0 +1,36 @@ 
+// SPDX-License-Identifier: GPL-2.0
+#include <linux/bpf.h>
+#include <linux/version.h>
+#include <bpf/bpf_helpers.h>
+
+struct {
+	__uint(type, BPF_MAP_TYPE_ARRAY);
+	__uint(max_entries, 1);
+	__type(key, int);
+	__type(value, unsigned long long);
+} time_map SEC(".maps");
+
+SEC("realtime_helper")
+int realtime_helper_test(struct __sk_buff *skb)
+{
+	unsigned long long *lasttime;
+	unsigned long long curtime;
+	int key = 0;
+	int err = 0;
+
+	lasttime = bpf_map_lookup_elem(&time_map, &key);
+	if (!lasttime)
+		goto err;
+
+	curtime = bpf_ktime_get_real_ns();
+	if (curtime <= *lasttime) {
+		err = 1;
+		goto err;
+	}
+	*lasttime = curtime;
+
+err:
+	return err;
+}
+
+char _license[] SEC("license") = "GPL";