Re: [PATCH v4] net: neighbour: introduce EVICT_NOCARRIER table option

[James Prestwood <prestwoj@gmail.com>]

Hi David,

On Tue, 2021-10-19 at 20:15 -0600, David Ahern wrote:
> On 10/18/21 1:26 PM, James Prestwood wrote:
> > diff --git a/Documentation/networking/ip-sysctl.rst
> > b/Documentation/networking/ip-sysctl.rst
> > index 16b8bf72feaf..e2aced01905a 100644
> > --- a/Documentation/networking/ip-sysctl.rst
> > +++ b/Documentation/networking/ip-sysctl.rst
> > @@ -200,6 +200,15 @@ neigh/default/unres_qlen - INTEGER
> >  
> >         Default: 101
> >  
> > +neigh/default/evict_nocarrier - BOOLEAN
> > +       Clears the neighbor cache on NOCARRIER events. This option
> > is important
> > +       for wireless devices where the cache should not be cleared
> > when roaming
> > +       between access points on the same network. In most cases
> > this should
> > +       remain as the default (1).
> > +
> > +       - 1 - (default): Clear the neighbor cache on NOCARRIER
> > events
> > +       - 0 - Do not clear neighbor cache on NOCARRIER events
> > +
> >  mtu_expires - INTEGER
> >         Time, in seconds, that cached PMTU information is kept.
> >  
> > diff --git a/include/net/neighbour.h b/include/net/neighbour.h
> > index e8e48be66755..71b28f83c3d3 100644
> > --- a/include/net/neighbour.h
> > +++ b/include/net/neighbour.h
> > @@ -54,7 +54,8 @@ enum {
> >         NEIGH_VAR_ANYCAST_DELAY,
> >         NEIGH_VAR_PROXY_DELAY,
> >         NEIGH_VAR_LOCKTIME,
> > -#define NEIGH_VAR_DATA_MAX (NEIGH_VAR_LOCKTIME + 1)
> > +       NEIGH_VAR_EVICT_NOCARRIER,
> > +#define NEIGH_VAR_DATA_MAX (NEIGH_VAR_EVICT_NOCARRIER + 1)
> >         /* Following are used as a second way to access one of the
> > above */
> >         NEIGH_VAR_QUEUE_LEN, /* same data as
> > NEIGH_VAR_QUEUE_LEN_BYTES */
> >         NEIGH_VAR_RETRANS_TIME_MS, /* same data as
> > NEIGH_VAR_RETRANS_TIME */
> > diff --git a/include/uapi/linux/neighbour.h
> > b/include/uapi/linux/neighbour.h
> > index db05fb55055e..1dc125dd4f50 100644
> > --- a/include/uapi/linux/neighbour.h
> > +++ b/include/uapi/linux/neighbour.h
> > @@ -152,6 +152,7 @@ enum {
> >         NDTPA_QUEUE_LENBYTES,           /* u32 */
> >         NDTPA_MCAST_REPROBES,           /* u32 */
> >         NDTPA_PAD,
> > +       NDTPA_EVICT_NOCARRIER,          /* u8 */
> 
> you are proposing a sysctl not a neighbor table attribute. ie., you
> don't need this.

Can't there be both, as there are with several of the other attributes?
I very well could have done it wrong, but my intent was to make this
settable via sysctl and RTNL.

But going the per netdev route as you describe below, yes this won't be
needed.

> 
> 
> >         __NDTPA_MAX
> >  };
> >  #define NDTPA_MAX (__NDTPA_MAX - 1)
> > diff --git a/net/core/neighbour.c b/net/core/neighbour.c
> > index 47931c8be04b..953253f3e491 100644
> > --- a/net/core/neighbour.c
> > +++ b/net/core/neighbour.c
> > @@ -318,7 +318,7 @@ static void pneigh_queue_purge(struct
> > sk_buff_head *list)
> >  }
> >  
> >  static void neigh_flush_dev(struct neigh_table *tbl, struct
> > net_device *dev,
> > -                           bool skip_perm)
> > +                           bool nocarrier)
> 
> why are you dropping the skip_perm arg? These are orthogonal skip
> options.

I felt it was more appropriate to describe the reason/event (no
carrier) that triggered the flush after adding this option. Rather than
using something called 'skip_perm' to skip non-perminant entries.

> 
> 
> your change seems all over the board here.

If it wasn't obvious this subsystem is totally new to me :)

> 
> You should be adding a per netdevice sysctl to allow this setting to
> be
> controlled per device, not a global setting or a table setting.
> 
> In neigh_carrier_down, check the sysctl for this device (and check
> 'all'
> device too) and if eviction on carrier down is not wanted, then skip
> the
> call to __neigh_ifdown.

Sure, this seems like a good path forward. I was not a fan of needing
to go into the neighbor iteration loop since we want to skip everything
for a given netdev anyways.

Thanks for the feedback

- James