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

Hi David,

On Tue, 2021-10-19 at 20:15 -0600, David Ahern wrote:

On 10/18/21 1:26 PM, James Prestwood wrote:

quoted

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.

quoted

        __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