Re: [RFC bpf-next 6/6] samples/bpf: Add meta data hash example to xdp_redirect_cpu

[Jesper Dangaard Brouer <brouer@redhat.com>]

On Tue, 26 Jun 2018 19:46:15 -0700
Saeed Mahameed <saeedm@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.

> 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@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.

> +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.

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.).

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.


> +	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

-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer