On Tue, 23 Aug 2022 00:59:01 -0700
John Fastabend <john.fastabend@xxxxxxxxx> wrote:
> > + * long bpf_skb_set_var_tunnel_opt(struct sk_buff *skb, void *opt, u32 size, u32 len)
> > + * Description
> > + * Set tunnel options metadata for the packet associated to *skb*
> > + * to the variable length *len* bytes of option data contained in
> > + * the raw buffer *opt* sized *size*.
> > + *
> > + * See also the description of the **bpf_skb_get_tunnel_opt**\ ()
> > + * helper for additional information.
> > + * Return
> > + * 0 on success, or a negative error in case of failure.
>
> This API feels akward to me. Could you collapse this by using a dynamic pointer,
> recently added? And drop the ptr_to_mem+const_size part at least? That seems
> redundant with latest kernels.
Revisiting this decision.
After following that path, seems to me that adding the newly proposed
'bpf_skb_set_tunnel_opt_dynptr' API creates awkwardness in user's bpf
program.
Suppose user needs to hold a map of the options received on incoming
traffic based on whatever 'bpf_skb_get_tunnel_opt' returns.
Then, when user needs to apply the options on the return traffic, we
have the following two alternative APIs:
option A: bpf_skb_set_var_tunnel_opt
------------------------------------
struct tun_opts {
__u8 data[MAX_OPT_SZ];
__u32 len;
};
BPF_MAP_TYPE_HASH opts_map; // __type(value, tun_opts)
...
struct tun_opts *opts;
opts = bpf_map_lookup_elem(&opts_map, &the_flow_key);
bpf_skb_set_var_tunnel_opt(skb, opts->data, sizeof(opts->data), opts->len);
option B: bpf_skb_set_tunnel_opt_dynptr
---------------------------------------
struct tun_opts {
__u8 data[MAX_OPT_SZ];
};
BPF_MAP_TYPE_HASH opts_map; // __type(value, tun_opts)
BPF_MAP_TYPE_HASH opts_len_map; // __type(value, __u32)
...
struct bpf_dynptr dptr;
struct tun_opts *opts;
__u32 *opts_len;
opts = bpf_map_lookup_elem(&opts_map, &the_flow_key);
opts_len = bpf_map_lookup_elem(&opts_len_map, &the_flow_key);
bpf_dynptr_from_mem(opts, sizeof(*opts), 0, &dptr); // construct a dynptr from the raw option data
bpf_dynptr_trim(&dptr, opts_len); // trim it based on stored option len
bpf_skb_set_tunnel_opt_dynptr(skb, &dptr);
IMO, the 2nd user program is less readable:
- need to store the received options length in a separate map
- 5 bpf function calls instead of 2
Despite the awkwardness of the 'bpf_skb_set_var_tunnel_opt' API (passing
both constant size *and* dynamic len), it really creates more simple and
readable ebpf programs.
WDYT?
Best
Shmulik
