On Wed, 2018-06-27 at 12:59 +0200, Jesper Dangaard Brouer wrote:
> On Tue, 26 Jun 2018 19:46:15 -0700
> Saeed Mahameed <sae...@dev.mellanox.co.il> wrote:
>
> > Add a new program (prog_num = 4) that will not parse packets and
> > will
> > use the meta data hash to spread/redirect traffic into different
> > cpus.
>
> You cannot "steal" prognum 4, as it is already used for
> "xdp_prognum4_ddos_filter_pktgen". Please append your new prog as
> #5.
>
Sure.
> > For the new program we set on bpf_set_link_xdp_fd:
> > xdp_flags |= XDP_FLAGS_META_HASH | XDP_FLAGS_META_VLAN;
> >
> > On mlx5 it will succeed since mlx5 already supports these flags.
> >
> > The new program will read the value of the hash from the data_meta
> > pointer from the xdp_md and will use it to compute the destination
> > cpu.
> >
> > Note: I didn't test this patch to show redirect works with the
> > hash!
> > I only used it to see that the hash and vlan values are set
> > correctly
> > by the driver and can be seen by the xdp program.
> >
> > * I faced some difficulties to read the hash value using the helper
> > functions defined in the previous patches, but once i used the same
> > logic
> > with out these functions it worked ! Will have to figure this out
> > later.
> >
> > Signed-off-by: Saeed Mahameed <sae...@mellanox.com>
> > ---
> > samples/bpf/xdp_redirect_cpu_kern.c | 67
> > +++++++++++++++++++++++++++++
> > samples/bpf/xdp_redirect_cpu_user.c | 7 +++
> > 2 files changed, 74 insertions(+)
> >
> > diff --git a/samples/bpf/xdp_redirect_cpu_kern.c
> > b/samples/bpf/xdp_redirect_cpu_kern.c
> > index 303e9e7161f3..d6b3f55f342a 100644
> > --- a/samples/bpf/xdp_redirect_cpu_kern.c
> > +++ b/samples/bpf/xdp_redirect_cpu_kern.c
> > @@ -376,6 +376,73 @@ int xdp_prognum3_proto_separate(struct xdp_md
> > *ctx)
> > return bpf_redirect_map(&cpu_map, cpu_dest, 0);
> > }
> >
> > +#if 0
> > +xdp_md_info_arr mdi = {
> > + [XDP_DATA_META_HASH] = {.offset = 0, .present = 1},
> > + [XDP_DATA_META_VLAN] = {.offset = sizeof(struct
> > xdp_md_hash), .present = 1},
> > +};
> > +#endif
>
> Sorry, no global variables avail in the generated BPF byte-code.
>
Yea i found out the hard way :), but for my final solution i would like
to somehow share the same static md info array between netdev and bpf
program, so this code was just experimental.
> > +SEC("xdp_cpu_map4_hash_separate")
> > +int xdp_prognum4_hash_separate(struct xdp_md *ctx)
> > +{
> > + void *data_meta = (void *)(long)ctx->data_meta;
> > + void *data_end = (void *)(long)ctx->data_end;
> > + void *data = (void *)(long)ctx->data;
> > + struct xdp_md_hash *hash;
> > + struct xdp_md_vlan *vlan;
> > + struct datarec *rec;
> > + u32 cpu_dest = 0;
> > + u32 cpu_idx = 0;
> > + u32 *cpu_lookup;
> > + u32 key = 0;
> > +
> > + /* Count RX packet in map */
> > + rec = bpf_map_lookup_elem(&rx_cnt, &key);
> > + if (!rec)
> > + return XDP_ABORTED;
> > + rec->processed++;
> > +
> > + /* for some reason this code fails to be verified */
> > +#if 0
> > + hash = xdp_data_meta_get_hash(mdi, data_meta);
>
> This will not work, because it is not implemented as a proper
> BPF-helper call.
>
> First, you currently store the xdp_md_info_arr inside the driver,
> which
> makes it hard for a helper to access this. For helper access, we
> could
> store this in xdp_rxq_info.
>
Good Idea!
> Second, in your design it looks like you are introducing a helper per
> possible item in xdp_md_info_arr. I think we can reduce this to a
> single helper, that takes a XDP_DATA_META_xxx flag, and returns an
> offset. (The helper could return a direct pointer, but I don't think
> the verfier can handle that, as it need to "see" this is related to
> data_meta pointer, and that we do the proper boundry checks.).
>
We can update the verifier to allow access to any offset between
data_meta and data_meta + offset(last meta data) + sizeof(last meta
data) ?
> The BPF prog already have direct memory access to the data_meta area,
> and all it really need is an offset. Sure, the XDP/bpf programmer
> could just calculate these offsets as constants, and remember to load
> the XDP prog with the flags that corresponds to the calculated
> offsets.
>
> But I think we can do something even smarter...
>
> It should be possible to convert/patch the BPF instructions, of the
> helper call that returns an offset, to instead avoid the call and
> either (1) provide the offset as a constant/IMM or (2) make BPF inst
> doing the lookup in xdp_md_info_arr.
>
I vote (2).
>
> > + if (hash + 1 > data)
> > + return XDP_ABORTED;
> > +
> > + vlan = xdp_data_meta_get_vlan(mdi, data_meta);
> > + if (vlan + 1 > data)
> > + return XDP_ABORTED;
> > +#endif
> > +
> > + /* Work around for the above code */
> > + hash = data_meta; /* since we know hash will appear first
> > */
> > + if (hash + 1 > data)
> > + return XDP_ABORTED;
> > +
> > +#if 0
> > + // Just for testing
> > + /* We know that vlan will appear after the hash */
> > + vlan = (void *)((char *)data_meta + sizeof(*hash));
> > + if (vlan + 1 > data) {
> > + return XDP_ABORTED;
> > + }
> > +#endif
>
>
