Message-ID: <20181015195805.7xob34egcs3pqvag@ast-mbp.dhcp.thefacebook.com>
Date:   Mon, 15 Oct 2018 12:58:07 -0700
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     Jakub Kicinski <jakub.kicinski@...ronome.com>
Cc:     daniel@...earbox.net, netdev@...r.kernel.org,
        oss-drivers@...ronome.com
Subject: Re: [PATCH bpf-next] tools: bpftool: add map create command

On Mon, Oct 15, 2018 at 09:49:08AM -0700, Jakub Kicinski wrote:
> On Fri, 12 Oct 2018 23:16:59 -0700, Alexei Starovoitov wrote:
> > On Fri, Oct 12, 2018 at 11:06:14AM -0700, Jakub Kicinski wrote:
> > > Add a way of creating maps from user space.  The command takes
> > > as parameters most of the attributes of the map creation system
> > > call command.  After map is created its pinned to bpffs.  This makes
> > > it possible to easily and dynamically (without rebuilding programs)
> > > test various corner cases related to map creation.
> > > 
> > > Map type names are taken from bpftool's array used for printing.
> > > In general these days we try to make use of libbpf type names, but
> > > there are no map type names in libbpf as of today.
> > > 
> > > As with most features I add the motivation is testing (offloads) :)
> > > 
> > > Signed-off-by: Jakub Kicinski <jakub.kicinski@...ronome.com>
> > > Reviewed-by: Quentin Monnet <quentin.monnet@...ronome.com>  
> > ...
> > >  	fprintf(stderr,
> > >  		"Usage: %s %s { show | list }   [MAP]\n"
> > > +		"       %s %s create     FILE type TYPE key KEY_SIZE value VALUE_SIZE \\\n"
> > > +		"                              entries MAX_ENTRIES [name NAME] [flags FLAGS] \\\n"
> > > +		"                              [dev NAME]\n"  
> > 
> > I suspect as soon as bpftool has an ability to create standalone maps
> > some folks will start relying on such interface.
> 
> That'd be cool, do you see any real life use cases where its useful
> outside of corner case testing?

In our XDP use case we have an odd protocol for different apps to share
common prog_array that is pinned in bpffs.
If cmdline creation of it via bpftool was available that would have been
an option to consider. Not saying that it would have been a better option.
Just another option.

> 
> > Therefore I'd like to request to make 'name' argument to be mandatory.
> 
> Will do in v2!

thx!
 
> > I think in the future we will require BTF to be mandatory too.
> > We need to move towards more transparent and debuggable infra.
> > Do you think requiring json description of key/value would be managable to implement?
> > Then bpftool could convert it to BTF and the map full be fully defined.
> > I certainly understand that bpf prog can disregard the key/value layout today,
> > but we will make verifier to enforce that in the future too.
> 
> I was hoping that we can leave BTF support as a future extension, and
> then once we have the option for the verifier to enforce BTF (a sysctl?)
> the bpftool map create without a BTF will get rejected as one would
> expect.  

right. something like sysctl in the future.

> IOW it's fine not to make BTF required at bpftool level and
> leave it to system configuration.
> 
> I'd love to implement the BTF support right away, but I'm not sure I
> can afford that right now time-wise.  The whole map create command is
> pretty trivial, but for BTF we don't even have a way of dumping it
> AFAICT.  We can pretty print values, but what is the format in which to
> express the BTF itself?  We could do JSON, do we use an external
> library?  Should we have a separate BTF command for that?

I prefer standard C type description for both input and output :)
Anyway that wasn't a request for you to do it now. More of the feature
request for somebody to put on todo list :)

Powered by blists - more mailing lists