Re: [PATCH bpf-next 6/9] bpf: offload: allow program and map sharing per-ASIC

[Jakub Kicinski <jakub.kicinski@netronome.com>]

On Mon, 16 Jul 2018 20:00:47 -0700, Alexei Starovoitov wrote:
> On Mon, Jul 16, 2018 at 07:37:20PM -0700, Jakub Kicinski wrote:
> > Create a higher-level entity to represent a device/ASIC to allow
> > programs and maps to be shared between device ports.  The extra
> > work is required to make sure we don't destroy BPF objects as
> > soon as the netdev for which they were loaded gets destroyed,
> > as other ports may still be using them.  When netdev goes away
> > all of its BPF objects will be moved to other netdevs of the
> > device, and only destroyed when last netdev is unregistered.
> > 
> > Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> > Reviewed-by: Quentin Monnet <quentin.monnet@netronome.com>  
> ..
> > -bool bpf_offload_dev_match(struct bpf_prog *prog, struct bpf_map *map)
> > +static bool __bpf_offload_dev_match(struct bpf_prog *prog,
> > +				    struct net_device *netdev)
> >  {
> > -	struct bpf_offloaded_map *offmap;
> > +	struct bpf_offload_netdev *ondev1, *ondev2;
> >  	struct bpf_prog_offload *offload;
> > -	bool ret;
> >  
> >  	if (!bpf_prog_is_dev_bound(prog->aux))
> >  		return false;
> > -	if (!bpf_map_is_dev_bound(map))
> > -		return bpf_map_offload_neutral(map);
> >  
> > -	down_read(&bpf_devs_lock);
> >  	offload = prog->aux->offload;
> > +	if (!offload)
> > +		return false;
> > +	if (offload->netdev == netdev)
> > +		return true;
> > +
> > +	ondev1 = bpf_offload_find_netdev(offload->netdev);
> > +	ondev2 = bpf_offload_find_netdev(netdev);
> > +
> > +	return ondev1 && ondev2 && ondev1->offdev == ondev2->offdev;
> > +}
> > +
> > +bool bpf_offload_dev_match(struct bpf_prog *prog, struct net_device *netdev)
> > +{
> > +	bool ret;
> > +
> > +	down_read(&bpf_devs_lock);
> > +	ret = __bpf_offload_dev_match(prog, netdev);
> > +	up_read(&bpf_devs_lock);
> > +
> > +	return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(bpf_offload_dev_match);
> > +
> > +bool bpf_offload_match(struct bpf_prog *prog, struct bpf_map *map)
> > +{
> > +	struct bpf_offloaded_map *offmap;
> > +	bool ret;
> > +
> > +	if (!bpf_map_is_dev_bound(map))
> > +		return bpf_map_offload_neutral(map);
> >  	offmap = map_to_offmap(map);
> >  
> > -	ret = offload && offload->netdev == offmap->netdev;
> > +	down_read(&bpf_devs_lock);
> > +	ret = __bpf_offload_dev_match(prog, offmap->netdev);
> >  	up_read(&bpf_devs_lock);
> >  
> >  	return ret;
> >  }
> >    
> ..
> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > index 9e2bf834f13a..2c5b923eef75 100644
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> > @@ -5054,7 +5054,7 @@ static int check_map_prog_compatibility(struct bpf_verifier_env *env,
> >  	}
> >  
> >  	if ((bpf_prog_is_dev_bound(prog->aux) || bpf_map_is_dev_bound(map)) &&
> > -	    !bpf_offload_dev_match(prog, map)) {
> > +	    !bpf_offload_match(prog, map)) {  
> 
> I'm confused with new names and renaming.
> May be split renaming into separate patch?
> Should new bpf_offload_match() be called bpf_offload_prog_map_match() ?
> or some other name?
> May be adding comments to these functions will make it clear...

It is messy.  The new functions to register/unregister ASIC are called
bpf_offload_dev_*, hence it seemed like a good idea to call the
function exported to the drivers bpf_offload_dev_match() (see patches 7
and 8) to keep the driver API consistent.  But then the old function
which is only used by the verivier has to be renamed.

I will use bpf_offload_prog_map_match() and split to a separate patch.