[PATCH net-next v2] net, neigh: introduce interval_probe_time for periodic probe

[PATCH net-next v2] net, neigh: introduce interval_probe_time for periodic probe

[Yuwei Wang]

commit 7482e3841d52 ("net, neigh: Add NTF_MANAGED flag for managed neighbor entries")
neighbor entries which with NTF_EXT_MANAGED flags will periodically call neigh_event_send()
for performing the resolution. and the interval was set to DELAY_PROBE_TIME

DELAY_PROBE_TIME was configured as the first probe time delay, and it makes sense to set it to `0`.

when DELAY_PROBE_TIME is `0`, the queue_delayed_work of neighbor entries with NTF_EXT_MANAGED will
be called recursively with no interval, and then threads of `system_power_efficient_wq` will consume 100% cpu. 

as commit messages mentioned in the above commit, we should introduce a new option which means resolution interval.

Signed-off-by: Yuwei Wang <wangyuweihx@gmail.com>
---
v2: 
- move `NDTPA_INTERVAL_PROBE_TIME` to the tail of uAPI enum
- add `NDTPA_INTERVAL_PROBE_TIME` to `nl_ntbl_parm_policy`
- add detail explain for the behevior when `DELAY_PROBE_TIME` is `0` in
  commit messaage

meanwhile, we should replace `DELAY_PROBE_TIME` with `INTERVAL_PROBE_TIME` 
in `drivers/net/ethernet/mellanox` after this patch was merged

and should we remove `include/uapi/linux/sysctl.h` seems it is no
longer be used.

 include/net/neighbour.h        |  3 ++-
 include/net/netevent.h         |  1 +
 include/uapi/linux/neighbour.h |  1 +
 include/uapi/linux/sysctl.h    | 37 +++++++++++++++++-----------------
 net/core/neighbour.c           | 15 ++++++++++++--
 net/decnet/dn_neigh.c          |  1 +
 net/ipv4/arp.c                 |  1 +
 net/ipv6/ndisc.c               |  1 +
 8 files changed, 39 insertions(+), 21 deletions(-)

diff --git a/include/net/neighbour.h b/include/net/neighbour.h
index 87419f7f5421..75786903f1d4 100644
--- a/include/net/neighbour.h
+++ b/include/net/neighbour.h
@@ -48,6 +48,7 @@ enum {
 	NEIGH_VAR_RETRANS_TIME,
 	NEIGH_VAR_BASE_REACHABLE_TIME,
 	NEIGH_VAR_DELAY_PROBE_TIME,
+	NEIGH_VAR_INTERVAL_PROBE_TIME,
 	NEIGH_VAR_GC_STALETIME,
 	NEIGH_VAR_QUEUE_LEN_BYTES,
 	NEIGH_VAR_PROXY_QLEN,
@@ -64,7 +65,7 @@ enum {
 	NEIGH_VAR_GC_THRESH1,
 	NEIGH_VAR_GC_THRESH2,
 	NEIGH_VAR_GC_THRESH3,
-	NEIGH_VAR_MAX
+	NEIGH_VAR_MAX,
 };
 
 struct neigh_parms {
diff --git a/include/net/netevent.h b/include/net/netevent.h
index 4107016c3bb4..121df77d653e 100644
--- a/include/net/netevent.h
+++ b/include/net/netevent.h
@@ -26,6 +26,7 @@ enum netevent_notif_type {
 	NETEVENT_NEIGH_UPDATE = 1, /* arg is struct neighbour ptr */
 	NETEVENT_REDIRECT,	   /* arg is struct netevent_redirect ptr */
 	NETEVENT_DELAY_PROBE_TIME_UPDATE, /* arg is struct neigh_parms ptr */
+	NETEVENT_INTERVAL_PROBE_TIME_UPDATE, /* arg is struct neigh_parms ptr */
 	NETEVENT_IPV4_MPATH_HASH_UPDATE, /* arg is struct net ptr */
 	NETEVENT_IPV6_MPATH_HASH_UPDATE, /* arg is struct net ptr */
 	NETEVENT_IPV4_FWD_UPDATE_PRIORITY_UPDATE, /* arg is struct net ptr */
diff --git a/include/uapi/linux/neighbour.h b/include/uapi/linux/neighbour.h
index 39c565e460c7..8713c3ea81b2 100644
--- a/include/uapi/linux/neighbour.h
+++ b/include/uapi/linux/neighbour.h
@@ -154,6 +154,7 @@ enum {
 	NDTPA_QUEUE_LENBYTES,		/* u32 */
 	NDTPA_MCAST_REPROBES,		/* u32 */
 	NDTPA_PAD,
+	NDTPA_INTERVAL_PROBE_TIME,	/* u64, msecs */
 	__NDTPA_MAX
 };
 #define NDTPA_MAX (__NDTPA_MAX - 1)
diff --git a/include/uapi/linux/sysctl.h b/include/uapi/linux/sysctl.h
index 6a3b194c50fe..53f06bfd2a37 100644
--- a/include/uapi/linux/sysctl.h
+++ b/include/uapi/linux/sysctl.h
@@ -584,24 +584,25 @@ enum {
 
 /* /proc/sys/net/<protocol>/neigh/<dev> */
 enum {
-	NET_NEIGH_MCAST_SOLICIT=1,
-	NET_NEIGH_UCAST_SOLICIT=2,
-	NET_NEIGH_APP_SOLICIT=3,
-	NET_NEIGH_RETRANS_TIME=4,
-	NET_NEIGH_REACHABLE_TIME=5,
-	NET_NEIGH_DELAY_PROBE_TIME=6,
-	NET_NEIGH_GC_STALE_TIME=7,
-	NET_NEIGH_UNRES_QLEN=8,
-	NET_NEIGH_PROXY_QLEN=9,
-	NET_NEIGH_ANYCAST_DELAY=10,
-	NET_NEIGH_PROXY_DELAY=11,
-	NET_NEIGH_LOCKTIME=12,
-	NET_NEIGH_GC_INTERVAL=13,
-	NET_NEIGH_GC_THRESH1=14,
-	NET_NEIGH_GC_THRESH2=15,
-	NET_NEIGH_GC_THRESH3=16,
-	NET_NEIGH_RETRANS_TIME_MS=17,
-	NET_NEIGH_REACHABLE_TIME_MS=18,
+	NET_NEIGH_MCAST_SOLICIT = 1,
+	NET_NEIGH_UCAST_SOLICIT = 2,
+	NET_NEIGH_APP_SOLICIT = 3,
+	NET_NEIGH_RETRANS_TIME = 4,
+	NET_NEIGH_REACHABLE_TIME = 5,
+	NET_NEIGH_DELAY_PROBE_TIME = 6,
+	NET_NEIGH_GC_STALE_TIME = 7,
+	NET_NEIGH_UNRES_QLEN = 8,
+	NET_NEIGH_PROXY_QLEN = 9,
+	NET_NEIGH_ANYCAST_DELAY = 10,
+	NET_NEIGH_PROXY_DELAY = 11,
+	NET_NEIGH_LOCKTIME = 12,
+	NET_NEIGH_GC_INTERVAL = 13,
+	NET_NEIGH_GC_THRESH1 = 14,
+	NET_NEIGH_GC_THRESH2 = 15,
+	NET_NEIGH_GC_THRESH3 = 16,
+	NET_NEIGH_RETRANS_TIME_MS = 17,
+	NET_NEIGH_REACHABLE_TIME_MS = 18,
+	NET_NEIGH_INTERVAL_PROBE_TIME = 19,
 };
 
 /* /proc/sys/net/dccp */
diff --git a/net/core/neighbour.c b/net/core/neighbour.c
index 47b6c1f0fdbb..92447f04cf07 100644
--- a/net/core/neighbour.c
+++ b/net/core/neighbour.c
@@ -1579,7 +1579,7 @@ static void neigh_managed_work(struct work_struct *work)
 	list_for_each_entry(neigh, &tbl->managed_list, managed_list)
 		neigh_event_send_probe(neigh, NULL, false);
 	queue_delayed_work(system_power_efficient_wq, &tbl->managed_work,
-			   NEIGH_VAR(&tbl->parms, DELAY_PROBE_TIME));
+			   NEIGH_VAR(&tbl->parms, INTERVAL_PROBE_TIME));
 	write_unlock_bh(&tbl->lock);
 }
 
@@ -2100,7 +2100,9 @@ static int neightbl_fill_parms(struct sk_buff *skb, struct neigh_parms *parms)
 	    nla_put_msecs(skb, NDTPA_PROXY_DELAY,
 			  NEIGH_VAR(parms, PROXY_DELAY), NDTPA_PAD) ||
 	    nla_put_msecs(skb, NDTPA_LOCKTIME,
-			  NEIGH_VAR(parms, LOCKTIME), NDTPA_PAD))
+			  NEIGH_VAR(parms, LOCKTIME), NDTPA_PAD) ||
+	    nla_put_msecs(skb, NDTPA_INTERVAL_PROBE_TIME,
+			  NEIGH_VAR(parms, INTERVAL_PROBE_TIME), NDTPA_PAD))
 		goto nla_put_failure;
 	return nla_nest_end(skb, nest);
 
@@ -2255,6 +2257,7 @@ static const struct nla_policy nl_ntbl_parm_policy[NDTPA_MAX+1] = {
 	[NDTPA_ANYCAST_DELAY]		= { .type = NLA_U64 },
 	[NDTPA_PROXY_DELAY]		= { .type = NLA_U64 },
 	[NDTPA_LOCKTIME]		= { .type = NLA_U64 },
+	[NDTPA_INTERVAL_PROBE_TIME]	= { .type = NLA_U64 },
 };
 
 static int neightbl_set(struct sk_buff *skb, struct nlmsghdr *nlh,
@@ -2373,6 +2376,11 @@ static int neightbl_set(struct sk_buff *skb, struct nlmsghdr *nlh,
 					      nla_get_msecs(tbp[i]));
 				call_netevent_notifiers(NETEVENT_DELAY_PROBE_TIME_UPDATE, p);
 				break;
+			case NDTPA_INTERVAL_PROBE_TIME:
+				NEIGH_VAR_SET(p, INTERVAL_PROBE_TIME,
+					      nla_get_msecs(tbp[i]));
+				call_netevent_notifiers(NETEVENT_INTERVAL_PROBE_TIME_UPDATE, p);
+				break;
 			case NDTPA_RETRANS_TIME:
 				NEIGH_VAR_SET(p, RETRANS_TIME,
 					      nla_get_msecs(tbp[i]));
@@ -3543,6 +3551,8 @@ static void neigh_proc_update(struct ctl_table *ctl, int write)
 	set_bit(index, p->data_state);
 	if (index == NEIGH_VAR_DELAY_PROBE_TIME)
 		call_netevent_notifiers(NETEVENT_DELAY_PROBE_TIME_UPDATE, p);
+	if (index == NEIGH_VAR_INTERVAL_PROBE_TIME)
+		call_netevent_notifiers(NETEVENT_INTERVAL_PROBE_TIME_UPDATE, p);
 	if (!dev) /* NULL dev means this is default value */
 		neigh_copy_dflt_parms(net, p, index);
 }
@@ -3676,6 +3686,7 @@ static struct neigh_sysctl_table {
 		NEIGH_SYSCTL_USERHZ_JIFFIES_ENTRY(RETRANS_TIME, "retrans_time"),
 		NEIGH_SYSCTL_JIFFIES_ENTRY(BASE_REACHABLE_TIME, "base_reachable_time"),
 		NEIGH_SYSCTL_JIFFIES_ENTRY(DELAY_PROBE_TIME, "delay_first_probe_time"),
+		NEIGH_SYSCTL_JIFFIES_ENTRY(INTERVAL_PROBE_TIME, "interval_probe_time"),
 		NEIGH_SYSCTL_JIFFIES_ENTRY(GC_STALETIME, "gc_stale_time"),
 		NEIGH_SYSCTL_ZERO_INTMAX_ENTRY(QUEUE_LEN_BYTES, "unres_qlen_bytes"),
 		NEIGH_SYSCTL_ZERO_INTMAX_ENTRY(PROXY_QLEN, "proxy_qlen"),
diff --git a/net/decnet/dn_neigh.c b/net/decnet/dn_neigh.c
index fbd98ac853ea..995b22841ebf 100644
--- a/net/decnet/dn_neigh.c
+++ b/net/decnet/dn_neigh.c
@@ -94,6 +94,7 @@ struct neigh_table dn_neigh_table = {
 			[NEIGH_VAR_RETRANS_TIME] = 1 * HZ,
 			[NEIGH_VAR_BASE_REACHABLE_TIME] = 30 * HZ,
 			[NEIGH_VAR_DELAY_PROBE_TIME] = 5 * HZ,
+			[NEIGH_VAR_INTERVAL_PROBE_TIME] = 5 * HZ,
 			[NEIGH_VAR_GC_STALETIME] = 60 * HZ,
 			[NEIGH_VAR_QUEUE_LEN_BYTES] = SK_WMEM_MAX,
 			[NEIGH_VAR_PROXY_QLEN] = 0,
diff --git a/net/ipv4/arp.c b/net/ipv4/arp.c
index ab4a5601c82a..dbea1f7a7e2b 100644
--- a/net/ipv4/arp.c
+++ b/net/ipv4/arp.c
@@ -168,6 +168,7 @@ struct neigh_table arp_tbl = {
 			[NEIGH_VAR_RETRANS_TIME] = 1 * HZ,
 			[NEIGH_VAR_BASE_REACHABLE_TIME] = 30 * HZ,
 			[NEIGH_VAR_DELAY_PROBE_TIME] = 5 * HZ,
+			[NEIGH_VAR_INTERVAL_PROBE_TIME] = 5 * HZ,
 			[NEIGH_VAR_GC_STALETIME] = 60 * HZ,
 			[NEIGH_VAR_QUEUE_LEN_BYTES] = SK_WMEM_MAX,
 			[NEIGH_VAR_PROXY_QLEN] = 64,
diff --git a/net/ipv6/ndisc.c b/net/ipv6/ndisc.c
index 254addad0dd3..283b0a188c0e 100644
--- a/net/ipv6/ndisc.c
+++ b/net/ipv6/ndisc.c
@@ -128,6 +128,7 @@ struct neigh_table nd_tbl = {
 			[NEIGH_VAR_RETRANS_TIME] = ND_RETRANS_TIMER,
 			[NEIGH_VAR_BASE_REACHABLE_TIME] = ND_REACHABLE_TIME,
 			[NEIGH_VAR_DELAY_PROBE_TIME] = 5 * HZ,
+			[NEIGH_VAR_INTERVAL_PROBE_TIME] = 5 * HZ,
 			[NEIGH_VAR_GC_STALETIME] = 60 * HZ,
 			[NEIGH_VAR_QUEUE_LEN_BYTES] = SK_WMEM_MAX,
 			[NEIGH_VAR_PROXY_QLEN] = 64,

base-commit: aa5334b1f96801cd09775217a72ff252ef614d7a
-- 
2.34.1

Re: [PATCH net-next v2] net, neigh: introduce interval_probe_time for periodic probe

[Paolo Abeni]

On Sun, 2022-05-22 at 03:17 +0000, Yuwei Wang wrote:
> commit 7482e3841d52 ("net, neigh: Add NTF_MANAGED flag for managed neighbor entries")
> neighbor entries which with NTF_EXT_MANAGED flags will periodically call neigh_event_send()
> for performing the resolution. and the interval was set to DELAY_PROBE_TIME
> 
> DELAY_PROBE_TIME was configured as the first probe time delay, and it makes sense to set it to `0`.
> 
> when DELAY_PROBE_TIME is `0`, the queue_delayed_work of neighbor entries with NTF_EXT_MANAGED will
> be called recursively with no interval, and then threads of `system_power_efficient_wq` will consume 100% cpu. 
> 
> as commit messages mentioned in the above commit, we should introduce a new option which means resolution interval.
> 
> Signed-off-by: Yuwei Wang <wangyuweihx@gmail.com>
> ---
> v2: 
> - move `NDTPA_INTERVAL_PROBE_TIME` to the tail of uAPI enum
> - add `NDTPA_INTERVAL_PROBE_TIME` to `nl_ntbl_parm_policy`
> - add detail explain for the behevior when `DELAY_PROBE_TIME` is `0` in
>   commit messaage
> 
> meanwhile, we should replace `DELAY_PROBE_TIME` with `INTERVAL_PROBE_TIME` 
> in `drivers/net/ethernet/mellanox` after this patch was merged
> 
> and should we remove `include/uapi/linux/sysctl.h` seems it is no
> longer be used.
> 
>  include/net/neighbour.h        |  3 ++-
>  include/net/netevent.h         |  1 +
>  include/uapi/linux/neighbour.h |  1 +
>  include/uapi/linux/sysctl.h    | 37 +++++++++++++++++-----------------
>  net/core/neighbour.c           | 15 ++++++++++++--
>  net/decnet/dn_neigh.c          |  1 +
>  net/ipv4/arp.c                 |  1 +
>  net/ipv6/ndisc.c               |  1 +
>  8 files changed, 39 insertions(+), 21 deletions(-)
> 
> diff --git a/include/net/neighbour.h b/include/net/neighbour.h
> index 87419f7f5421..75786903f1d4 100644
> --- a/include/net/neighbour.h
> +++ b/include/net/neighbour.h
> @@ -48,6 +48,7 @@ enum {
>  	NEIGH_VAR_RETRANS_TIME,
>  	NEIGH_VAR_BASE_REACHABLE_TIME,
>  	NEIGH_VAR_DELAY_PROBE_TIME,
> +	NEIGH_VAR_INTERVAL_PROBE_TIME,
>  	NEIGH_VAR_GC_STALETIME,
>  	NEIGH_VAR_QUEUE_LEN_BYTES,
>  	NEIGH_VAR_PROXY_QLEN,
> @@ -64,7 +65,7 @@ enum {
>  	NEIGH_VAR_GC_THRESH1,
>  	NEIGH_VAR_GC_THRESH2,
>  	NEIGH_VAR_GC_THRESH3,
> -	NEIGH_VAR_MAX
> +	NEIGH_VAR_MAX,

You should avoid style-only changes in area not touched otherwise by
this 

>  };
>  
>  struct neigh_parms {
> diff --git a/include/net/netevent.h b/include/net/netevent.h
> index 4107016c3bb4..121df77d653e 100644
> --- a/include/net/netevent.h
> +++ b/include/net/netevent.h
> @@ -26,6 +26,7 @@ enum netevent_notif_type {
>  	NETEVENT_NEIGH_UPDATE = 1, /* arg is struct neighbour ptr */
>  	NETEVENT_REDIRECT,	   /* arg is struct netevent_redirect ptr */
>  	NETEVENT_DELAY_PROBE_TIME_UPDATE, /* arg is struct neigh_parms ptr */
> +	NETEVENT_INTERVAL_PROBE_TIME_UPDATE, /* arg is struct neigh_parms ptr */

Are you sure we need to notify the drivers about this parameter change?
The host will periodically resolve the neighbours, and that should work
regardless of the NIC offload. I think we don't need additional
notifications.

>  	NETEVENT_IPV4_MPATH_HASH_UPDATE, /* arg is struct net ptr */
>  	NETEVENT_IPV6_MPATH_HASH_UPDATE, /* arg is struct net ptr */
>  	NETEVENT_IPV4_FWD_UPDATE_PRIORITY_UPDATE, /* arg is struct net ptr */
> diff --git a/include/uapi/linux/neighbour.h b/include/uapi/linux/neighbour.h
> index 39c565e460c7..8713c3ea81b2 100644
> --- a/include/uapi/linux/neighbour.h
> +++ b/include/uapi/linux/neighbour.h
> @@ -154,6 +154,7 @@ enum {
>  	NDTPA_QUEUE_LENBYTES,		/* u32 */
>  	NDTPA_MCAST_REPROBES,		/* u32 */
>  	NDTPA_PAD,
> +	NDTPA_INTERVAL_PROBE_TIME,	/* u64, msecs */
>  	__NDTPA_MAX
>  };
>  #define NDTPA_MAX (__NDTPA_MAX - 1)
> diff --git a/include/uapi/linux/sysctl.h b/include/uapi/linux/sysctl.h
> index 6a3b194c50fe..53f06bfd2a37 100644
> --- a/include/uapi/linux/sysctl.h
> +++ b/include/uapi/linux/sysctl.h
> @@ -584,24 +584,25 @@ enum {
>  
>  /* /proc/sys/net/<protocol>/neigh/<dev> */
>  enum {
> -	NET_NEIGH_MCAST_SOLICIT=1,
> -	NET_NEIGH_UCAST_SOLICIT=2,
> -	NET_NEIGH_APP_SOLICIT=3,
> -	NET_NEIGH_RETRANS_TIME=4,
> -	NET_NEIGH_REACHABLE_TIME=5,
> -	NET_NEIGH_DELAY_PROBE_TIME=6,
> -	NET_NEIGH_GC_STALE_TIME=7,
> -	NET_NEIGH_UNRES_QLEN=8,
> -	NET_NEIGH_PROXY_QLEN=9,
> -	NET_NEIGH_ANYCAST_DELAY=10,
> -	NET_NEIGH_PROXY_DELAY=11,
> -	NET_NEIGH_LOCKTIME=12,
> -	NET_NEIGH_GC_INTERVAL=13,
> -	NET_NEIGH_GC_THRESH1=14,
> -	NET_NEIGH_GC_THRESH2=15,
> -	NET_NEIGH_GC_THRESH3=16,
> -	NET_NEIGH_RETRANS_TIME_MS=17,
> -	NET_NEIGH_REACHABLE_TIME_MS=18,
> +	NET_NEIGH_MCAST_SOLICIT = 1,
> +	NET_NEIGH_UCAST_SOLICIT = 2,
> +	NET_NEIGH_APP_SOLICIT = 3,
> +	NET_NEIGH_RETRANS_TIME = 4,
> +	NET_NEIGH_REACHABLE_TIME = 5,
> +	NET_NEIGH_DELAY_PROBE_TIME = 6,
> +	NET_NEIGH_GC_STALE_TIME = 7,
> +	NET_NEIGH_UNRES_QLEN = 8,
> +	NET_NEIGH_PROXY_QLEN = 9,
> +	NET_NEIGH_ANYCAST_DELAY = 10,
> +	NET_NEIGH_PROXY_DELAY = 11,
> +	NET_NEIGH_LOCKTIME = 12,
> +	NET_NEIGH_GC_INTERVAL = 13,
> +	NET_NEIGH_GC_THRESH1 = 14,
> +	NET_NEIGH_GC_THRESH2 = 15,
> +	NET_NEIGH_GC_THRESH3 = 16,
> +	NET_NEIGH_RETRANS_TIME_MS = 17,
> +	NET_NEIGH_REACHABLE_TIME_MS = 18,
> +	NET_NEIGH_INTERVAL_PROBE_TIME = 19,
>  };
>  
>  /* /proc/sys/net/dccp */
> diff --git a/net/core/neighbour.c b/net/core/neighbour.c
> index 47b6c1f0fdbb..92447f04cf07 100644
> --- a/net/core/neighbour.c
> +++ b/net/core/neighbour.c
> @@ -1579,7 +1579,7 @@ static void neigh_managed_work(struct work_struct *work)
>  	list_for_each_entry(neigh, &tbl->managed_list, managed_list)
>  		neigh_event_send_probe(neigh, NULL, false);
>  	queue_delayed_work(system_power_efficient_wq, &tbl->managed_work,
> -			   NEIGH_VAR(&tbl->parms, DELAY_PROBE_TIME));
> +			   NEIGH_VAR(&tbl->parms, INTERVAL_PROBE_TIME));
>  	write_unlock_bh(&tbl->lock);
>  }
>  
> @@ -2100,7 +2100,9 @@ static int neightbl_fill_parms(struct sk_buff *skb, struct neigh_parms *parms)
>  	    nla_put_msecs(skb, NDTPA_PROXY_DELAY,
>  			  NEIGH_VAR(parms, PROXY_DELAY), NDTPA_PAD) ||
>  	    nla_put_msecs(skb, NDTPA_LOCKTIME,
> -			  NEIGH_VAR(parms, LOCKTIME), NDTPA_PAD))
> +			  NEIGH_VAR(parms, LOCKTIME), NDTPA_PAD) ||
> +	    nla_put_msecs(skb, NDTPA_INTERVAL_PROBE_TIME,
> +			  NEIGH_VAR(parms, INTERVAL_PROBE_TIME), NDTPA_PAD))
>  		goto nla_put_failure;
>  	return nla_nest_end(skb, nest);
>  
> @@ -2255,6 +2257,7 @@ static const struct nla_policy nl_ntbl_parm_policy[NDTPA_MAX+1] = {
>  	[NDTPA_ANYCAST_DELAY]		= { .type = NLA_U64 },
>  	[NDTPA_PROXY_DELAY]		= { .type = NLA_U64 },
>  	[NDTPA_LOCKTIME]		= { .type = NLA_U64 },
> +	[NDTPA_INTERVAL_PROBE_TIME]	= { .type = NLA_U64 },

since a 0 value would not make any sense here and will cause problems,
what about adding '.min = 1' ?


Thanks!

Paolo

Re: [PATCH net-next v2] net, neigh: introduce interval_probe_time for periodic probe

[Yuwei Wang]

On Tue, 24 May 2022 at 16:38, Paolo Abeni <pabeni@redhat.com> wrote:
>
> On Sun, 2022-05-22 at 03:17 +0000, Yuwei Wang wrote:
> > commit 7482e3841d52 ("net, neigh: Add NTF_MANAGED flag for managed neighbor entries")
> > neighbor entries which with NTF_EXT_MANAGED flags will periodically call neigh_event_send()
> > for performing the resolution. and the interval was set to DELAY_PROBE_TIME
> >
> > DELAY_PROBE_TIME was configured as the first probe time delay, and it makes sense to set it to `0`.
> >
> > when DELAY_PROBE_TIME is `0`, the queue_delayed_work of neighbor entries with NTF_EXT_MANAGED will
> > be called recursively with no interval, and then threads of `system_power_efficient_wq` will consume 100% cpu.
> >
> > as commit messages mentioned in the above commit, we should introduce a new option which means resolution interval.
> >
> > Signed-off-by: Yuwei Wang <wangyuweihx@gmail.com>
> > ---
> > v2:
> > - move `NDTPA_INTERVAL_PROBE_TIME` to the tail of uAPI enum
> > - add `NDTPA_INTERVAL_PROBE_TIME` to `nl_ntbl_parm_policy`
> > - add detail explain for the behevior when `DELAY_PROBE_TIME` is `0` in
> >   commit messaage
> >
> > meanwhile, we should replace `DELAY_PROBE_TIME` with `INTERVAL_PROBE_TIME`
> > in `drivers/net/ethernet/mellanox` after this patch was merged
> >
> > and should we remove `include/uapi/linux/sysctl.h` seems it is no
> > longer be used.
> >
> >  include/net/neighbour.h        |  3 ++-
> >  include/net/netevent.h         |  1 +
> >  include/uapi/linux/neighbour.h |  1 +
> >  include/uapi/linux/sysctl.h    | 37 +++++++++++++++++-----------------
> >  net/core/neighbour.c           | 15 ++++++++++++--
> >  net/decnet/dn_neigh.c          |  1 +
> >  net/ipv4/arp.c                 |  1 +
> >  net/ipv6/ndisc.c               |  1 +
> >  8 files changed, 39 insertions(+), 21 deletions(-)
> >
> > diff --git a/include/net/neighbour.h b/include/net/neighbour.h
> > index 87419f7f5421..75786903f1d4 100644
> > --- a/include/net/neighbour.h
> > +++ b/include/net/neighbour.h
> > @@ -48,6 +48,7 @@ enum {
> >       NEIGH_VAR_RETRANS_TIME,
> >       NEIGH_VAR_BASE_REACHABLE_TIME,
> >       NEIGH_VAR_DELAY_PROBE_TIME,
> > +     NEIGH_VAR_INTERVAL_PROBE_TIME,
> >       NEIGH_VAR_GC_STALETIME,
> >       NEIGH_VAR_QUEUE_LEN_BYTES,
> >       NEIGH_VAR_PROXY_QLEN,
> > @@ -64,7 +65,7 @@ enum {
> >       NEIGH_VAR_GC_THRESH1,
> >       NEIGH_VAR_GC_THRESH2,
> >       NEIGH_VAR_GC_THRESH3,
> > -     NEIGH_VAR_MAX
> > +     NEIGH_VAR_MAX,
>
> You should avoid style-only changes in area not touched otherwise by
> this

OK, I will redo this change in the next patch version.

>
> >  };
> >
> >  struct neigh_parms {
> > diff --git a/include/net/netevent.h b/include/net/netevent.h
> > index 4107016c3bb4..121df77d653e 100644
> > --- a/include/net/netevent.h
> > +++ b/include/net/netevent.h
> > @@ -26,6 +26,7 @@ enum netevent_notif_type {
> >       NETEVENT_NEIGH_UPDATE = 1, /* arg is struct neighbour ptr */
> >       NETEVENT_REDIRECT,         /* arg is struct netevent_redirect ptr */
> >       NETEVENT_DELAY_PROBE_TIME_UPDATE, /* arg is struct neigh_parms ptr */
> > +     NETEVENT_INTERVAL_PROBE_TIME_UPDATE, /* arg is struct neigh_parms ptr */
>
> Are you sure we need to notify the drivers about this parameter change?
> The host will periodically resolve the neighbours, and that should work
> regardless of the NIC offload. I think we don't need additional
> notifications.
>

`mlxsw_sp_router_netevent_event` in
drivers/net/ethernet/mellanox/mlx5/core/en/rep/neigh.c and
`mlx5e_rep_netevent_event` in
drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c still
use `NETEVENT_DELAY_PROBE_TIME_UPDATE` to receive the update event of
`DELAY_PROBE_TIME` as the probe interval.

I think we are supposed to replace `NETEVENT_DELAY_PROBE_TIME_UPDATE` with
`NETEVENT_INTERVAL_PROBE_TIME_UPDATE` after this patch is merged.

> >       NETEVENT_IPV4_MPATH_HASH_UPDATE, /* arg is struct net ptr */
> >       NETEVENT_IPV6_MPATH_HASH_UPDATE, /* arg is struct net ptr */
> >       NETEVENT_IPV4_FWD_UPDATE_PRIORITY_UPDATE, /* arg is struct net ptr */
> > diff --git a/include/uapi/linux/neighbour.h b/include/uapi/linux/neighbour.h
> > index 39c565e460c7..8713c3ea81b2 100644
> > --- a/include/uapi/linux/neighbour.h
> > +++ b/include/uapi/linux/neighbour.h
> > @@ -154,6 +154,7 @@ enum {
> >       NDTPA_QUEUE_LENBYTES,           /* u32 */
> >       NDTPA_MCAST_REPROBES,           /* u32 */
> >       NDTPA_PAD,
> > +     NDTPA_INTERVAL_PROBE_TIME,      /* u64, msecs */
> >       __NDTPA_MAX
> >  };
> >  #define NDTPA_MAX (__NDTPA_MAX - 1)
> > diff --git a/net/core/neighbour.c b/net/core/neighbour.c
> > index 47b6c1f0fdbb..92447f04cf07 100644
> > --- a/net/core/neighbour.c
> > +++ b/net/core/neighbour.c
> > @@ -1579,7 +1579,7 @@ static void neigh_managed_work(struct work_struct *work)
> >       list_for_each_entry(neigh, &tbl->managed_list, managed_list)
> >               neigh_event_send_probe(neigh, NULL, false);
> >       queue_delayed_work(system_power_efficient_wq, &tbl->managed_work,
> > -                        NEIGH_VAR(&tbl->parms, DELAY_PROBE_TIME));
> > +                        NEIGH_VAR(&tbl->parms, INTERVAL_PROBE_TIME));
> >       write_unlock_bh(&tbl->lock);
> >  }
> >
> > @@ -2100,7 +2100,9 @@ static int neightbl_fill_parms(struct sk_buff *skb, struct neigh_parms *parms)
> >           nla_put_msecs(skb, NDTPA_PROXY_DELAY,
> >                         NEIGH_VAR(parms, PROXY_DELAY), NDTPA_PAD) ||
> >           nla_put_msecs(skb, NDTPA_LOCKTIME,
> > -                       NEIGH_VAR(parms, LOCKTIME), NDTPA_PAD))
> > +                       NEIGH_VAR(parms, LOCKTIME), NDTPA_PAD) ||
> > +         nla_put_msecs(skb, NDTPA_INTERVAL_PROBE_TIME,
> > +                       NEIGH_VAR(parms, INTERVAL_PROBE_TIME), NDTPA_PAD))
> >               goto nla_put_failure;
> >       return nla_nest_end(skb, nest);
> >
> > @@ -2255,6 +2257,7 @@ static const struct nla_policy nl_ntbl_parm_policy[NDTPA_MAX+1] = {
> >       [NDTPA_ANYCAST_DELAY]           = { .type = NLA_U64 },
> >       [NDTPA_PROXY_DELAY]             = { .type = NLA_U64 },
> >       [NDTPA_LOCKTIME]                = { .type = NLA_U64 },
> > +     [NDTPA_INTERVAL_PROBE_TIME]     = { .type = NLA_U64 },
>
> since a 0 value would not make any sense here and will cause problems,
> what about adding '.min = 1' ?

Thanks, good point! I will update this in the next patch version.

>
>
> Thanks!
>
> Paolo
>

Thanks!

Yuwei Wang

Re: [PATCH net-next v2] net, neigh: introduce interval_probe_time for periodic probe

[Paolo Abeni]

On Tue, 2022-05-24 at 17:38 +0800, Yuwei Wang wrote:
> On Tue, 24 May 2022 at 16:38, Paolo Abeni <pabeni@redhat.com> wrote:
> > 
> > On Sun, 2022-05-22 at 03:17 +0000, Yuwei Wang wrote:
> > 
> > > diff --git a/include/net/netevent.h b/include/net/netevent.h
> > > index 4107016c3bb4..121df77d653e 100644
> > > --- a/include/net/netevent.h
> > > +++ b/include/net/netevent.h
> > > @@ -26,6 +26,7 @@ enum netevent_notif_type {
> > >       NETEVENT_NEIGH_UPDATE = 1, /* arg is struct neighbour ptr */
> > >       NETEVENT_REDIRECT,         /* arg is struct netevent_redirect ptr */
> > >       NETEVENT_DELAY_PROBE_TIME_UPDATE, /* arg is struct neigh_parms ptr */
> > > +     NETEVENT_INTERVAL_PROBE_TIME_UPDATE, /* arg is struct neigh_parms ptr */
> > 
> > Are you sure we need to notify the drivers about this parameter change?
> > The host will periodically resolve the neighbours, and that should work
> > regardless of the NIC offload. I think we don't need additional
> > notifications.
> > 
> 
> `mlxsw_sp_router_netevent_event` in
> drivers/net/ethernet/mellanox/mlx5/core/en/rep/neigh.c and
> `mlx5e_rep_netevent_event` in
> drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c still
> use `NETEVENT_DELAY_PROBE_TIME_UPDATE` to receive the update event of
> `DELAY_PROBE_TIME` as the probe interval.
> 
> I think we are supposed to replace `NETEVENT_DELAY_PROBE_TIME_UPDATE` with
> `NETEVENT_INTERVAL_PROBE_TIME_UPDATE` after this patch is merged.

AFAICS the event notification is to let neigh_timer_handler() cope
properly with NIC offloading the data plane.

In such scenario packets (forwarded by the NIC) don't reach the host,
and neigh->confirmed can be untouched for a long time fooling
neigh_timer_handler() into a timeout.

The event notification allows the NIC to perform the correct actions to
avoid such timeout.

In case of MANAGED neighbour, the host is periodically sending probe
request, and both req/replies should not be offloaded. AFAICS no action
is expected from the NIC to cope with INTERVAL_PROBE_TIME changes.

Cheers,

Paolo

Re: [PATCH net-next v2] net, neigh: introduce interval_probe_time for periodic probe

[Yuwei Wang]

On Tue, 24 May 2022 at 18:41, Paolo Abeni <pabeni@redhat.com> wrote:
>
> On Tue, 2022-05-24 at 17:38 +0800, Yuwei Wang wrote:
> > On Tue, 24 May 2022 at 16:38, Paolo Abeni <pabeni@redhat.com> wrote:
> > >
> > > On Sun, 2022-05-22 at 03:17 +0000, Yuwei Wang wrote:
> > >
> > > > diff --git a/include/net/netevent.h b/include/net/netevent.h
> > > > index 4107016c3bb4..121df77d653e 100644
> > > > --- a/include/net/netevent.h
> > > > +++ b/include/net/netevent.h
> > > > @@ -26,6 +26,7 @@ enum netevent_notif_type {
> > > >       NETEVENT_NEIGH_UPDATE = 1, /* arg is struct neighbour ptr */
> > > >       NETEVENT_REDIRECT,         /* arg is struct netevent_redirect ptr */
> > > >       NETEVENT_DELAY_PROBE_TIME_UPDATE, /* arg is struct neigh_parms ptr */
> > > > +     NETEVENT_INTERVAL_PROBE_TIME_UPDATE, /* arg is struct neigh_parms ptr */
> > >
> > > Are you sure we need to notify the drivers about this parameter change?
> > > The host will periodically resolve the neighbours, and that should work
> > > regardless of the NIC offload. I think we don't need additional
> > > notifications.
> > >
> >
> > `mlxsw_sp_router_netevent_event` in
> > drivers/net/ethernet/mellanox/mlx5/core/en/rep/neigh.c and
> > `mlx5e_rep_netevent_event` in
> > drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c still
> > use `NETEVENT_DELAY_PROBE_TIME_UPDATE` to receive the update event of
> > `DELAY_PROBE_TIME` as the probe interval.
> >
> > I think we are supposed to replace `NETEVENT_DELAY_PROBE_TIME_UPDATE` with
> > `NETEVENT_INTERVAL_PROBE_TIME_UPDATE` after this patch is merged.
>
> AFAICS the event notification is to let neigh_timer_handler() cope
> properly with NIC offloading the data plane.
>
> In such scenario packets (forwarded by the NIC) don't reach the host,
> and neigh->confirmed can be untouched for a long time fooling
> neigh_timer_handler() into a timeout.
>
> The event notification allows the NIC to perform the correct actions to
> avoid such timeout.
>
> In case of MANAGED neighbour, the host is periodically sending probe
> request, and both req/replies should not be offloaded. AFAICS no action
> is expected from the NIC to cope with INTERVAL_PROBE_TIME changes.

I think `INTERVAL_PROBE_TIME` is not only for MANAGED neighbour,
if the driver needs periodically poll the device for neighbours activity,
we also should use  `INTERVAL_PROBE_TIME` rather than `DELAY_PROBE_TIME` as
the polling interval.

but as
Link: https://lore.kernel.org/netdev/20211011121238.25542-5-daniel@iogearbox.net/

> mlxsw which has similar driver-internal infrastructure
> c723c735fa6b ("mlxsw: spectrum_router: Periodically update the kernel's neigh table").
> In future, the latter could possibly reuse the NTF_MANAGED neighbors as well.

It seems that the behavior probe periodically by the driver will be
deprecated and replaced by setting the MANAGED flag.
or we can keep using `DELAY_PROBE_TIME` before it is replaced?

Look forward to your reply.

Yuwei Wang

Re: [PATCH net-next v2] net, neigh: introduce interval_probe_time for periodic probe

[Daniel Borkmann]

On 5/24/22 12:41 PM, Paolo Abeni wrote:
> On Tue, 2022-05-24 at 17:38 +0800, Yuwei Wang wrote:
>> On Tue, 24 May 2022 at 16:38, Paolo Abeni <pabeni@redhat.com> wrote:
>>>
>>> On Sun, 2022-05-22 at 03:17 +0000, Yuwei Wang wrote:
>>>
>>>> diff --git a/include/net/netevent.h b/include/net/netevent.h
>>>> index 4107016c3bb4..121df77d653e 100644
>>>> --- a/include/net/netevent.h
>>>> +++ b/include/net/netevent.h
>>>> @@ -26,6 +26,7 @@ enum netevent_notif_type {
>>>>        NETEVENT_NEIGH_UPDATE = 1, /* arg is struct neighbour ptr */
>>>>        NETEVENT_REDIRECT,         /* arg is struct netevent_redirect ptr */
>>>>        NETEVENT_DELAY_PROBE_TIME_UPDATE, /* arg is struct neigh_parms ptr */
>>>> +     NETEVENT_INTERVAL_PROBE_TIME_UPDATE, /* arg is struct neigh_parms ptr */
>>>
>>> Are you sure we need to notify the drivers about this parameter change?
>>> The host will periodically resolve the neighbours, and that should work
>>> regardless of the NIC offload. I think we don't need additional
>>> notifications.
>>>
>>
>> `mlxsw_sp_router_netevent_event` in
>> drivers/net/ethernet/mellanox/mlx5/core/en/rep/neigh.c and
>> `mlx5e_rep_netevent_event` in
>> drivers/net/ethernet/mellanox/mlxsw/spectrum_router.c still
>> use `NETEVENT_DELAY_PROBE_TIME_UPDATE` to receive the update event of
>> `DELAY_PROBE_TIME` as the probe interval.
>>
>> I think we are supposed to replace `NETEVENT_DELAY_PROBE_TIME_UPDATE` with
>> `NETEVENT_INTERVAL_PROBE_TIME_UPDATE` after this patch is merged.
> 
> AFAICS the event notification is to let neigh_timer_handler() cope
> properly with NIC offloading the data plane.
> 
> In such scenario packets (forwarded by the NIC) don't reach the host,
> and neigh->confirmed can be untouched for a long time fooling
> neigh_timer_handler() into a timeout.
> 
> The event notification allows the NIC to perform the correct actions to
> avoid such timeout.
> 
> In case of MANAGED neighbour, the host is periodically sending probe
> request, and both req/replies should not be offloaded. AFAICS no action
> is expected from the NIC to cope with INTERVAL_PROBE_TIME changes.

Right, maybe we could just split this into two: 1) prevent misconfig (see
below), and 2) make the timeout configurable as what Yuwei has. Wdyt?

diff --git a/net/core/neighbour.c b/net/core/neighbour.c
index 47b6c1f0fdbb..54625287ee5b 100644
--- a/net/core/neighbour.c
+++ b/net/core/neighbour.c
@@ -1579,7 +1579,7 @@ static void neigh_managed_work(struct work_struct *work)
         list_for_each_entry(neigh, &tbl->managed_list, managed_list)
                 neigh_event_send_probe(neigh, NULL, false);
         queue_delayed_work(system_power_efficient_wq, &tbl->managed_work,
-                          NEIGH_VAR(&tbl->parms, DELAY_PROBE_TIME));
+                          max(NEIGH_VAR(&tbl->parms, DELAY_PROBE_TIME), HZ));
         write_unlock_bh(&tbl->lock);
  }

Thanks,
Daniel

Re: [PATCH net-next v2] net, neigh: introduce interval_probe_time for periodic probe

[Jakub Kicinski]

On Tue, 24 May 2022 17:32:57 +0200 Daniel Borkmann wrote:
> Right, maybe we could just split this into two: 1) prevent misconfig (see
> below), and 2) make the timeout configurable as what Yuwei has. Wdyt?
> 
> diff --git a/net/core/neighbour.c b/net/core/neighbour.c
> index 47b6c1f0fdbb..54625287ee5b 100644
> --- a/net/core/neighbour.c
> +++ b/net/core/neighbour.c
> @@ -1579,7 +1579,7 @@ static void neigh_managed_work(struct work_struct *work)
>          list_for_each_entry(neigh, &tbl->managed_list, managed_list)
>                  neigh_event_send_probe(neigh, NULL, false);
>          queue_delayed_work(system_power_efficient_wq, &tbl->managed_work,
> -                          NEIGH_VAR(&tbl->parms, DELAY_PROBE_TIME));
> +                          max(NEIGH_VAR(&tbl->parms, DELAY_PROBE_TIME), HZ));
>          write_unlock_bh(&tbl->lock);
>   }

FWIW that was my reaction as well. Let's do that unless someone
disagrees.

Re: [PATCH net-next v2] net, neigh: introduce interval_probe_time for periodic probe

[Yuwei Wang]

On Wed, 25 May 2022 at 02:07, Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Tue, 24 May 2022 17:32:57 +0200 Daniel Borkmann wrote:
> > Right, maybe we could just split this into two: 1) prevent misconfig (see
> > below), and 2) make the timeout configurable as what Yuwei has. Wdyt?
> >
> > diff --git a/net/core/neighbour.c b/net/core/neighbour.c
> > index 47b6c1f0fdbb..54625287ee5b 100644
> > --- a/net/core/neighbour.c
> > +++ b/net/core/neighbour.c
> > @@ -1579,7 +1579,7 @@ static void neigh_managed_work(struct work_struct *work)
> >          list_for_each_entry(neigh, &tbl->managed_list, managed_list)
> >                  neigh_event_send_probe(neigh, NULL, false);
> >          queue_delayed_work(system_power_efficient_wq, &tbl->managed_work,
> > -                          NEIGH_VAR(&tbl->parms, DELAY_PROBE_TIME));
> > +                          max(NEIGH_VAR(&tbl->parms, DELAY_PROBE_TIME), HZ));
> >          write_unlock_bh(&tbl->lock);
> >   }
>
> FWIW that was my reaction as well. Let's do that unless someone
> disagrees.

I agree too, so there will be as following parts:
1) prevent misconfig by offering a minimum value
2) separate the params `INTERVAL_PROBE_TIME` as the probe interval for
`MANAGED` neigh
3) notify the change of `INTERVAL_PROBE_TIME` and set the driver poll interval
according to `INTERVAL_PROBE_TIME` instead of `DELAY_PROBE_TIME`

I still have doubt about whether we need part 3, or if exist this scenario:
- the NIC offloading the data plane.
- the driver needs periodically poll the device for neighbours activity

May I ask for further explanation?

Thanks,
Yuwei Wang

Re: [PATCH net-next v2] net, neigh: introduce interval_probe_time for periodic probe

[Daniel Borkmann]

On 5/24/22 9:13 PM, Yuwei Wang wrote:
> On Wed, 25 May 2022 at 02:07, Jakub Kicinski <kuba@kernel.org> wrote:
>> On Tue, 24 May 2022 17:32:57 +0200 Daniel Borkmann wrote:
>>> Right, maybe we could just split this into two: 1) prevent misconfig (see
>>> below), and 2) make the timeout configurable as what Yuwei has. Wdyt?
>>>
>>> diff --git a/net/core/neighbour.c b/net/core/neighbour.c
>>> index 47b6c1f0fdbb..54625287ee5b 100644
>>> --- a/net/core/neighbour.c
>>> +++ b/net/core/neighbour.c
>>> @@ -1579,7 +1579,7 @@ static void neigh_managed_work(struct work_struct *work)
>>>           list_for_each_entry(neigh, &tbl->managed_list, managed_list)
>>>                   neigh_event_send_probe(neigh, NULL, false);
>>>           queue_delayed_work(system_power_efficient_wq, &tbl->managed_work,
>>> -                          NEIGH_VAR(&tbl->parms, DELAY_PROBE_TIME));
>>> +                          max(NEIGH_VAR(&tbl->parms, DELAY_PROBE_TIME), HZ));
>>>           write_unlock_bh(&tbl->lock);
>>>    }
>>
>> FWIW that was my reaction as well. Let's do that unless someone
>> disagrees.
> 
> I agree too, so there will be as following parts:
> 1) prevent misconfig by offering a minimum value
> 2) separate the params `INTERVAL_PROBE_TIME` as the probe interval for
> `MANAGED` neigh

Ok.

> 3) notify the change of `INTERVAL_PROBE_TIME` and set the driver poll interval
> according to `INTERVAL_PROBE_TIME` instead of `DELAY_PROBE_TIME`
> 
> I still have doubt about whether we need part 3, or if exist this scenario:
> - the NIC offloading the data plane.
> - the driver needs periodically poll the device for neighbours activity
> 
> May I ask for further explanation?

Well, but for that case we would need some in-tree driver users of managed neigh
first, and they can probably drive it according to their needs if they require a
notifier message. Are you saying you are panning to convert the mlx one over to
use managed neigh?

Thanks,
Daniel

Re: [PATCH net-next v2] net, neigh: introduce interval_probe_time for periodic probe

[Daniel Borkmann]

Hey Yuwei,

On 5/24/22 9:13 PM, Yuwei Wang wrote:
[...]
> I agree too, so there will be as following parts:
> 1) prevent misconfig by offering a minimum value
> 2) separate the params `INTERVAL_PROBE_TIME` as the probe interval for
> `MANAGED` neigh

Given net-next is open, could you rebase and send a v3 of your probe time knob?

Thanks,
Daniel