| Message ID | 20201001020504.18151-2-bimmy.pujari@intel.com |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | [bpf-next,v7,1/2] bpf: Add bpf_ktime_get_real_ns | expand |
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.
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.
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.
> 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...
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 --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";