Re: [bpf-next,v3] samples: bpf: add max_pckt_size option at xdp_adjust_tail

["Daniel T. Lee" <danieltimlee@gmail.com>]

On Thu, Sep 19, 2019 at 3:00 AM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Wed, Sep 18, 2019 at 10:37 AM Daniel T. Lee <danieltimlee@gmail.com> wrote:
> >
> > On Tue, Sep 17, 2019 at 1:04 PM Andrii Nakryiko
> > <andrii.nakryiko@gmail.com> wrote:
> > >
> > > On Wed, Sep 11, 2019 at 2:33 PM Daniel T. Lee <danieltimlee@gmail.com> wrote:
> > > >
> > > > Currently, at xdp_adjust_tail_kern.c, MAX_PCKT_SIZE is limited
> > > > to 600. To make this size flexible, a new map 'pcktsz' is added.
> > > >
> > > > By updating new packet size to this map from the userland,
> > > > xdp_adjust_tail_kern.o will use this value as a new max_pckt_size.
> > > >
> > > > If no '-P <MAX_PCKT_SIZE>' option is used, the size of maximum packet
> > > > will be 600 as a default.
> > > >
> > > > Signed-off-by: Daniel T. Lee <danieltimlee@gmail.com>
> > > >
> > > > ---
> > > > Changes in v2:
> > > >     - Change the helper to fetch map from 'bpf_map__next' to
> > > >     'bpf_object__find_map_fd_by_name'.
> > > >
> > > >  samples/bpf/xdp_adjust_tail_kern.c | 23 +++++++++++++++++++----
> > > >  samples/bpf/xdp_adjust_tail_user.c | 28 ++++++++++++++++++++++------
> > > >  2 files changed, 41 insertions(+), 10 deletions(-)
> > > >
> > > > diff --git a/samples/bpf/xdp_adjust_tail_kern.c b/samples/bpf/xdp_adjust_tail_kern.c
> > > > index 411fdb21f8bc..d6d84ffe6a7a 100644
> > > > --- a/samples/bpf/xdp_adjust_tail_kern.c
> > > > +++ b/samples/bpf/xdp_adjust_tail_kern.c
> > > > @@ -25,6 +25,13 @@
> > > >  #define ICMP_TOOBIG_SIZE 98
> > > >  #define ICMP_TOOBIG_PAYLOAD_SIZE 92
> > > >
> > > > +struct bpf_map_def SEC("maps") pcktsz = {
> > > > +       .type = BPF_MAP_TYPE_ARRAY,
> > > > +       .key_size = sizeof(__u32),
> > > > +       .value_size = sizeof(__u32),
> > > > +       .max_entries = 1,
> > > > +};
> > > > +
> > >
> > > Hey Daniel,
> > >
> > > This looks like an ideal use case for global variables on BPF side. I
> > > think it's much cleaner and will make BPF side of things simpler.
> > > Would you mind giving global data a spin instead of adding this map?
> > >
> >
> > Sure thing!
> > But, I'm not sure there is global variables for BPF?
> > AFAIK, there aren't any support for global variables yet in BPF
> > program (_kern.c).
> >
> >     # when defining global variable at _kern.c
> >     libbpf: bpf: relocation: not yet supported relo for non-static
> > global '<var>' variable found in insns[39].code 0x18
>
> just what it says: use static global variable (also volatile to
> prevent compiler optimizations) :)
>
> static volatile __u32 pcktsz; /* this should work */
>

My apologies, but I'm not sure I'm following.
What you are saying is, should I define global variable to _kern,c
and access and modify this variable from _user.c?

For example,

<_kern.c>
static volatile __u32 pcktsz = 300;

<_user.c>
extern __u32 pcktsz;
// Later in code
pcktsz = 400;

Is this code means similar to what you've said?
AFAIK, 'static' keyword for global variable restricts scope to file itself,
so the 'accessing' and 'modifying' this variable from the <_user.c>
isn't available.

The reason why I've used bpf map for this 'pcktsz' option is,
I've wanted to run this kernel xdp program (xdp_adjust_tail_kern.o)
as it itself, not heavily controlled by user program (./xdp_adjust_tail).

When this 'pcktsz' option is implemented in bpf map, user can simply
modify 'map' to change this size. (such as bpftool prog map)

But when this variable comes to global data, it can't be changed
after the program gets loaded.

I really appreciate your time and effort for the review.
But I'm sorry that I seem to get it wrong.

Thanks,
Daniel

> >
> > By the way, thanks for the review.
> >
> > Thanks,
> > Daniel
> >
> >
> > > >  struct bpf_map_def SEC("maps") icmpcnt = {
> > > >         .type = BPF_MAP_TYPE_ARRAY,
> > > >         .key_size = sizeof(__u32),
> > > > @@ -64,7 +71,8 @@ static __always_inline void ipv4_csum(void *data_start, int data_size,
> > > >         *csum = csum_fold_helper(*csum);
> > > >  }
> > > >
> > >
> > > [...]