Lorenzo Bianconi wrote:
> From: Eelco Chaudron <echaudro@xxxxxxxxxx>
>
> This patch adds support for multi-buffer for the following helpers:
> - bpf_xdp_output()
> - bpf_perf_event_output()
>
> Signed-off-by: Eelco Chaudron <echaudro@xxxxxxxxxx>
> Signed-off-by: Lorenzo Bianconi <lorenzo@xxxxxxxxxx>
> ---
Ah ok so at least xdp_output will work with all bytes. But this is
getting close to having access into the frags so I think doing
the last bit shouldn't be too hard?
> kernel/trace/bpf_trace.c | 3 +
> net/core/filter.c | 72 +++++++++-
> .../selftests/bpf/prog_tests/xdp_bpf2bpf.c | 127 ++++++++++++------
> .../selftests/bpf/progs/test_xdp_bpf2bpf.c | 2 +-
> 4 files changed, 160 insertions(+), 44 deletions(-)
>
> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index d2d7cf6cfe83..ee926ec64f78 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -1365,6 +1365,7 @@ static const struct bpf_func_proto bpf_perf_event_output_proto_raw_tp = {
>
> extern const struct bpf_func_proto bpf_skb_output_proto;
> extern const struct bpf_func_proto bpf_xdp_output_proto;
> +extern const struct bpf_func_proto bpf_xdp_get_buff_len_trace_proto;
>
> BPF_CALL_3(bpf_get_stackid_raw_tp, struct bpf_raw_tracepoint_args *, args,
> struct bpf_map *, map, u64, flags)
> @@ -1460,6 +1461,8 @@ tracing_prog_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
> return &bpf_sock_from_file_proto;
> case BPF_FUNC_get_socket_cookie:
> return &bpf_get_socket_ptr_cookie_proto;
> + case BPF_FUNC_xdp_get_buff_len:
> + return &bpf_xdp_get_buff_len_trace_proto;
> #endif
> case BPF_FUNC_seq_printf:
> return prog->expected_attach_type == BPF_TRACE_ITER ?
> diff --git a/net/core/filter.c b/net/core/filter.c
> index b0855f2d4726..f7211b7908a9 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -3939,6 +3939,15 @@ const struct bpf_func_proto bpf_xdp_get_buff_len_proto = {
> .arg1_type = ARG_PTR_TO_CTX,
> };
>
> +BTF_ID_LIST_SINGLE(bpf_xdp_get_buff_len_bpf_ids, struct, xdp_buff)
> +
> +const struct bpf_func_proto bpf_xdp_get_buff_len_trace_proto = {
> + .func = bpf_xdp_get_buff_len,
> + .gpl_only = false,
> + .arg1_type = ARG_PTR_TO_BTF_ID,
> + .arg1_btf_id = &bpf_xdp_get_buff_len_bpf_ids[0],
> +};
> +
> BPF_CALL_2(bpf_xdp_adjust_tail, struct xdp_buff *, xdp, int, offset)
> {
> void *data_hard_end = xdp_data_hard_end(xdp); /* use xdp->frame_sz */
> @@ -4606,10 +4615,56 @@ static const struct bpf_func_proto bpf_sk_ancestor_cgroup_id_proto = {
> };
> #endif
>
> -static unsigned long bpf_xdp_copy(void *dst_buff, const void *src_buff,
> +static unsigned long bpf_xdp_copy(void *dst_buff, const void *ctx,
> unsigned long off, unsigned long len)
> {
> - memcpy(dst_buff, src_buff + off, len);
> + struct xdp_buff *xdp = (struct xdp_buff *)ctx;
> + struct skb_shared_info *sinfo;
> + unsigned long base_len;
> +
> + if (likely(!xdp_buff_is_mb(xdp))) {
> + memcpy(dst_buff, xdp->data + off, len);
> + return 0;
> + }
> +
> + base_len = xdp->data_end - xdp->data;
> + sinfo = xdp_get_shared_info_from_buff(xdp);
> + do {
> + const void *src_buff = NULL;
> + unsigned long copy_len = 0;
> +
> + if (off < base_len) {
> + src_buff = xdp->data + off;
> + copy_len = min(len, base_len - off);
> + } else {
> + unsigned long frag_off_total = base_len;
> + int i;
> +
> + for (i = 0; i < sinfo->nr_frags; i++) {
> + skb_frag_t *frag = &sinfo->frags[i];
> + unsigned long frag_len, frag_off;
> +
> + frag_len = skb_frag_size(frag);
> + frag_off = off - frag_off_total;
> + if (frag_off < frag_len) {
> + src_buff = skb_frag_address(frag) +
> + frag_off;
> + copy_len = min(len,
> + frag_len - frag_off);
> + break;
> + }
> + frag_off_total += frag_len;
> + }
> + }
> + if (!src_buff)
> + break;
> +
> + memcpy(dst_buff, src_buff, copy_len);
> + off += copy_len;
> + len -= copy_len;
> + dst_buff += copy_len;
This block reads odd to be because it requires looping over the frags
multiple times? Why not something like this,
if (off < base_len) {
src_buff = xdp->data + off
copy_len = min...
memcpy(dst_buff, src_buff, copy_len)
off += copylen
len -= copylen
dst_buff += copylen;
}
for (i = 0; i , nr_frags; i++) {
frag = ...
...
if frag_off < fraglen
...
memcpy()
update(off, len, dst_buff)
}
Maybe use a helper to set off,len and dst_buff if worried about the
duplication. Seems cleaner than walking through 0..n-1 frags for
each copy.
> + } while (len);
> +
> return 0;
> }
