diff mbox series

[bpf-next] selftests/bpf: Properly enable hwtstamp in xdp_hw_metadata

Message ID 20230125223205.3933482-1-sdf@google.com (mailing list archive)
State Superseded
Delegated to: BPF
Headers show
Series [bpf-next] selftests/bpf: Properly enable hwtstamp in xdp_hw_metadata | expand

Checks

Context Check Description
bpf/vmtest-bpf-next-PR success PR summary
netdev/tree_selection success Clearly marked for bpf-next, async
netdev/fixes_present success Fixes tag not required for -next series
netdev/subject_prefix success Link
netdev/cover_letter success Single patches do not need cover letters
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/cc_maintainers warning 7 maintainers not CCed: kuba@kernel.org mykolal@fb.com davem@davemloft.net linux-kselftest@vger.kernel.org shuah@kernel.org hawk@kernel.org netdev@vger.kernel.org
netdev/build_clang success Errors and warnings before: 0 this patch: 0
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success Fixes tag looks correct
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 52 lines checked
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
bpf/vmtest-bpf-next-VM_Test-11 success Logs for test_maps on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-1 success Logs for ShellCheck
bpf/vmtest-bpf-next-VM_Test-7 success Logs for llvm-toolchain
bpf/vmtest-bpf-next-VM_Test-8 success Logs for set-matrix
bpf/vmtest-bpf-next-VM_Test-2 success Logs for build for aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-3 success Logs for build for aarch64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-5 success Logs for build for x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-6 success Logs for build for x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-4 success Logs for build for s390x with gcc
bpf/vmtest-bpf-next-VM_Test-12 success Logs for test_maps on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-13 success Logs for test_maps on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-22 success Logs for test_progs_no_alu32 on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-23 success Logs for test_progs_no_alu32 on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-24 success Logs for test_progs_no_alu32_parallel on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-27 success Logs for test_progs_no_alu32_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-28 success Logs for test_progs_no_alu32_parallel on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-29 success Logs for test_progs_parallel on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-32 success Logs for test_progs_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-33 success Logs for test_progs_parallel on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-34 success Logs for test_verifier on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-37 success Logs for test_verifier on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-38 success Logs for test_verifier on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-9 success Logs for test_maps on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-10 success Logs for test_maps on aarch64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-14 success Logs for test_progs on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-15 success Logs for test_progs on aarch64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-17 success Logs for test_progs on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-18 success Logs for test_progs on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-19 success Logs for test_progs_no_alu32 on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-20 success Logs for test_progs_no_alu32 on aarch64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-25 success Logs for test_progs_no_alu32_parallel on aarch64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-30 success Logs for test_progs_parallel on aarch64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-35 success Logs for test_verifier on aarch64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-26 success Logs for test_progs_no_alu32_parallel on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-31 fail Logs for test_progs_parallel on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-16 success Logs for test_progs on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-36 success Logs for test_verifier on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-21 success Logs for test_progs_no_alu32 on s390x with gcc

Commit Message

Stanislav Fomichev Jan. 25, 2023, 10:32 p.m. UTC 
The existing timestamping_enable() is a no-op because it applies
to the socket-related path that we are not verifying here
anymore. (but still leaving the code around hoping we can
have xdp->skb path verified here as well)

  poll: 1 (0)
  xsk_ring_cons__peek: 1
  0xf64788: rx_desc[0]->addr=100000000008000 addr=8100 comp_addr=8000
  rx_hash: 3697961069
  rx_timestamp:  1674657672142214773 (sec:1674657672.1422)
  XDP RX-time:   1674657709561774876 (sec:1674657709.5618) delta sec:37.4196
  AF_XDP time:   1674657709561871034 (sec:1674657709.5619) delta
sec:0.0001 (96.158 usec)
  0xf64788: complete idx=8 addr=8000

Also, maybe something to archive here, see [0] for Jesper's note
about NIC vs host clock delta.

0: https://lore.kernel.org/bpf/f3a116dc-1b14-3432-ad20-a36179ef0608@redhat.com/

Fixes: 297a3f124155 ("selftests/bpf: Simple program to dump XDP RX metadata")
Reported-by: Jesper Dangaard Brouer <jbrouer@redhat.com>
Tested-by: Jesper Dangaard Brouer <jbrouer@redhat.com>
Signed-off-by: Stanislav Fomichev <sdf@google.com>
---
 tools/testing/selftests/bpf/xdp_hw_metadata.c | 28 ++++++++++++++++++-
 1 file changed, 27 insertions(+), 1 deletion(-)

Comments

Martin KaFai Lau Jan. 26, 2023, 1:12 a.m. UTC | #1 
On 1/25/23 2:32 PM, Stanislav Fomichev wrote:
> The existing timestamping_enable() is a no-op because it applies
> to the socket-related path that we are not verifying here
> anymore. (but still leaving the code around hoping we can
> have xdp->skb path verified here as well)
> 
>    poll: 1 (0)
>    xsk_ring_cons__peek: 1
>    0xf64788: rx_desc[0]->addr=100000000008000 addr=8100 comp_addr=8000
>    rx_hash: 3697961069
>    rx_timestamp:  1674657672142214773 (sec:1674657672.1422)
>    XDP RX-time:   1674657709561774876 (sec:1674657709.5618) delta sec:37.4196
>    AF_XDP time:   1674657709561871034 (sec:1674657709.5619) delta
> sec:0.0001 (96.158 usec)
>    0xf64788: complete idx=8 addr=8000
> 
> Also, maybe something to archive here, see [0] for Jesper's note
> about NIC vs host clock delta.
> 
> 0: https://lore.kernel.org/bpf/f3a116dc-1b14-3432-ad20-a36179ef0608@redhat.com/
> 
> Fixes: 297a3f124155 ("selftests/bpf: Simple program to dump XDP RX metadata")
> Reported-by: Jesper Dangaard Brouer <jbrouer@redhat.com>
> Tested-by: Jesper Dangaard Brouer <jbrouer@redhat.com>
> Signed-off-by: Stanislav Fomichev <sdf@google.com>
> ---
>   tools/testing/selftests/bpf/xdp_hw_metadata.c | 28 ++++++++++++++++++-
>   1 file changed, 27 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/testing/selftests/bpf/xdp_hw_metadata.c b/tools/testing/selftests/bpf/xdp_hw_metadata.c
> index 0008f0f239e8..dc899c53db5e 100644
> --- a/tools/testing/selftests/bpf/xdp_hw_metadata.c
> +++ b/tools/testing/selftests/bpf/xdp_hw_metadata.c
> @@ -24,6 +24,7 @@
>   #include <linux/net_tstamp.h>
>   #include <linux/udp.h>
>   #include <linux/sockios.h>
> +#include <linux/net_tstamp.h>
>   #include <sys/mman.h>
>   #include <net/if.h>
>   #include <poll.h>
> @@ -278,13 +279,36 @@ static int rxq_num(const char *ifname)
>   
>   	ret = ioctl(fd, SIOCETHTOOL, &ifr);
>   	if (ret < 0)
> -		error(-1, errno, "socket");
> +		error(-1, errno, "ioctl(SIOCETHTOOL)");
>   
>   	close(fd);
>   
>   	return ch.rx_count + ch.combined_count;
>   }
>   
> +static void hwtstamp_enable(const char *ifname)
> +{
> +	struct hwtstamp_config cfg = {
> +		.rx_filter = HWTSTAMP_FILTER_ALL,
> +	};
> +
> +	struct ifreq ifr = {
> +		.ifr_data = (void *)&cfg,
> +	};
> +	strcpy(ifr.ifr_name, ifname);
> +	int fd, ret;
> +
> +	fd = socket(AF_UNIX, SOCK_DGRAM, 0);
> +	if (fd < 0)
> +		error(-1, errno, "socket");
> +
> +	ret = ioctl(fd, SIOCSHWTSTAMP, &ifr);
> +	if (ret < 0)
> +		error(-1, errno, "ioctl(SIOCSHWTSTAMP)");
> +
> +	close(fd);
> +}
> +
>   static void cleanup(void)
>   {
>   	LIBBPF_OPTS(bpf_xdp_attach_opts, opts);
> @@ -341,6 +365,8 @@ int main(int argc, char *argv[])
>   
>   	printf("rxq: %d\n", rxq);
>   
> +	hwtstamp_enable(ifname);

Will it to restore to the earlier setting when the process exits?
Stanislav Fomichev Jan. 26, 2023, 5:31 p.m. UTC | #2 
On Wed, Jan 25, 2023 at 5:12 PM Martin KaFai Lau <martin.lau@linux.dev> wrote:
>
> On 1/25/23 2:32 PM, Stanislav Fomichev wrote:
> > The existing timestamping_enable() is a no-op because it applies
> > to the socket-related path that we are not verifying here
> > anymore. (but still leaving the code around hoping we can
> > have xdp->skb path verified here as well)
> >
> >    poll: 1 (0)
> >    xsk_ring_cons__peek: 1
> >    0xf64788: rx_desc[0]->addr=100000000008000 addr=8100 comp_addr=8000
> >    rx_hash: 3697961069
> >    rx_timestamp:  1674657672142214773 (sec:1674657672.1422)
> >    XDP RX-time:   1674657709561774876 (sec:1674657709.5618) delta sec:37.4196
> >    AF_XDP time:   1674657709561871034 (sec:1674657709.5619) delta
> > sec:0.0001 (96.158 usec)
> >    0xf64788: complete idx=8 addr=8000
> >
> > Also, maybe something to archive here, see [0] for Jesper's note
> > about NIC vs host clock delta.
> >
> > 0: https://lore.kernel.org/bpf/f3a116dc-1b14-3432-ad20-a36179ef0608@redhat.com/
> >
> > Fixes: 297a3f124155 ("selftests/bpf: Simple program to dump XDP RX metadata")
> > Reported-by: Jesper Dangaard Brouer <jbrouer@redhat.com>
> > Tested-by: Jesper Dangaard Brouer <jbrouer@redhat.com>
> > Signed-off-by: Stanislav Fomichev <sdf@google.com>
> > ---
> >   tools/testing/selftests/bpf/xdp_hw_metadata.c | 28 ++++++++++++++++++-
> >   1 file changed, 27 insertions(+), 1 deletion(-)
> >
> > diff --git a/tools/testing/selftests/bpf/xdp_hw_metadata.c b/tools/testing/selftests/bpf/xdp_hw_metadata.c
> > index 0008f0f239e8..dc899c53db5e 100644
> > --- a/tools/testing/selftests/bpf/xdp_hw_metadata.c
> > +++ b/tools/testing/selftests/bpf/xdp_hw_metadata.c
> > @@ -24,6 +24,7 @@
> >   #include <linux/net_tstamp.h>
> >   #include <linux/udp.h>
> >   #include <linux/sockios.h>
> > +#include <linux/net_tstamp.h>
> >   #include <sys/mman.h>
> >   #include <net/if.h>
> >   #include <poll.h>
> > @@ -278,13 +279,36 @@ static int rxq_num(const char *ifname)
> >
> >       ret = ioctl(fd, SIOCETHTOOL, &ifr);
> >       if (ret < 0)
> > -             error(-1, errno, "socket");
> > +             error(-1, errno, "ioctl(SIOCETHTOOL)");
> >
> >       close(fd);
> >
> >       return ch.rx_count + ch.combined_count;
> >   }
> >
> > +static void hwtstamp_enable(const char *ifname)
> > +{
> > +     struct hwtstamp_config cfg = {
> > +             .rx_filter = HWTSTAMP_FILTER_ALL,
> > +     };
> > +
> > +     struct ifreq ifr = {
> > +             .ifr_data = (void *)&cfg,
> > +     };
> > +     strcpy(ifr.ifr_name, ifname);
> > +     int fd, ret;
> > +
> > +     fd = socket(AF_UNIX, SOCK_DGRAM, 0);
> > +     if (fd < 0)
> > +             error(-1, errno, "socket");
> > +
> > +     ret = ioctl(fd, SIOCSHWTSTAMP, &ifr);
> > +     if (ret < 0)
> > +             error(-1, errno, "ioctl(SIOCSHWTSTAMP)");
> > +
> > +     close(fd);
> > +}
> > +
> >   static void cleanup(void)
> >   {
> >       LIBBPF_OPTS(bpf_xdp_attach_opts, opts);
> > @@ -341,6 +365,8 @@ int main(int argc, char *argv[])
> >
> >       printf("rxq: %d\n", rxq);
> >
> > +     hwtstamp_enable(ifname);
>
> Will it to restore to the earlier setting when the process exits?

Nope, do you think we should? Probably a good idea, let me add that...
Jesper Dangaard Brouer Jan. 27, 2023, 9:34 a.m. UTC | #3 
On 25/01/2023 23.32, Stanislav Fomichev wrote:
> The existing timestamping_enable() is a no-op because it applies
> to the socket-related path that we are not verifying here
> anymore. (but still leaving the code around hoping we can
> have xdp->skb path verified here as well)
> 
>    poll: 1 (0)
>    xsk_ring_cons__peek: 1
>    0xf64788: rx_desc[0]->addr=100000000008000 addr=8100 comp_addr=8000
>    rx_hash: 3697961069
>    rx_timestamp:  1674657672142214773 (sec:1674657672.1422)
>    XDP RX-time:   1674657709561774876 (sec:1674657709.5618) delta sec:37.4196
>    AF_XDP time:   1674657709561871034 (sec:1674657709.5619) delta sec:0.0001 (96.158 usec)

This output contains some extra output data, which is not part of
current upstream.
I will soon submit an RFC-patch with this extra output to discuss and
figure out what timestamp type we want/expect HW to provide.

>    0xf64788: complete idx=8 addr=8000
> 
> Also, maybe something to archive here, see [0] for Jesper's note
> about NIC vs host clock delta.
> 
> 0: https://lore.kernel.org/bpf/f3a116dc-1b14-3432-ad20-a36179ef0608@redhat.com/
> 

--Jesper
diff mbox series

Patch

diff --git a/tools/testing/selftests/bpf/xdp_hw_metadata.c b/tools/testing/selftests/bpf/xdp_hw_metadata.c
index 0008f0f239e8..dc899c53db5e 100644
--- a/tools/testing/selftests/bpf/xdp_hw_metadata.c
+++ b/tools/testing/selftests/bpf/xdp_hw_metadata.c
@@ -24,6 +24,7 @@ 
 #include <linux/net_tstamp.h>
 #include <linux/udp.h>
 #include <linux/sockios.h>
+#include <linux/net_tstamp.h>
 #include <sys/mman.h>
 #include <net/if.h>
 #include <poll.h>
@@ -278,13 +279,36 @@  static int rxq_num(const char *ifname)
 
 	ret = ioctl(fd, SIOCETHTOOL, &ifr);
 	if (ret < 0)
-		error(-1, errno, "socket");
+		error(-1, errno, "ioctl(SIOCETHTOOL)");
 
 	close(fd);
 
 	return ch.rx_count + ch.combined_count;
 }
 
+static void hwtstamp_enable(const char *ifname)
+{
+	struct hwtstamp_config cfg = {
+		.rx_filter = HWTSTAMP_FILTER_ALL,
+	};
+
+	struct ifreq ifr = {
+		.ifr_data = (void *)&cfg,
+	};
+	strcpy(ifr.ifr_name, ifname);
+	int fd, ret;
+
+	fd = socket(AF_UNIX, SOCK_DGRAM, 0);
+	if (fd < 0)
+		error(-1, errno, "socket");
+
+	ret = ioctl(fd, SIOCSHWTSTAMP, &ifr);
+	if (ret < 0)
+		error(-1, errno, "ioctl(SIOCSHWTSTAMP)");
+
+	close(fd);
+}
+
 static void cleanup(void)
 {
 	LIBBPF_OPTS(bpf_xdp_attach_opts, opts);
@@ -341,6 +365,8 @@  int main(int argc, char *argv[])
 
 	printf("rxq: %d\n", rxq);
 
+	hwtstamp_enable(ifname);
+
 	rx_xsk = malloc(sizeof(struct xsk) * rxq);
 	if (!rx_xsk)
 		error(-1, ENOMEM, "malloc");