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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, Jan 27, 2023 at 5:36 AM Jesper Dangaard Brouer
<jbrouer@xxxxxxxxxx> 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@xxxxxxxxxx/
> >
> > v2:
> > - Restore original value (Martin)
> >
> > Fixes: 297a3f124155 ("selftests/bpf: Simple program to dump XDP RX metadata")
> > Reported-by: Jesper Dangaard Brouer <jbrouer@xxxxxxxxxx>
> > Tested-by: Jesper Dangaard Brouer <jbrouer@xxxxxxxxxx>
> > Signed-off-by: Stanislav Fomichev <sdf@xxxxxxxxxx>
> > ---
> >   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");
>



[Index of Archives]     [Linux Samsung SoC]     [Linux Rockchip SoC]     [Linux Actions SoC]     [Linux for Synopsys ARC Processors]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]


  Powered by Linux