Re: [PATCH nf-next RFC,v2 6/6] netfilter: nft_flow_offload: add ndo hooks for hardware offload

[Pablo Neira Ayuso <pablo@netfilter.org>]

On Fri, Dec 08, 2017 at 11:18:36AM +0100, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
[...]
> 
> > diff --git a/net/netfilter/nf_flow_table.c b/net/netfilter/nf_flow_table.c
> > index ff27dad268c3..c578c3aec0e0 100644
> > --- a/net/netfilter/nf_flow_table.c
> > +++ b/net/netfilter/nf_flow_table.c
> > @@ -212,6 +212,21 @@ int nf_flow_table_iterate(struct nf_flowtable *flow_table,
> >  }
> >  EXPORT_SYMBOL_GPL(nf_flow_table_iterate);
> >  
> > +static void flow_offload_hw_del(struct flow_offload *flow)
> > +{
> > +	struct net_device *indev;
> > +	int ret, ifindex;
> > +
> > +	rtnl_lock();
> > +	ifindex = flow->tuplehash[FLOW_OFFLOAD_DIR_ORIGINAL].tuple.iifidx;
> > +	indev = __dev_get_by_index(&init_net, ifindex);
> 
> I think this should pass struct net * as arg to flow_offload_hw_del.
>
> > +	if (WARN_ON(!indev))
> > +		return;
> > +
> > +	ret = indev->netdev_ops->ndo_flow_offload(FLOW_OFFLOAD_DEL, flow);
> > +	rtnl_unlock();
> > +}
> 
> Please no rtnl lock unless absolutely needed.
> Seems this could even avoid the mutex completely by using
> dev_get_by_index + dev_put.

OK, we still need to make sure that we additions and deletions from
hardware don't occur concurrently, but that we can probably do it with
another mutex.

> > +static int do_flow_offload(struct flow_offload *flow)
> > +{
> > +	struct net_device *indev;
> > +	int ret, ifindex;
> > +
> > +	rtnl_lock();
> > +	ifindex = flow->tuplehash[FLOW_OFFLOAD_DIR_ORIGINAL].tuple.iifidx;
> > +	indev = __dev_get_by_index(&init_net, ifindex);
> 
> likewise.
> 
> > +#define FLOW_HW_WORK_TIMEOUT	msecs_to_jiffies(100)
> > +
> > +static struct delayed_work nft_flow_offload_dwork;
> 
> I would go with struct work and no delay at all.

Will have a look into this, thanks!