diff mbox series

net, neigh: introduce interval_probe_time for periodic probe

Message ID 20220520055104.1528845-1-wangyuweihx@gmail.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series net, neigh: introduce interval_probe_time for periodic probe | expand

Checks

Context Check Description
netdev/fixes_present success Fixes tag not required for -next series
netdev/subject_prefix warning Target tree name not specified in the subject
netdev/cover_letter success Single patches do not need cover letters
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 17949 this patch: 17949
netdev/cc_maintainers warning 6 maintainers not CCed: keescook@chromium.org razor@blackwall.org yoshfuji@linux-ipv6.org pabeni@redhat.com prestwoj@gmail.com linux-decnet-user@lists.sourceforge.net
netdev/build_clang success Errors and warnings before: 3363 this patch: 3363
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 17190 this patch: 17190
netdev/checkpatch warning WARNING: line length of 87 exceeds 80 columns WARNING: line length of 96 exceeds 80 columns
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
netdev/tree_selection success Guessing tree name failed - patch did not apply

Commit Message

Yuwei Wang May 20, 2022, 5:51 a.m. UTC 
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 resolution of neighbor entries with NTF_EXT_MANAGED will
trap in an infinity recursion.

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

 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           | 14 +++++++++++--
 net/decnet/dn_neigh.c          |  1 +
 net/ipv4/arp.c                 |  1 +
 net/ipv6/ndisc.c               |  1 +
 8 files changed, 38 insertions(+), 21 deletions(-)


base-commit: 0784c25d21cfe14c128ea5ed3c9ab843fdfac737

Comments

Jakub Kicinski May 21, 2022, 12:56 a.m. UTC | #1 
On Fri, 20 May 2022 05:51:04 +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 resolution of neighbor entries with NTF_EXT_MANAGED will
> trap in an infinity recursion.

Recursion or will constantly get re-resolved?

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

> diff --git a/include/uapi/linux/neighbour.h b/include/uapi/linux/neighbour.h
> index 39c565e460c7..5ae538be64b9 100644
> --- a/include/uapi/linux/neighbour.h
> +++ b/include/uapi/linux/neighbour.h
> @@ -143,6 +143,7 @@ enum {
>  	NDTPA_RETRANS_TIME,		/* u64, msecs */
>  	NDTPA_GC_STALETIME,		/* u64, msecs */
>  	NDTPA_DELAY_PROBE_TIME,		/* u64, msecs */
> +	NDTPA_INTERVAL_PROBE_TIME,	/* u64, msecs */
>  	NDTPA_QUEUE_LEN,		/* u32 */
>  	NDTPA_APP_PROBES,		/* u32 */
>  	NDTPA_UCAST_PROBES,		/* u32 */

You can't insert values in the middle of a uAPI enum,
you'll break binary compatibility with older kernels.
Yuwei Wang May 21, 2022, 6:12 p.m. UTC | #2 
On Sat, May 21, 2022 at 8:56 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Fri, 20 May 2022 05:51:04 +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 resolution of neighbor entries with NTF_EXT_MANAGED will
> > trap in an infinity recursion.
>
> Recursion or will constantly get re-resolved?

Recursive call `neigh_event_send_probe` with no interval,
which means threads of `system_power_efficient_wq` will consume 100% cpu.
Re-resolved or not depend on `neigh->nud_state`, if `neigh->nud_state`
is NUD_REACHABLE,
`neigh_event_send_probe` will return immediately with no action
>
> > 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>
>
> > diff --git a/include/uapi/linux/neighbour.h b/include/uapi/linux/neighbour.h
> > index 39c565e460c7..5ae538be64b9 100644
> > --- a/include/uapi/linux/neighbour.h
> > +++ b/include/uapi/linux/neighbour.h
> > @@ -143,6 +143,7 @@ enum {
> >       NDTPA_RETRANS_TIME,             /* u64, msecs */
> >       NDTPA_GC_STALETIME,             /* u64, msecs */
> >       NDTPA_DELAY_PROBE_TIME,         /* u64, msecs */
> > +     NDTPA_INTERVAL_PROBE_TIME,      /* u64, msecs */
> >       NDTPA_QUEUE_LEN,                /* u32 */
> >       NDTPA_APP_PROBES,               /* u32 */
> >       NDTPA_UCAST_PROBES,             /* u32 */
>
> You can't insert values in the middle of a uAPI enum,
> you'll break binary compatibility with older kernels.

Thanks, I will move it to the tail in patch v2
diff mbox series

Patch

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..5ae538be64b9 100644
--- a/include/uapi/linux/neighbour.h
+++ b/include/uapi/linux/neighbour.h
@@ -143,6 +143,7 @@  enum {
 	NDTPA_RETRANS_TIME,		/* u64, msecs */
 	NDTPA_GC_STALETIME,		/* u64, msecs */
 	NDTPA_DELAY_PROBE_TIME,		/* u64, msecs */
+	NDTPA_INTERVAL_PROBE_TIME,	/* u64, msecs */
 	NDTPA_QUEUE_LEN,		/* u32 */
 	NDTPA_APP_PROBES,		/* u32 */
 	NDTPA_UCAST_PROBES,		/* u32 */
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..f07cac60c834 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);
 
@@ -2373,6 +2375,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 +3550,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 +3685,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,