* [PATCH bpf-next v3 2/2] selftests/bpf: Verifying real time helper function
2020-09-24 2:25 [PATCH bpf-next v3 1/2] bpf: Add bpf_ktime_get_real_ns bimmy.pujari
@ 2020-09-24 2:25 ` bimmy.pujari
2020-09-24 15:34 ` Daniel Borkmann
2020-09-24 20:22 ` Andrii Nakryiko
2020-09-24 2:41 ` [PATCH bpf-next v3 1/2] bpf: Add bpf_ktime_get_real_ns Maciej Żenczykowski
2020-09-24 15:31 ` Daniel Borkmann
2 siblings, 2 replies; 7+ messages in thread
From: bimmy.pujari @ 2020-09-24 2:25 UTC (permalink / raw)
To: bpf
Cc: netdev, mchehab, ast, daniel, kafai, maze, bimmy.pujari,
ashkan.nikravesh
From: Bimmy Pujari <bimmy.pujari@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.pujari@intel.com>
---
.../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
diff --git a/tools/testing/selftests/bpf/progs/xdping_kern.c b/tools/testing/selftests/bpf/progs/xdping_kern.c
index 6b9ca40bd1f4..0a152f6835fe 100644
--- a/tools/testing/selftests/bpf/progs/xdping_kern.c
+++ b/tools/testing/selftests/bpf/progs/xdping_kern.c
@@ -1,184 +1,5 @@
// SPDX-License-Identifier: GPL-2.0
-/* Copyright (c) 2019, Oracle and/or its affiliates. All rights reserved. */
+#undef REALTIME
-#define KBUILD_MODNAME "foo"
-#include <stddef.h>
-#include <string.h>
-#include <linux/bpf.h>
-#include <linux/icmp.h>
-#include <linux/in.h>
-#include <linux/if_ether.h>
-#include <linux/if_packet.h>
-#include <linux/if_vlan.h>
-#include <linux/ip.h>
+#include "xdping_kern.h"
-#include <bpf/bpf_helpers.h>
-#include <bpf/bpf_endian.h>
-
-#include "xdping.h"
-
-struct {
- __uint(type, BPF_MAP_TYPE_HASH);
- __uint(max_entries, 256);
- __type(key, __u32);
- __type(value, struct pinginfo);
-} ping_map SEC(".maps");
-
-static __always_inline void swap_src_dst_mac(void *data)
-{
- unsigned short *p = data;
- unsigned short dst[3];
-
- dst[0] = p[0];
- dst[1] = p[1];
- dst[2] = p[2];
- p[0] = p[3];
- p[1] = p[4];
- p[2] = p[5];
- p[3] = dst[0];
- p[4] = dst[1];
- p[5] = dst[2];
-}
-
-static __always_inline __u16 csum_fold_helper(__wsum sum)
-{
- sum = (sum & 0xffff) + (sum >> 16);
- return ~((sum & 0xffff) + (sum >> 16));
-}
-
-static __always_inline __u16 ipv4_csum(void *data_start, int data_size)
-{
- __wsum sum;
-
- sum = bpf_csum_diff(0, 0, data_start, data_size, 0);
- return csum_fold_helper(sum);
-}
-
-#define ICMP_ECHO_LEN 64
-
-static __always_inline int icmp_check(struct xdp_md *ctx, int type)
-{
- void *data_end = (void *)(long)ctx->data_end;
- void *data = (void *)(long)ctx->data;
- struct ethhdr *eth = data;
- struct icmphdr *icmph;
- struct iphdr *iph;
-
- if (data + sizeof(*eth) + sizeof(*iph) + ICMP_ECHO_LEN > data_end)
- return XDP_PASS;
-
- if (eth->h_proto != bpf_htons(ETH_P_IP))
- return XDP_PASS;
-
- iph = data + sizeof(*eth);
-
- if (iph->protocol != IPPROTO_ICMP)
- return XDP_PASS;
-
- if (bpf_ntohs(iph->tot_len) - sizeof(*iph) != ICMP_ECHO_LEN)
- return XDP_PASS;
-
- icmph = data + sizeof(*eth) + sizeof(*iph);
-
- if (icmph->type != type)
- return XDP_PASS;
-
- return XDP_TX;
-}
-
-SEC("xdpclient")
-int xdping_client(struct xdp_md *ctx)
-{
- void *data_end = (void *)(long)ctx->data_end;
- void *data = (void *)(long)ctx->data;
- struct pinginfo *pinginfo = NULL;
- struct ethhdr *eth = data;
- struct icmphdr *icmph;
- struct iphdr *iph;
- __u64 recvtime;
- __be32 raddr;
- __be16 seq;
- int ret;
- __u8 i;
-
- ret = icmp_check(ctx, ICMP_ECHOREPLY);
-
- if (ret != XDP_TX)
- return ret;
-
- iph = data + sizeof(*eth);
- icmph = data + sizeof(*eth) + sizeof(*iph);
- raddr = iph->saddr;
-
- /* Record time reply received. */
- recvtime = bpf_ktime_get_ns();
- pinginfo = bpf_map_lookup_elem(&ping_map, &raddr);
- if (!pinginfo || pinginfo->seq != icmph->un.echo.sequence)
- return XDP_PASS;
-
- if (pinginfo->start) {
-#pragma clang loop unroll(full)
- for (i = 0; i < XDPING_MAX_COUNT; i++) {
- if (pinginfo->times[i] == 0)
- break;
- }
- /* verifier is fussy here... */
- if (i < XDPING_MAX_COUNT) {
- pinginfo->times[i] = recvtime -
- pinginfo->start;
- pinginfo->start = 0;
- i++;
- }
- /* No more space for values? */
- if (i == pinginfo->count || i == XDPING_MAX_COUNT)
- return XDP_PASS;
- }
-
- /* Now convert reply back into echo request. */
- swap_src_dst_mac(data);
- iph->saddr = iph->daddr;
- iph->daddr = raddr;
- icmph->type = ICMP_ECHO;
- seq = bpf_htons(bpf_ntohs(icmph->un.echo.sequence) + 1);
- icmph->un.echo.sequence = seq;
- icmph->checksum = 0;
- icmph->checksum = ipv4_csum(icmph, ICMP_ECHO_LEN);
-
- pinginfo->seq = seq;
- pinginfo->start = bpf_ktime_get_ns();
-
- return XDP_TX;
-}
-
-SEC("xdpserver")
-int xdping_server(struct xdp_md *ctx)
-{
- void *data_end = (void *)(long)ctx->data_end;
- void *data = (void *)(long)ctx->data;
- struct ethhdr *eth = data;
- struct icmphdr *icmph;
- struct iphdr *iph;
- __be32 raddr;
- int ret;
-
- ret = icmp_check(ctx, ICMP_ECHO);
-
- if (ret != XDP_TX)
- return ret;
-
- iph = data + sizeof(*eth);
- icmph = data + sizeof(*eth) + sizeof(*iph);
- raddr = iph->saddr;
-
- /* Now convert request into echo reply. */
- swap_src_dst_mac(data);
- iph->saddr = iph->daddr;
- iph->daddr = raddr;
- icmph->type = ICMP_ECHOREPLY;
- icmph->checksum = 0;
- icmph->checksum = ipv4_csum(icmph, ICMP_ECHO_LEN);
-
- return XDP_TX;
-}
-
-char _license[] SEC("license") = "GPL";
diff --git a/tools/testing/selftests/bpf/progs/xdping_kern.h b/tools/testing/selftests/bpf/progs/xdping_kern.h
new file mode 100644
index 000000000000..ca4b62214808
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/xdping_kern.h
@@ -0,0 +1,193 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/* Copyright (c) 2019, Oracle and/or its affiliates. All rights reserved. */
+
+#define KBUILD_MODNAME "foo"
+#include <stddef.h>
+#include <string.h>
+#include <linux/bpf.h>
+#include <linux/icmp.h>
+#include <linux/in.h>
+#include <linux/if_ether.h>
+#include <linux/if_packet.h>
+#include <linux/if_vlan.h>
+#include <linux/ip.h>
+
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_endian.h>
+
+#include "xdping.h"
+
+struct {
+ __uint(type, BPF_MAP_TYPE_HASH);
+ __uint(max_entries, 256);
+ __type(key, __u32);
+ __type(value, struct pinginfo);
+} ping_map SEC(".maps");
+
+static __always_inline void swap_src_dst_mac(void *data)
+{
+ unsigned short *p = data;
+ unsigned short dst[3];
+
+ dst[0] = p[0];
+ dst[1] = p[1];
+ dst[2] = p[2];
+ p[0] = p[3];
+ p[1] = p[4];
+ p[2] = p[5];
+ p[3] = dst[0];
+ p[4] = dst[1];
+ p[5] = dst[2];
+}
+
+static __always_inline __u16 csum_fold_helper(__wsum sum)
+{
+ sum = (sum & 0xffff) + (sum >> 16);
+ return ~((sum & 0xffff) + (sum >> 16));
+}
+
+static __always_inline __u16 ipv4_csum(void *data_start, int data_size)
+{
+ __wsum sum;
+
+ sum = bpf_csum_diff(0, 0, data_start, data_size, 0);
+ return csum_fold_helper(sum);
+}
+
+#define ICMP_ECHO_LEN 64
+
+static __always_inline int icmp_check(struct xdp_md *ctx, int type)
+{
+ void *data_end = (void *)(long)ctx->data_end;
+ void *data = (void *)(long)ctx->data;
+ struct ethhdr *eth = data;
+ struct icmphdr *icmph;
+ struct iphdr *iph;
+
+ if (data + sizeof(*eth) + sizeof(*iph) + ICMP_ECHO_LEN > data_end)
+ return XDP_PASS;
+
+ if (eth->h_proto != bpf_htons(ETH_P_IP))
+ return XDP_PASS;
+
+ iph = data + sizeof(*eth);
+
+ if (iph->protocol != IPPROTO_ICMP)
+ return XDP_PASS;
+
+ if (bpf_ntohs(iph->tot_len) - sizeof(*iph) != ICMP_ECHO_LEN)
+ return XDP_PASS;
+
+ icmph = data + sizeof(*eth) + sizeof(*iph);
+
+ if (icmph->type != type)
+ return XDP_PASS;
+
+ return XDP_TX;
+}
+
+SEC("xdpclient")
+int xdping_client(struct xdp_md *ctx)
+{
+ void *data_end = (void *)(long)ctx->data_end;
+ void *data = (void *)(long)ctx->data;
+ struct pinginfo *pinginfo = NULL;
+ struct ethhdr *eth = data;
+ struct icmphdr *icmph;
+ struct iphdr *iph;
+ __u64 recvtime;
+ __be32 raddr;
+ __be16 seq;
+ int ret;
+ __u8 i;
+
+ ret = icmp_check(ctx, ICMP_ECHOREPLY);
+
+ if (ret != XDP_TX)
+ return ret;
+
+ iph = data + sizeof(*eth);
+ icmph = data + sizeof(*eth) + sizeof(*iph);
+ raddr = iph->saddr;
+
+ /* Record time reply received. */
+#ifdef REALTIME
+ recvtime = bpf_ktime_get_real_ns();
+#else
+ recvtime = bpf_ktime_get_ns();
+#endif
+ pinginfo = bpf_map_lookup_elem(&ping_map, &raddr);
+ if (!pinginfo || pinginfo->seq != icmph->un.echo.sequence)
+ return XDP_PASS;
+
+ if (pinginfo->start) {
+#pragma clang loop unroll(full)
+ for (i = 0; i < XDPING_MAX_COUNT; i++) {
+ if ((pinginfo->times[i] == 0) ||
+ (pinginfo->start >= recvtime))
+ break;
+ }
+ /* verifier is fussy here... */
+ if (i < XDPING_MAX_COUNT) {
+ pinginfo->times[i] = recvtime -
+ pinginfo->start;
+ pinginfo->start = 0;
+ i++;
+ }
+ /* No more space for values? */
+ if (i == pinginfo->count || i == XDPING_MAX_COUNT)
+ return XDP_PASS;
+ }
+
+ /* Now convert reply back into echo request. */
+ swap_src_dst_mac(data);
+ iph->saddr = iph->daddr;
+ iph->daddr = raddr;
+ icmph->type = ICMP_ECHO;
+ seq = bpf_htons(bpf_ntohs(icmph->un.echo.sequence) + 1);
+ icmph->un.echo.sequence = seq;
+ icmph->checksum = 0;
+ icmph->checksum = ipv4_csum(icmph, ICMP_ECHO_LEN);
+
+ pinginfo->seq = seq;
+#ifdef REALTIME
+ pinginfo->start = bpf_ktime_get_real_ns();
+#else
+ pinginfo->start = bpf_ktime_get_ns();
+#endif
+
+ return XDP_TX;
+}
+
+SEC("xdpserver")
+int xdping_server(struct xdp_md *ctx)
+{
+ void *data_end = (void *)(long)ctx->data_end;
+ void *data = (void *)(long)ctx->data;
+ struct ethhdr *eth = data;
+ struct icmphdr *icmph;
+ struct iphdr *iph;
+ __be32 raddr;
+ int ret;
+
+ ret = icmp_check(ctx, ICMP_ECHO);
+
+ if (ret != XDP_TX)
+ return ret;
+
+ iph = data + sizeof(*eth);
+ icmph = data + sizeof(*eth) + sizeof(*iph);
+ raddr = iph->saddr;
+
+ /* Now convert request into echo reply. */
+ swap_src_dst_mac(data);
+ iph->saddr = iph->daddr;
+ iph->daddr = raddr;
+ icmph->type = ICMP_ECHOREPLY;
+ icmph->checksum = 0;
+ icmph->checksum = ipv4_csum(icmph, ICMP_ECHO_LEN);
+
+ return XDP_TX;
+}
+
+char _license[] SEC("license") = "GPL";
diff --git a/tools/testing/selftests/bpf/progs/xdping_realtime_kern.c b/tools/testing/selftests/bpf/progs/xdping_realtime_kern.c
new file mode 100644
index 000000000000..85f9d9bfc5b7
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/xdping_realtime_kern.c
@@ -0,0 +1,4 @@
+// SPDX-License-Identifier: GPL-2.0
+#define REALTIME
+
+#include "xdping_kern.h"
diff --git a/tools/testing/selftests/bpf/test_xdping.sh b/tools/testing/selftests/bpf/test_xdping.sh
index c2f0ddb45531..3e357755b279 100755
--- a/tools/testing/selftests/bpf/test_xdping.sh
+++ b/tools/testing/selftests/bpf/test_xdping.sh
@@ -42,11 +42,13 @@ setup()
ip addr add ${LOCAL_IP}/24 dev veth1
ip netns exec $TARGET_NS ip link set veth0 up
ip link set veth1 up
+ ln -s xdping xdping_realtime
}
cleanup()
{
set +e
+ rm xdping_realtime
ip netns delete $TARGET_NS 2>/dev/null
ip link del veth1 2>/dev/null
if [[ $server_pid -ne 0 ]]; then
@@ -54,7 +56,7 @@ cleanup()
fi
}
-test()
+test1()
{
client_args="$1"
server_args="$2"
@@ -77,6 +79,28 @@ test()
echo "Test client args '$client_args'; server args '$server_args': PASS"
}
+test2()
+{
+ client_args="$1"
+ server_args="$2"
+
+ echo "Test client args '$client_args'; server args '$server_args'"
+
+ server_pid=0
+ if [[ -n "$server_args" ]]; then
+ ip netns exec $TARGET_NS ./xdping_realtime $server_args &
+ server_pid=$!
+ sleep 10
+ fi
+ ./xdping_realtime $client_args $TARGET_IP
+
+ if [[ $server_pid -ne 0 ]]; then
+ kill -TERM $server_pid
+ server_pid=0
+ fi
+
+ echo "Test client args '$client_args'; server args '$server_args': PASS"
+}
set -e
server_pid=0
@@ -85,14 +109,18 @@ trap cleanup EXIT
setup
-for server_args in "" "-I veth0 -s -S" ; do
- # client in skb mode
- client_args="-I veth1 -S"
- test "$client_args" "$server_args"
+for tests in test1 test2; do
+ echo "Running ${tests}:"
+ for server_args in "" "-I veth0 -s -S" ; do
+ # client in skb mode
+ client_args="-I veth1 -S"
+ ${tests} "$client_args" "$server_args"
+
+ # client with count of 10 RTT measurements.
+ client_args="-I veth1 -S -c 10"
+ ${tests} "$client_args" "$server_args"
+ done
- # client with count of 10 RTT measurements.
- client_args="-I veth1 -S -c 10"
- test "$client_args" "$server_args"
done
echo "OK. All tests passed"
--
2.17.1
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH bpf-next v3 2/2] selftests/bpf: Verifying real time helper function
2020-09-24 2:25 ` [PATCH bpf-next v3 2/2] selftests/bpf: Verifying real time helper function bimmy.pujari
@ 2020-09-24 15:34 ` Daniel Borkmann
2020-09-24 20:22 ` Andrii Nakryiko
1 sibling, 0 replies; 7+ messages in thread
From: Daniel Borkmann @ 2020-09-24 15:34 UTC (permalink / raw)
To: bimmy.pujari, bpf; +Cc: netdev, mchehab, ast, kafai, maze, ashkan.nikravesh
On 9/24/20 4:25 AM, bimmy.pujari@intel.com wrote:
> From: Bimmy Pujari <bimmy.pujari@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.pujari@intel.com>
[...]
> diff --git a/tools/testing/selftests/bpf/progs/xdping_realtime_kern.c b/tools/testing/selftests/bpf/progs/xdping_realtime_kern.c
> new file mode 100644
> index 000000000000..85f9d9bfc5b7
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/xdping_realtime_kern.c
> @@ -0,0 +1,4 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#define REALTIME
> +
Instead of moving everything into xdping_kern.h and then adding a REALTIME define just
for doing ...
+#ifdef REALTIME
+ recvtime = bpf_ktime_get_real_ns();
+#else
+ recvtime = bpf_ktime_get_ns();
+#endif
... why not simply just provide a small inline helper with the bpf_ktime_*() implementation
instead so we can avoid the ugly ifdef altogether..
> +#include "xdping_kern.h"
> diff --git a/tools/testing/selftests/bpf/test_xdping.sh b/tools/testing/selftests/bpf/test_xdping.sh
> index c2f0ddb45531..3e357755b279 100755
> --- a/tools/testing/selftests/bpf/test_xdping.sh
[...]
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH bpf-next v3 2/2] selftests/bpf: Verifying real time helper function
2020-09-24 2:25 ` [PATCH bpf-next v3 2/2] selftests/bpf: Verifying real time helper function bimmy.pujari
2020-09-24 15:34 ` Daniel Borkmann
@ 2020-09-24 20:22 ` Andrii Nakryiko
2020-09-25 0:22 ` Pujari, Bimmy
1 sibling, 1 reply; 7+ messages in thread
From: Andrii Nakryiko @ 2020-09-24 20:22 UTC (permalink / raw)
To: Pujari, Bimmy
Cc: bpf, Networking, mchehab, Alexei Starovoitov, Daniel Borkmann,
Martin Lau, Maciej Żenczykowski, Nikravesh, Ashkan
On Wed, Sep 23, 2020 at 7:26 PM <bimmy.pujari@intel.com> wrote:
>
> From: Bimmy Pujari <bimmy.pujari@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.pujari@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
>
[...]
^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: [PATCH bpf-next v3 2/2] selftests/bpf: Verifying real time helper function
2020-09-24 20:22 ` Andrii Nakryiko
@ 2020-09-25 0:22 ` Pujari, Bimmy
0 siblings, 0 replies; 7+ messages in thread
From: Pujari, Bimmy @ 2020-09-25 0:22 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: bpf, Networking, mchehab, Alexei Starovoitov, Daniel Borkmann,
Martin Lau, Maciej Żenczykowski, Nikravesh, Ashkan
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@gmail.com>
Sent: Thursday, September 24, 2020 1:22 PM
To: Pujari, Bimmy <bimmy.pujari@intel.com>
Cc: bpf <bpf@vger.kernel.org>; Networking <netdev@vger.kernel.org>; mchehab@kernel.org; Alexei Starovoitov <ast@kernel.org>; Daniel Borkmann <daniel@iogearbox.net>; Martin Lau <kafai@fb.com>; Maciej Żenczykowski <maze@google.com>; Nikravesh, Ashkan <ashkan.nikravesh@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.pujari@intel.com> wrote:
>
> From: Bimmy Pujari <bimmy.pujari@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.pujari@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
>
[...]
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH bpf-next v3 1/2] bpf: Add bpf_ktime_get_real_ns
2020-09-24 2:25 [PATCH bpf-next v3 1/2] bpf: Add bpf_ktime_get_real_ns bimmy.pujari
2020-09-24 2:25 ` [PATCH bpf-next v3 2/2] selftests/bpf: Verifying real time helper function bimmy.pujari
@ 2020-09-24 2:41 ` Maciej Żenczykowski
2020-09-24 15:31 ` Daniel Borkmann
2 siblings, 0 replies; 7+ messages in thread
From: Maciej Żenczykowski @ 2020-09-24 2:41 UTC (permalink / raw)
To: bimmy.pujari
Cc: bpf, Linux NetDev, mchehab, Alexei Starovoitov, Daniel Borkmann,
Martin Lau, ashkan.nikravesh
On Wed, Sep 23, 2020 at 7:26 PM <bimmy.pujari@intel.com> wrote:
>
> From: Bimmy Pujari <bimmy.pujari@intel.com>
>
> The existing bpf helper functions to get timestamp return the time
> elapsed since system boot. This timestamp is not particularly useful
> where epoch timestamp is required or more than one server is involved
> and time sync is required. Instead, you want to use CLOCK_REALTIME,
> which provides epoch timestamp.
> Hence add bfp_ktime_get_real_ns() based around CLOCK_REALTIME.
>
> Signed-off-by: Ashkan Nikravesh <ashkan.nikravesh@intel.com>
> Signed-off-by: Bimmy Pujari <bimmy.pujari@intel.com>
> ---
> drivers/media/rc/bpf-lirc.c | 2 ++
> include/linux/bpf.h | 1 +
> include/uapi/linux/bpf.h | 8 ++++++++
> kernel/bpf/core.c | 1 +
> kernel/bpf/helpers.c | 13 +++++++++++++
> kernel/trace/bpf_trace.c | 2 ++
> tools/include/uapi/linux/bpf.h | 8 ++++++++
> 7 files changed, 35 insertions(+)
>
> diff --git a/drivers/media/rc/bpf-lirc.c b/drivers/media/rc/bpf-lirc.c
> index 5bb144435c16..649015fef3c1 100644
> --- a/drivers/media/rc/bpf-lirc.c
> +++ b/drivers/media/rc/bpf-lirc.c
> @@ -105,6 +105,8 @@ lirc_mode2_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
> return &bpf_ktime_get_ns_proto;
> case BPF_FUNC_ktime_get_boot_ns:
> return &bpf_ktime_get_boot_ns_proto;
> + case BPF_FUNC_ktime_get_real_ns:
> + return &bpf_ktime_get_real_ns_proto;
> case BPF_FUNC_tail_call:
> return &bpf_tail_call_proto;
> case BPF_FUNC_get_prandom_u32:
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index fc5c901c7542..18c4fdce65c8 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -1757,6 +1757,7 @@ extern const struct bpf_func_proto bpf_get_numa_node_id_proto;
> extern const struct bpf_func_proto bpf_tail_call_proto;
> extern const struct bpf_func_proto bpf_ktime_get_ns_proto;
> extern const struct bpf_func_proto bpf_ktime_get_boot_ns_proto;
> +extern const struct bpf_func_proto bpf_ktime_get_real_ns_proto;
> extern const struct bpf_func_proto bpf_get_current_pid_tgid_proto;
> extern const struct bpf_func_proto bpf_get_current_uid_gid_proto;
> extern const struct bpf_func_proto bpf_get_current_comm_proto;
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index a22812561064..198e69a6508d 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -3586,6 +3586,13 @@ union bpf_attr {
> * the data in *dst*. This is a wrapper of **copy_from_user**\ ().
> * Return
> * 0 on success, or a negative error in case of failure.
> + *
> + * u64 bpf_ktime_get_real_ns(void)
> + * Description
> + * Return the real time in nanoseconds.
> + * See: **clock_gettime**\ (**CLOCK_REALTIME**)
> + * Return
> + * Current *ktime*.
> */
> #define __BPF_FUNC_MAPPER(FN) \
> FN(unspec), \
> @@ -3737,6 +3744,7 @@ union bpf_attr {
> FN(inode_storage_delete), \
> FN(d_path), \
> FN(copy_from_user), \
> + FN(ktime_get_real_ns), \
> /* */
>
> /* integer value in 'imm' field of BPF_CALL instruction selects which helper
> diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
> index c4811b139caa..0dbbda9b743b 100644
> --- a/kernel/bpf/core.c
> +++ b/kernel/bpf/core.c
> @@ -2208,6 +2208,7 @@ const struct bpf_func_proto bpf_get_smp_processor_id_proto __weak;
> const struct bpf_func_proto bpf_get_numa_node_id_proto __weak;
> const struct bpf_func_proto bpf_ktime_get_ns_proto __weak;
> const struct bpf_func_proto bpf_ktime_get_boot_ns_proto __weak;
> +const struct bpf_func_proto bpf_ktime_get_real_ns_proto __weak;
>
> const struct bpf_func_proto bpf_get_current_pid_tgid_proto __weak;
> const struct bpf_func_proto bpf_get_current_uid_gid_proto __weak;
> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> index 5cc7425ee476..300db9269996 100644
> --- a/kernel/bpf/helpers.c
> +++ b/kernel/bpf/helpers.c
> @@ -166,6 +166,17 @@ const struct bpf_func_proto bpf_ktime_get_boot_ns_proto = {
> .gpl_only = false,
> .ret_type = RET_INTEGER,
> };
> +BPF_CALL_0(bpf_ktime_get_real_ns)
> +{
> + /* NMI safe access to clock realtime */
> + return ktime_get_real_fast_ns();
> +}
> +
> +const struct bpf_func_proto bpf_ktime_get_real_ns_proto = {
> + .func = bpf_ktime_get_real_ns,
> + .gpl_only = false,
> + .ret_type = RET_INTEGER,
> +};
>
> BPF_CALL_0(bpf_get_current_pid_tgid)
> {
> @@ -657,6 +668,8 @@ bpf_base_func_proto(enum bpf_func_id func_id)
> return &bpf_ktime_get_ns_proto;
> case BPF_FUNC_ktime_get_boot_ns:
> return &bpf_ktime_get_boot_ns_proto;
> + case BPF_FUNC_ktime_get_real_ns:
> + return &bpf_ktime_get_real_ns_proto;
> case BPF_FUNC_ringbuf_output:
> return &bpf_ringbuf_output_proto;
> case BPF_FUNC_ringbuf_reserve:
> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index 36508f46a8db..8ea2a0e50041 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -1167,6 +1167,8 @@ bpf_tracing_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
> return &bpf_ktime_get_ns_proto;
> case BPF_FUNC_ktime_get_boot_ns:
> return &bpf_ktime_get_boot_ns_proto;
> + case BPF_FUNC_ktime_get_real_ns:
> + return &bpf_ktime_get_real_ns_proto;
> case BPF_FUNC_tail_call:
> return &bpf_tail_call_proto;
> case BPF_FUNC_get_current_pid_tgid:
> diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
> index a22812561064..198e69a6508d 100644
> --- a/tools/include/uapi/linux/bpf.h
> +++ b/tools/include/uapi/linux/bpf.h
> @@ -3586,6 +3586,13 @@ union bpf_attr {
> * the data in *dst*. This is a wrapper of **copy_from_user**\ ().
> * Return
> * 0 on success, or a negative error in case of failure.
> + *
> + * u64 bpf_ktime_get_real_ns(void)
> + * Description
> + * Return the real time in nanoseconds.
> + * See: **clock_gettime**\ (**CLOCK_REALTIME**)
> + * Return
> + * Current *ktime*.
> */
> #define __BPF_FUNC_MAPPER(FN) \
> FN(unspec), \
> @@ -3737,6 +3744,7 @@ union bpf_attr {
> FN(inode_storage_delete), \
> FN(d_path), \
> FN(copy_from_user), \
> + FN(ktime_get_real_ns), \
> /* */
>
> /* integer value in 'imm' field of BPF_CALL instruction selects which helper
> --
> 2.17.1
Reviewed-by: Maciej Żenczykowski <maze@google.com>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH bpf-next v3 1/2] bpf: Add bpf_ktime_get_real_ns
2020-09-24 2:25 [PATCH bpf-next v3 1/2] bpf: Add bpf_ktime_get_real_ns bimmy.pujari
2020-09-24 2:25 ` [PATCH bpf-next v3 2/2] selftests/bpf: Verifying real time helper function bimmy.pujari
2020-09-24 2:41 ` [PATCH bpf-next v3 1/2] bpf: Add bpf_ktime_get_real_ns Maciej Żenczykowski
@ 2020-09-24 15:31 ` Daniel Borkmann
2 siblings, 0 replies; 7+ messages in thread
From: Daniel Borkmann @ 2020-09-24 15:31 UTC (permalink / raw)
To: bimmy.pujari, bpf
Cc: netdev, mchehab, ast, kafai, maze, ashkan.nikravesh, dsahern,
andrii.nakryiko
On 9/24/20 4:25 AM, bimmy.pujari@intel.com wrote:
> From: Bimmy Pujari <bimmy.pujari@intel.com>
>
> The existing bpf helper functions to get timestamp return the time
> elapsed since system boot. This timestamp is not particularly useful
> where epoch timestamp is required or more than one server is involved
> and time sync is required. Instead, you want to use CLOCK_REALTIME,
> which provides epoch timestamp.
> Hence add bfp_ktime_get_real_ns() based around CLOCK_REALTIME.
nit: ^ typo
>
> Signed-off-by: Ashkan Nikravesh <ashkan.nikravesh@intel.com>
> Signed-off-by: Bimmy Pujari <bimmy.pujari@intel.com>
> ---
> drivers/media/rc/bpf-lirc.c | 2 ++
> include/linux/bpf.h | 1 +
> include/uapi/linux/bpf.h | 8 ++++++++
> kernel/bpf/core.c | 1 +
> kernel/bpf/helpers.c | 13 +++++++++++++
> kernel/trace/bpf_trace.c | 2 ++
> tools/include/uapi/linux/bpf.h | 8 ++++++++
> 7 files changed, 35 insertions(+)
[...]
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index fc5c901c7542..18c4fdce65c8 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -1757,6 +1757,7 @@ extern const struct bpf_func_proto bpf_get_numa_node_id_proto;
> extern const struct bpf_func_proto bpf_tail_call_proto;
> extern const struct bpf_func_proto bpf_ktime_get_ns_proto;
> extern const struct bpf_func_proto bpf_ktime_get_boot_ns_proto;
> +extern const struct bpf_func_proto bpf_ktime_get_real_ns_proto;
> extern const struct bpf_func_proto bpf_get_current_pid_tgid_proto;
> extern const struct bpf_func_proto bpf_get_current_uid_gid_proto;
> extern const struct bpf_func_proto bpf_get_current_comm_proto;
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index a22812561064..198e69a6508d 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -3586,6 +3586,13 @@ union bpf_attr {
> * the data in *dst*. This is a wrapper of **copy_from_user**\ ().
> * Return
> * 0 on success, or a negative error in case of failure.
> + *
> + * u64 bpf_ktime_get_real_ns(void)
> + * Description
> + * Return the real time in nanoseconds.
> + * See: **clock_gettime**\ (**CLOCK_REALTIME**)
So from prior discussion with Andrii and David my impression was that the plan
was to at least properly document the clock and its limitations at minimum in
order to provide some basic guidance for users? Seems this was not yet addressed
here.
> + * Return
> + * Current *ktime*.
> */
> #define __BPF_FUNC_MAPPER(FN) \
> FN(unspec), \
> @@ -3737,6 +3744,7 @@ union bpf_attr {
> FN(inode_storage_delete), \
> FN(d_path), \
> FN(copy_from_user), \
> + FN(ktime_get_real_ns), \
> /* */
>
> /* integer value in 'imm' field of BPF_CALL instruction selects which helper
> diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
> index c4811b139caa..0dbbda9b743b 100644
> --- a/kernel/bpf/core.c
> +++ b/kernel/bpf/core.c
> @@ -2208,6 +2208,7 @@ const struct bpf_func_proto bpf_get_smp_processor_id_proto __weak;
> const struct bpf_func_proto bpf_get_numa_node_id_proto __weak;
> const struct bpf_func_proto bpf_ktime_get_ns_proto __weak;
> const struct bpf_func_proto bpf_ktime_get_boot_ns_proto __weak;
> +const struct bpf_func_proto bpf_ktime_get_real_ns_proto __weak;
>
> const struct bpf_func_proto bpf_get_current_pid_tgid_proto __weak;
> const struct bpf_func_proto bpf_get_current_uid_gid_proto __weak;
> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> index 5cc7425ee476..300db9269996 100644
> --- a/kernel/bpf/helpers.c
> +++ b/kernel/bpf/helpers.c
> @@ -166,6 +166,17 @@ const struct bpf_func_proto bpf_ktime_get_boot_ns_proto = {
> .gpl_only = false,
> .ret_type = RET_INTEGER,
> };
> +BPF_CALL_0(bpf_ktime_get_real_ns)
> +{
nit: newline before BPF_CALL_0(...)
> + /* NMI safe access to clock realtime */
> + return ktime_get_real_fast_ns();
> +}
> +
> +const struct bpf_func_proto bpf_ktime_get_real_ns_proto = {
> + .func = bpf_ktime_get_real_ns,
> + .gpl_only = false,
> + .ret_type = RET_INTEGER,
> +};
>
> BPF_CALL_0(bpf_get_current_pid_tgid)
> {
[...]
^ permalink raw reply [flat|nested] 7+ messages in thread