* [PATCH bpf-next v2] selftests/bpf: Properly enable hwtstamp in xdp_hw_metadata
@ 2023-01-26 22:50 Stanislav Fomichev
2023-01-27 7:20 ` patchwork-bot+netdevbpf
2023-01-27 13:36 ` Jesper Dangaard Brouer
0 siblings, 2 replies; 4+ messages in thread
From: Stanislav Fomichev @ 2023-01-26 22:50 UTC (permalink / raw)
To: bpf
Cc: ast, daniel, andrii, martin.lau, song, yhs, john.fastabend,
kpsingh, sdf, haoluo, jolsa, Jesper Dangaard Brouer
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/
v2:
- Restore original value (Martin)
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 | 45 ++++++++++++++++++-
1 file changed, 44 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..3823b1c499cc 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,53 @@ 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_ioctl(int op, const char *ifname, struct hwtstamp_config *cfg)
+{
+ 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, op, &ifr);
+ if (ret < 0)
+ error(-1, errno, "ioctl(%d)", op);
+
+ close(fd);
+}
+
+static struct hwtstamp_config saved_hwtstamp_cfg;
+static const char *saved_hwtstamp_ifname;
+
+static void hwtstamp_restore(void)
+{
+ hwtstamp_ioctl(SIOCSHWTSTAMP, saved_hwtstamp_ifname, &saved_hwtstamp_cfg);
+}
+
+static void hwtstamp_enable(const char *ifname)
+{
+ struct hwtstamp_config cfg = {
+ .rx_filter = HWTSTAMP_FILTER_ALL,
+ };
+
+ hwtstamp_ioctl(SIOCGHWTSTAMP, ifname, &saved_hwtstamp_cfg);
+ saved_hwtstamp_ifname = strdup(ifname);
+ atexit(hwtstamp_restore);
+
+ hwtstamp_ioctl(SIOCSHWTSTAMP, ifname, &cfg);
+}
+
static void cleanup(void)
{
LIBBPF_OPTS(bpf_xdp_attach_opts, opts);
@@ -341,6 +382,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");
--
2.39.1.456.gfc5497dd1b-goog
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH bpf-next v2] selftests/bpf: Properly enable hwtstamp in xdp_hw_metadata
2023-01-26 22:50 [PATCH bpf-next v2] selftests/bpf: Properly enable hwtstamp in xdp_hw_metadata Stanislav Fomichev
@ 2023-01-27 7:20 ` patchwork-bot+netdevbpf
2023-01-27 13:36 ` Jesper Dangaard Brouer
1 sibling, 0 replies; 4+ messages in thread
From: patchwork-bot+netdevbpf @ 2023-01-27 7:20 UTC (permalink / raw)
To: Stanislav Fomichev
Cc: bpf, ast, daniel, andrii, martin.lau, song, yhs, john.fastabend,
kpsingh, haoluo, jolsa, jbrouer
Hello:
This patch was applied to bpf/bpf-next.git (master)
by Martin KaFai Lau <martin.lau@kernel.org>:
On Thu, 26 Jan 2023 14:50:30 -0800 you 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
>
> [...]
Here is the summary with links:
- [bpf-next,v2] selftests/bpf: Properly enable hwtstamp in xdp_hw_metadata
https://git.kernel.org/bpf/bpf-next/c/a5f3a3f7c172
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH bpf-next v2] selftests/bpf: Properly enable hwtstamp in xdp_hw_metadata
2023-01-26 22:50 [PATCH bpf-next v2] selftests/bpf: Properly enable hwtstamp in xdp_hw_metadata Stanislav Fomichev
2023-01-27 7:20 ` patchwork-bot+netdevbpf
@ 2023-01-27 13:36 ` Jesper Dangaard Brouer
2023-01-27 17:09 ` Stanislav Fomichev
1 sibling, 1 reply; 4+ messages in thread
From: Jesper Dangaard Brouer @ 2023-01-27 13:36 UTC (permalink / raw)
To: Stanislav Fomichev, bpf
Cc: brouer, ast, daniel, andrii, martin.lau, song, yhs,
john.fastabend, kpsingh, haoluo, jolsa, Jesper Dangaard Brouer
On 26/01/2023 23.50, 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
>
Again for the record, this output is from my devel version of
xdp_hw_metadata and not what is currently upstream.
Small nit below, which is too late as this is already applied.
> 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/
>
> v2:
> - Restore original value (Martin)
>
> 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 | 45 ++++++++++++++++++-
> 1 file changed, 44 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..3823b1c499cc 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,53 @@ 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_ioctl(int op, const char *ifname, struct hwtstamp_config *cfg)
> +{
> + struct ifreq ifr = {
> + .ifr_data = (void *)cfg,
> + };
> + strcpy(ifr.ifr_name, ifname);
I would use strncpy like:
strncpy(ifr.ifr_name, ifname, IFNAMSIZ - 1);
> + int fd, ret;
> +
> + fd = socket(AF_UNIX, SOCK_DGRAM, 0);
> + if (fd < 0)
> + error(-1, errno, "socket");
> +
> + ret = ioctl(fd, op, &ifr);
> + if (ret < 0)
> + error(-1, errno, "ioctl(%d)", op);
> +
> + close(fd);
> +}
> +
> +static struct hwtstamp_config saved_hwtstamp_cfg;
> +static const char *saved_hwtstamp_ifname;
> +
> +static void hwtstamp_restore(void)
> +{
> + hwtstamp_ioctl(SIOCSHWTSTAMP, saved_hwtstamp_ifname, &saved_hwtstamp_cfg);
> +}
> +
> +static void hwtstamp_enable(const char *ifname)
> +{
> + struct hwtstamp_config cfg = {
> + .rx_filter = HWTSTAMP_FILTER_ALL,
> + };
> +
> + hwtstamp_ioctl(SIOCGHWTSTAMP, ifname, &saved_hwtstamp_cfg);
> + saved_hwtstamp_ifname = strdup(ifname);
> + atexit(hwtstamp_restore);
> +
> + hwtstamp_ioctl(SIOCSHWTSTAMP, ifname, &cfg);
> +}
> +
> static void cleanup(void)
> {
> LIBBPF_OPTS(bpf_xdp_attach_opts, opts);
> @@ -341,6 +382,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");
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH bpf-next v2] selftests/bpf: Properly enable hwtstamp in xdp_hw_metadata
2023-01-27 13:36 ` Jesper Dangaard Brouer
@ 2023-01-27 17:09 ` Stanislav Fomichev
0 siblings, 0 replies; 4+ messages in thread
From: Stanislav Fomichev @ 2023-01-27 17:09 UTC (permalink / raw)
To: Jesper Dangaard Brouer
Cc: bpf, brouer, ast, daniel, andrii, martin.lau, song, yhs,
john.fastabend, kpsingh, haoluo, jolsa
On Fri, Jan 27, 2023 at 5:36 AM Jesper Dangaard Brouer
<jbrouer@redhat.com> wrote:
>
>
>
> On 26/01/2023 23.50, 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
> >
>
> Again for the record, this output is from my devel version of
> xdp_hw_metadata and not what is currently upstream.
Yeah, that's fine, I just wanted to record it somewhere :-)
> Small nit below, which is too late as this is already applied.
>
> > 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/
> >
> > v2:
> > - Restore original value (Martin)
> >
> > 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 | 45 ++++++++++++++++++-
> > 1 file changed, 44 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..3823b1c499cc 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,53 @@ 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_ioctl(int op, const char *ifname, struct hwtstamp_config *cfg)
> > +{
> > + struct ifreq ifr = {
> > + .ifr_data = (void *)cfg,
> > + };
> > + strcpy(ifr.ifr_name, ifname);
>
> I would use strncpy like:
>
> strncpy(ifr.ifr_name, ifname, IFNAMSIZ - 1);
Good point, maybe we can fold this into some of the next patches to
this file. Since this is for internal dev consumption, doesn't look
like a huge deal to me to deserve a separate patch.
> > + int fd, ret;
> > +
> > + fd = socket(AF_UNIX, SOCK_DGRAM, 0);
> > + if (fd < 0)
> > + error(-1, errno, "socket");
> > +
> > + ret = ioctl(fd, op, &ifr);
> > + if (ret < 0)
> > + error(-1, errno, "ioctl(%d)", op);
> > +
> > + close(fd);
> > +}
> > +
> > +static struct hwtstamp_config saved_hwtstamp_cfg;
> > +static const char *saved_hwtstamp_ifname;
> > +
> > +static void hwtstamp_restore(void)
> > +{
> > + hwtstamp_ioctl(SIOCSHWTSTAMP, saved_hwtstamp_ifname, &saved_hwtstamp_cfg);
> > +}
> > +
> > +static void hwtstamp_enable(const char *ifname)
> > +{
> > + struct hwtstamp_config cfg = {
> > + .rx_filter = HWTSTAMP_FILTER_ALL,
> > + };
> > +
> > + hwtstamp_ioctl(SIOCGHWTSTAMP, ifname, &saved_hwtstamp_cfg);
> > + saved_hwtstamp_ifname = strdup(ifname);
> > + atexit(hwtstamp_restore);
> > +
> > + hwtstamp_ioctl(SIOCSHWTSTAMP, ifname, &cfg);
> > +}
> > +
> > static void cleanup(void)
> > {
> > LIBBPF_OPTS(bpf_xdp_attach_opts, opts);
> > @@ -341,6 +382,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");
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2023-01-27 17:11 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-01-26 22:50 [PATCH bpf-next v2] selftests/bpf: Properly enable hwtstamp in xdp_hw_metadata Stanislav Fomichev
2023-01-27 7:20 ` patchwork-bot+netdevbpf
2023-01-27 13:36 ` Jesper Dangaard Brouer
2023-01-27 17:09 ` Stanislav Fomichev
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox