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

[David Ahern <dsahern@gmail.com>]

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.


>  	__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.


your change seems all over the board here.

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.