Message-ID: <CAC1LvL0_DhsStjzHhRY_JrCVeBW0J6M2CDJ6qT77Do-deXq8Zg@mail.gmail.com>
Date:   Thu, 24 Jun 2021 08:16:46 -0700
From:   Zvi Effron <zeffron@...tgames.com>
To:     John Fastabend <john.fastabend@...il.com>
Cc:     Eelco Chaudron <echaudro@...hat.com>,
        Lorenzo Bianconi <lorenzo@...nel.org>,
        bpf <bpf@...r.kernel.org>, netdev@...r.kernel.org,
        lorenzo.bianconi@...hat.com, David Miller <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>, shayagr@...zon.com,
        sameehj@...zon.com, dsahern@...nel.org,
        Jesper Dangaard Brouer <brouer@...hat.com>,
        jasowang@...hat.com, alexander.duyck@...il.com, saeed@...nel.org,
        Maciej Fijalkowski <maciej.fijalkowski@...el.com>,
        "Karlsson, Magnus" <magnus.karlsson@...el.com>,
        tirthendu.sarkar@...el.com
Subject: Re: [PATCH v9 bpf-next 08/14] bpf: add multi-buff support to the
 bpf_xdp_adjust_tail() API

On Thu, Jun 24, 2021 at 7:24 AM John Fastabend <john.fastabend@...il.com> wrote:
>
> Eelco Chaudron wrote:
> >
> >
> > On 23 Jun 2021, at 1:37, John Fastabend wrote:
> >
> > > Lorenzo Bianconi wrote:
> > >> From: Eelco Chaudron <echaudro@...hat.com>
> > >>
> > >> This change adds support for tail growing and shrinking for XDP multi-buff.
> > >>
> > >
> > > It would be nice if the commit message gave us some details on how the
> > > growing/shrinking works in the multi-buff support.
> >
> > Will add this to the next rev.
> >
> > >> Signed-off-by: Eelco Chaudron <echaudro@...hat.com>
> > >> Signed-off-by: Lorenzo Bianconi <lorenzo@...nel.org>
> > >> ---
> > >>  include/net/xdp.h |  7 ++++++
> > >>  net/core/filter.c | 62 +++++++++++++++++++++++++++++++++++++++++++++++
> > >>  net/core/xdp.c    |  5 ++--
> > >>  3 files changed, 72 insertions(+), 2 deletions(-)
> > >>
> > >> diff --git a/include/net/xdp.h b/include/net/xdp.h
> > >> index 935a6f83115f..3525801c6ed5 100644
> > >> --- a/include/net/xdp.h
> > >> +++ b/include/net/xdp.h
> > >> @@ -132,6 +132,11 @@ xdp_get_shared_info_from_buff(struct xdp_buff *xdp)
> > >>    return (struct skb_shared_info *)xdp_data_hard_end(xdp);
> > >>  }
> > >>
> > >> +static inline unsigned int xdp_get_frag_tailroom(const skb_frag_t *frag)
> > >> +{
> > >> +  return PAGE_SIZE - skb_frag_size(frag) - skb_frag_off(frag);
> > >> +}
> > >> +
> > >>  struct xdp_frame {
> > >>    void *data;
> > >>    u16 len;
> > >> @@ -259,6 +264,8 @@ struct xdp_frame *xdp_convert_buff_to_frame(struct xdp_buff *xdp)
> > >>    return xdp_frame;
> > >>  }
> > >>
> > >> +void __xdp_return(void *data, struct xdp_mem_info *mem, bool napi_direct,
> > >> +            struct xdp_buff *xdp);
> > >>  void xdp_return_frame(struct xdp_frame *xdpf);
> > >>  void xdp_return_frame_rx_napi(struct xdp_frame *xdpf);
> > >>  void xdp_return_buff(struct xdp_buff *xdp);
> > >> diff --git a/net/core/filter.c b/net/core/filter.c
> > >> index caa88955562e..05f574a3d690 100644
> > >> --- a/net/core/filter.c
> > >> +++ b/net/core/filter.c
> > >> @@ -3859,11 +3859,73 @@ static const struct bpf_func_proto bpf_xdp_adjust_head_proto = {
> > >>    .arg2_type      = ARG_ANYTHING,
> > >>  };
> > >>
> > >> +static int bpf_xdp_mb_adjust_tail(struct xdp_buff *xdp, int offset)
> > >> +{
> > >> +  struct skb_shared_info *sinfo;
> > >> +
> > >> +  if (unlikely(!xdp_buff_is_mb(xdp)))
> > >> +          return -EINVAL;
> > >> +
> > >> +  sinfo = xdp_get_shared_info_from_buff(xdp);
> > >> +  if (offset >= 0) {
> > >> +          skb_frag_t *frag = &sinfo->frags[sinfo->nr_frags - 1];
> > >> +          int size;
> > >> +
> > >> +          if (unlikely(offset > xdp_get_frag_tailroom(frag)))
> > >> +                  return -EINVAL;
> > >> +
> > >> +          size = skb_frag_size(frag);
> > >> +          memset(skb_frag_address(frag) + size, 0, offset);
> > >> +          skb_frag_size_set(frag, size + offset);
> > >> +          sinfo->data_len += offset;
> > >
> > > Can you add some comment on how this works? So today I call
> > > bpf_xdp_adjust_tail() to add some trailer to my packet.
> > > This looks like it adds tailroom to the last frag? But, then
> > > how do I insert my trailer? I don't think we can without the
> > > extra multi-buffer access support right.
> >
> > You are right, we need some kind of multi-buffer access helpers.
> >
> > > Also data_end will be unchanged yet it will return 0 so my
> > > current programs will likely be a bit confused by this.
> >
> > Guess this is the tricky part, applications need to be multi-buffer aware. If current applications rely on bpf_xdp_adjust_tail(+) to determine maximum frame length this approach might not work. In this case, we might need an additional helper to do tail expansion with multi buffer support.
> >
> > But then the question arrives how would mb unaware application behave in general when an mb packet is supplied?? It would definitely not determine the correct packet length.
>
> Right that was my conclusion as well. Existing programs might
> have subtle side effects if they start running on multibuffer
> drivers as is. I don't have any good ideas though on how
> to handle this.
>

Would it be possible to detect multibuffer awareness of a program at load
(or attach) time, perhaps by looking for the use of the new multibuffer
helpers? That might make it possible to reject a non-multibuffer aware
program on multibuffer drivers (or maybe even put the driver into a
non-multibuffer mode at attach time), or at the very least issue a
warning?

> >
> > >> +  } else {
> >
>
>

Powered by blists - more mailing lists