Date:   Wed, 27 Jun 2018 18:04:18 +0000
From:   Saeed Mahameed <saeedm@...lanox.com>
To:     "saeedm@....mellanox.co.il" <saeedm@....mellanox.co.il>,
        "brouer@...hat.com" <brouer@...hat.com>
CC:     "alexander.h.duyck@...el.com" <alexander.h.duyck@...el.com>,
        "peter.waskiewicz.jr@...el.com" <peter.waskiewicz.jr@...el.com>,
        Rony Efraim <ronye@...lanox.com>,
        "borkmann@...earbox.net" <borkmann@...earbox.net>,
        Tariq Toukan <tariqt@...lanox.com>,
        "neerav.parikh@...el.com" <neerav.parikh@...el.com>,
        Opher Reviv <opher@...lanox.com>,
        "alexei.starovoitov@...il.com" <alexei.starovoitov@...il.com>,
        "pjwaskiewicz@...il.com" <pjwaskiewicz@...il.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "ttoukan.linux@...il.com" <ttoukan.linux@...il.com>
Subject: Re: [RFC bpf-next 6/6] samples/bpf: Add meta data hash example to
 xdp_redirect_cpu

On Wed, 2018-06-27 at 12:59 +0200, Jesper Dangaard Brouer wrote:
> On Tue, 26 Jun 2018 19:46:15 -0700
> Saeed Mahameed <saeedm@....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 <saeedm@...lanox.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
> 
> 

Powered by blists - more mailing lists