On Fri, Dec 08, 2017 at 11:18:36AM +0100, Florian Westphal wrote:
> Pablo Neira Ayuso <pa...@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!
