[xdp-hints] Re: [PATCH bpf-next] selftests/bpf: add options and ZC mode to xdp_hw_metadata

[Larysa Zaremba <larysa.zaremba@intel.com>]

On Mon, Oct 02, 2023 at 09:46:08AM -0700, Stanislav Fomichev wrote:
> On Mon, Oct 2, 2023 at 9:35 AM Larysa Zaremba <larysa.zaremba@intel.com> wrote:
> >
> > By default, xdp_hw_metadata runs in AF_XDP copy mode. However, hints are
> > also supposed to be supported in ZC mode, which is usually implemented
> > separately in driver, and so needs to be tested too.
> >
> > Add an option to run xdp_hw_metadata in ZC mode.
> >
> > As for now, xdp_hw_metadata accepts no options, so add simple option
> > parsing logic and a help message.
> >
> > For quick reference, also add an ingress packet generation command to the
> > help message. The command comes from [0].
> >
> > [0] https://lore.kernel.org/all/20230119221536.3349901-18-sdf@google.com/
> 
> I did similar changes in my pending [0], but I made the zerocopy, not
> the copy mode, the default.
> If you want to get this in faster (my series will probably need
> another iteration), let's maybe do the same here?
> ZC as a default feels better.
> 
> 0: https://lore.kernel.org/bpf/20230914210452.2588884-9-sdf@google.com/

I do not need those changes in tree ASAP, that is just something I had locally 
for some time and decided to send. So I think I can wait for your series. This 
way it is less work for both of us.

> 
> 
> > Signed-off-by: Larysa Zaremba <larysa.zaremba@intel.com>
> > ---
> >  tools/testing/selftests/bpf/xdp_hw_metadata.c | 59 ++++++++++++++++---
> >  1 file changed, 52 insertions(+), 7 deletions(-)
> >
> > diff --git a/tools/testing/selftests/bpf/xdp_hw_metadata.c b/tools/testing/selftests/bpf/xdp_hw_metadata.c
> > index 613321eb84c1..c1d1b161a964 100644
> > --- a/tools/testing/selftests/bpf/xdp_hw_metadata.c
> > +++ b/tools/testing/selftests/bpf/xdp_hw_metadata.c
> > @@ -26,6 +26,7 @@
> >  #include <linux/sockios.h>
> >  #include <sys/mman.h>
> >  #include <net/if.h>
> > +#include <ctype.h>
> >  #include <poll.h>
> >  #include <time.h>
> >
> > @@ -49,6 +50,7 @@ struct xsk {
> >  struct xdp_hw_metadata *bpf_obj;
> >  struct xsk *rx_xsk;
> >  const char *ifname;
> > +bool zero_copy;
> >  int ifindex;
> >  int rxq;
> >
> > @@ -60,7 +62,7 @@ static int open_xsk(int ifindex, struct xsk *xsk, __u32 queue_id)
> >         const struct xsk_socket_config socket_config = {
> >                 .rx_size = XSK_RING_PROD__DEFAULT_NUM_DESCS,
> >                 .tx_size = XSK_RING_PROD__DEFAULT_NUM_DESCS,
> > -               .bind_flags = XDP_COPY,
> > +               .bind_flags = zero_copy ? XDP_ZEROCOPY : XDP_COPY,
> >         };
> >         const struct xsk_umem_config umem_config = {
> >                 .fill_size = XSK_RING_PROD__DEFAULT_NUM_DESCS,
> > @@ -404,6 +406,54 @@ static void timestamping_enable(int fd, int val)
> >                 error(1, errno, "setsockopt(SO_TIMESTAMPING)");
> >  }
> >
> > +static void print_usage(void)
> > +{
> > +       const char *usage =
> > +               "  Usage: xdp_hw_metadata [OPTIONS] [IFNAME]\n"
> 
> Maybe [OPTIONS] <IFNAME> to mark ifname as required?
> 
> > +               "  Options:\n"
> > +               "  -z            Run AF_XDP in ZC mode (copy mode is used by default)\n"
> > +               "  -h            Display this help and exit\n\n"
> > +               "  Generate test packets on other machine with:\n"
> > +               "    echo -n xdp | nc -u -q1 <dst_ip> 9091\n";
> > +
> > +       printf("%s", usage);
> > +}
> > +
> > +static void read_args(int argc, char *argv[])
> > +{
> > +       char opt;
> > +
> > +       while ((opt = getopt(argc, argv, "zh")) != -1) {
> > +               switch (opt) {
> > +               case 'z':
> > +                       zero_copy = true;
> > +                       break;
> > +               case 'h':
> > +                       print_usage();
> > +                       exit(0);
> > +               case '?':
> > +                       if (isprint(optopt))
> > +                               fprintf(stderr, "Unknown option: -%c\n", optopt);
> > +                       fallthrough;
> > +               default:
> > +                       print_usage();
> > +                       error(-1, opterr, "Command line options error");
> > +               }
> > +       }
> > +
> > +       if (optind >= argc) {
> > +               fprintf(stderr, "No device name provided\n");
> > +               print_usage();
> > +               exit(-1);
> > +       }
> > +
> > +       ifname = argv[optind];
> > +       ifindex = if_nametoindex(ifname);
> > +
> > +       if (!ifname)
> > +               error(-1, errno, "Invalid interface name");
> > +}
> > +
> >  int main(int argc, char *argv[])
> >  {
> >         clockid_t clock_id = CLOCK_TAI;
> > @@ -413,13 +463,8 @@ int main(int argc, char *argv[])
> >
> >         struct bpf_program *prog;
> >
> > -       if (argc != 2) {
> > -               fprintf(stderr, "pass device name\n");
> > -               return -1;
> > -       }
> > +       read_args(argc, argv);
> >
> > -       ifname = argv[1];
> > -       ifindex = if_nametoindex(ifname);
> >         rxq = rxq_num(ifname);
> >
> >         printf("rxq: %d\n", rxq);
> > --
> > 2.41.0
> >