Skip to contentSkip to conversation

neighbour.c: Avoid GC directly after state change

PATCH In review Ulf Samuelsson 4 participants 7 messages Mar 11, 2015 – Mar 17, 2015

Subsystems (2)
Networking Network drivers
lore
Share & tools
lore

Ulf Samuelsson

author
#0 lore raw

PATCH 1/1 neighbour.c: Avoid GC directly after state change

From: Ulf Samuelsson <[email protected]>

The neighbour state is changed in the ARP timer handler. If the state is changed to NUD_STALE, then the neighbour entry becomes a candidate for garbage collection.

The garbage collection is handled by a "periodic work" routine.

When :

* noone refers to the entry
* the state is no longer valid (I.E: NUD_STALE).
* the timeout value has been reached or state is FAILED

the "periodic work" routine will notify
the stack that the entry should be deleted.

A user application monitoring and controlling the neighbour table using NETLINK may fail, if the "period work" routine is run directly after the state has been changed to NUD_STALE, but before the user application has had a chance to change the state to something valid.

The "period work" routine will detect the NUD_STALE state and if the timeout value has been reached, it will notify the stack that the entry should be deleted.

The patch adds a check in the periodic work routine which will skip test for garbage collection unless a number of ticks has passed since the last time the neighbour entry state was changed.

The feature is controlled through Kconfig

The featuree is enabled by setting ARP_GC_APPLY_GUARDBAND The guardband time (in ticks) is set in ARP_GC_GUARDBAND Default time is 100 ms if HZ_### is set.

Signed-off-by Ulf Samuelsson <[email protected]>
---
 net/Kconfig          |   32 ++++++++++++++++++++++++++++++++
 net/core/neighbour.c |   15 ++++++++++++---
 2 files changed, 44 insertions(+), 3 deletions(-)
net/Kconfig +32 -0
index 44dd578..099a5dd 100644
@@ -77,6 +77,38 @@ config INET
 	  Short answer: say Y.
 
 if INET
+
+#
+# Core Network configuration
+#
+
+config ARP_GC_APPLY_GUARDBAND
+	bool "IP: ARP: Avoid garbage collection directly after state change"
+	default n
+	---help---
+	  With this item selected, an entry in the neighbour table
+	  will not be garbage collected directly after the ARP state
+	  has changed to STALE of FAILED
+	  This allows an application program change the state to something valid
+	  before garbage colllection occurs.
+
+	  If unsure, say N.
+
+config ARP_GC_GUARDBAND
+	int "Guardband time on garbage collection"
+	depends on ARP_GC_APPLY_GUARDBAND
+	default 10 if HZ_100
+	default 25 if HZ_250
+	default 30 if HZ_300
+	default 100 if HZ_1000
+	default 100
+
+	---help---
+	  The number of ticks to delay garbage collection
+	  after the neighbour entry has been updated
+	  A delay of 100 ms is reasonable.
+	  With CONFIG_HZ = 250, this value should be 25
+
 source "net/ipv4/Kconfig"
 source "net/ipv6/Kconfig"
 source "net/netlabel/Kconfig"
net/core/neighbour.c +12 -3
index 70fe9e1..194195d 100644
@@ -786,13 +786,23 @@ static void neigh_periodic_work(struct work_struct *work)
 
 			state = n->nud_state;
 			if (state & (NUD_PERMANENT | NUD_IN_TIMER)) {
-				write_unlock(&n->lock);
 				goto next_elt;
 			}
 
 			if (time_before(n->used, n->confirmed))
 				n->used = n->confirmed;
 
+#if defined(CONFIG_ARP_GC_APPLY_GUARDBAND)
+			/* Do not garbage collect directly after we
+			 * updated n->state to allow applications to
+			 * react to the event
+			 */
+			if (time_before(jiffies,
+					n->updated + CONFIG_ARP_GC_GUARDBAND)) {
+				goto next_elt;
+			}
+#endif
+
 			if (atomic_read(&n->refcnt) == 1 &&
 			    (state == NUD_FAILED ||
 			     time_after(jiffies, n->used + NEIGH_VAR(n->parms, GC_STALETIME)))) {

@@ -802,9 +812,8 @@ static void neigh_periodic_work(struct work_struct *work)
 				neigh_cleanup_and_release(n);
 				continue;
 			}
-			write_unlock(&n->lock);
-
 next_elt:
+			write_unlock(&n->lock);
 			np = &n->next;
 		}
 		/*
Signature · 1 line
1.7.9.5
#1 lore raw

Hello.

Quoted 6 lines · Ulf Samuelsson

From: Ulf Samuelsson <[email protected]>

The neighbour state is changed in the ARP timer handler. If the state is changed to NUD_STALE, then the neighbour entry becomes a candidate for garbage collection.

Quoted 6 lines

The garbage collection is handled by a "periodic work" routine.

When :

* noone refers to the entry
* the state is no longer valid (I.E: NUD_STALE).

NUD_STALE is still valid.

Quoted 20 lines

* the timeout value has been reached or state is FAILED

the "periodic work" routine will notify
the stack that the entry should be deleted.

A user application monitoring and controlling the neighbour table using NETLINK may fail, if the "period work" routine is run directly after the state has been changed to NUD_STALE, but before the user application has had a chance to change the state to something valid.

The "period work" routine will detect the NUD_STALE state and if the timeout value has been reached, it will notify the stack that the entry should be deleted.

The patch adds a check in the periodic work routine which will skip test for garbage collection unless a number of ticks has passed since the last time the neighbour entry state was changed.

Quoted 5 lines

The feature is controlled through Kconfig

The featuree is enabled by setting ARP_GC_APPLY_GUARDBAND The guardband time (in ticks) is set in ARP_GC_GUARDBAND Default time is 100 ms if HZ_### is set.

We have "lower limit" not to start releasing neighbour entries. Try increasing gc_thresh1.

--yoshfuji

Quoted 91 lines
Signed-off-by Ulf Samuelsson <[email protected]>
---
 net/Kconfig          |   32 ++++++++++++++++++++++++++++++++
 net/core/neighbour.c |   15 ++++++++++++---
 2 files changed, 44 insertions(+), 3 deletions(-)
net/Kconfig +32 -0
index 44dd578..099a5dd 100644
@@ -77,6 +77,38 @@ config INET
 	  Short answer: say Y.
 
 if INET
+
+#
+# Core Network configuration
+#
+
+config ARP_GC_APPLY_GUARDBAND
+	bool "IP: ARP: Avoid garbage collection directly after state change"
+	default n
+	---help---
+	  With this item selected, an entry in the neighbour table
+	  will not be garbage collected directly after the ARP state
+	  has changed to STALE of FAILED
+	  This allows an application program change the state to something valid
+	  before garbage colllection occurs.
+
+	  If unsure, say N.
+
+config ARP_GC_GUARDBAND
+	int "Guardband time on garbage collection"
+	depends on ARP_GC_APPLY_GUARDBAND
+	default 10 if HZ_100
+	default 25 if HZ_250
+	default 30 if HZ_300
+	default 100 if HZ_1000
+	default 100
+
+	---help---
+	  The number of ticks to delay garbage collection
+	  after the neighbour entry has been updated
+	  A delay of 100 ms is reasonable.
+	  With CONFIG_HZ = 250, this value should be 25
+
 source "net/ipv4/Kconfig"
 source "net/ipv6/Kconfig"
 source "net/netlabel/Kconfig"
net/core/neighbour.c +12 -3
index 70fe9e1..194195d 100644
@@ -786,13 +786,23 @@ static void neigh_periodic_work(struct work_struct *work)
 
 			state = n->nud_state;
 			if (state & (NUD_PERMANENT | NUD_IN_TIMER)) {
-				write_unlock(&n->lock);
 				goto next_elt;
 			}
 
 			if (time_before(n->used, n->confirmed))
 				n->used = n->confirmed;
 
+#if defined(CONFIG_ARP_GC_APPLY_GUARDBAND)
+			/* Do not garbage collect directly after we
+			 * updated n->state to allow applications to
+			 * react to the event
+			 */
+			if (time_before(jiffies,
+					n->updated + CONFIG_ARP_GC_GUARDBAND)) {
+				goto next_elt;
+			}
+#endif
+
 			if (atomic_read(&n->refcnt) == 1 &&
 			    (state == NUD_FAILED ||
 			     time_after(jiffies, n->used + NEIGH_VAR(n->parms, GC_STALETIME)))) {

@@ -802,9 +812,8 @@ static void neigh_periodic_work(struct work_struct *work)
 				neigh_cleanup_and_release(n);
 				continue;
 			}
-			write_unlock(&n->lock);
-
 next_elt:
+			write_unlock(&n->lock);
 			np = &n->next;
 		}
 		/*
#2 lore raw

Den 2015-03-15 09:27, YOSHIFUJI Hideaki skrev:

Quoted 9 lines

Hello.

Quoted 6 lines · Ulf Samuelsson

From: Ulf Samuelsson <[email protected]>

The neighbour state is changed in the ARP timer handler. If the state is changed to NUD_STALE, then the neighbour entry becomes a candidate for garbage collection.

Quoted 7 lines
Quoted 6 lines

The garbage collection is handled by a "periodic work" routine.

When :

* noone refers to the entry
* the state is no longer valid (I.E: NUD_STALE).

NUD_STALE is still valid.

Yes, my fault.
The condition which causes garbage collection to be skipped is.

if (state & (NUD_PERMANENT | NUD_IN_TIMER)) {

NUD_STALE is not part of that, so GC will not be skipped, and therefore the patch is needed if you want to be able to use the API to modify the neigh statemachine..

Quoted 21 lines
Quoted 21 lines

* the timeout value has been reached or state is FAILED

the "periodic work" routine will notify
the stack that the entry should be deleted.

A user application monitoring and controlling the neighbour table using NETLINK may fail, if the "period work" routine is run directly after the state has been changed to NUD_STALE, but before the user application has had a chance to change the state to something valid.

The "period work" routine will detect the NUD_STALE state and if the timeout value has been reached, it will notify the stack that the entry should be deleted.

The patch adds a check in the periodic work routine which will skip test for garbage collection unless a number of ticks has passed since the last time the neighbour entry state was changed.

Quoted 7 lines
Quoted 5 lines

The feature is controlled through Kconfig

The featuree is enabled by setting ARP_GC_APPLY_GUARDBAND The guardband time (in ticks) is set in ARP_GC_GUARDBAND Default time is 100 ms if HZ_### is set.

We have "lower limit" not to start releasing neighbour entries. Try increasing gc_thresh1.

Why would that work?

The only place where this is used is

    "if (atomic_read(&tbl->entries) < tbl->gc_thresh1)"

tbl->entries is related to how many entries there are in the neighbour table.

The only way I think this would work, is if this is raised so high that garbage collection does not occur.

That is not the intention.

It does not solve the race condition between the timer_handler and the periodic_work.

BR
Ulf Samuelsson

Quoted 93 lines

--yoshfuji

Quoted 90 lines
Signed-off-by Ulf Samuelsson <[email protected]>
---
  net/Kconfig          |   32 ++++++++++++++++++++++++++++++++
  net/core/neighbour.c |   15 ++++++++++++---
  2 files changed, 44 insertions(+), 3 deletions(-)
net/Kconfig +32 -0
index 44dd578..099a5dd 100644
@@ -77,6 +77,38 @@ config INET
  	  Short answer: say Y.
 
  if INET
+
+#
+# Core Network configuration
+#
+
+config ARP_GC_APPLY_GUARDBAND
+	bool "IP: ARP: Avoid garbage collection directly after state change"
+	default n
+	---help---
+	  With this item selected, an entry in the neighbour table
+	  will not be garbage collected directly after the ARP state
+	  has changed to STALE of FAILED
+	  This allows an application program change the state to something valid
+	  before garbage colllection occurs.
+
+	  If unsure, say N.
+
+config ARP_GC_GUARDBAND
+	int "Guardband time on garbage collection"
+	depends on ARP_GC_APPLY_GUARDBAND
+	default 10 if HZ_100
+	default 25 if HZ_250
+	default 30 if HZ_300
+	default 100 if HZ_1000
+	default 100
+
+	---help---
+	  The number of ticks to delay garbage collection
+	  after the neighbour entry has been updated
+	  A delay of 100 ms is reasonable.
+	  With CONFIG_HZ = 250, this value should be 25
+
  source "net/ipv4/Kconfig"
  source "net/ipv6/Kconfig"
  source "net/netlabel/Kconfig"
net/core/neighbour.c +12 -3
index 70fe9e1..194195d 100644
@@ -786,13 +786,23 @@ static void neigh_periodic_work(struct work_struct *work)
 
  			state = n->nud_state;
  			if (state & (NUD_PERMANENT | NUD_IN_TIMER)) {
-				write_unlock(&n->lock);
  				goto next_elt;
  			}
 
  			if (time_before(n->used, n->confirmed))
  				n->used = n->confirmed;
 
+#if defined(CONFIG_ARP_GC_APPLY_GUARDBAND)
+			/* Do not garbage collect directly after we
+			 * updated n->state to allow applications to
+			 * react to the event
+			 */
+			if (time_before(jiffies,
+					n->updated + CONFIG_ARP_GC_GUARDBAND)) {
+				goto next_elt;
+			}
+#endif
+
  			if (atomic_read(&n->refcnt) == 1 &&
  			    (state == NUD_FAILED ||
  			     time_after(jiffies, n->used + NEIGH_VAR(n->parms, GC_STALETIME)))) {

@@ -802,9 +812,8 @@ static void neigh_periodic_work(struct work_struct *work)
  				neigh_cleanup_and_release(n);
  				continue;
  			}
-			write_unlock(&n->lock);
-
  next_elt:
+			write_unlock(&n->lock);
  			np = &n->next;
  		}
  		/*
Signature · 4 lines
Best Regards
Ulf Samuelsson
[email protected]
+46 722 427437
#3 lore raw

Hello.

Quoted 60 lines · Ulf Samuelsson

Den 2015-03-15 09:27, YOSHIFUJI Hideaki skrev:

Quoted 9 lines

Hello.

Quoted 6 lines · Ulf Samuelsson

From: Ulf Samuelsson <[email protected]>

The neighbour state is changed in the ARP timer handler. If the state is changed to NUD_STALE, then the neighbour entry becomes a candidate for garbage collection.

Quoted 7 lines
Quoted 6 lines

The garbage collection is handled by a "periodic work" routine.

When :

* noone refers to the entry
* the state is no longer valid (I.E: NUD_STALE).

NUD_STALE is still valid.

Yes, my fault.
The condition which causes garbage collection to be skipped is.

if (state & (NUD_PERMANENT | NUD_IN_TIMER)) {

NUD_STALE is not part of that, so GC will not be skipped, and therefore the patch is needed if you want to be able to use the API to modify the neigh statemachine..

Quoted 21 lines
Quoted 21 lines

* the timeout value has been reached or state is FAILED

the "periodic work" routine will notify
the stack that the entry should be deleted.

A user application monitoring and controlling the neighbour table using NETLINK may fail, if the "period work" routine is run directly after the state has been changed to NUD_STALE, but before the user application has had a chance to change the state to something valid.

The "period work" routine will detect the NUD_STALE state and if the timeout value has been reached, it will notify the stack that the entry should be deleted.

The patch adds a check in the periodic work routine which will skip test for garbage collection unless a number of ticks has passed since the last time the neighbour entry state was changed.

Quoted 7 lines
Quoted 5 lines

The feature is controlled through Kconfig

The featuree is enabled by setting ARP_GC_APPLY_GUARDBAND The guardband time (in ticks) is set in ARP_GC_GUARDBAND Default time is 100 ms if HZ_### is set.

We have "lower limit" not to start releasing neighbour entries. Try increasing gc_thresh1.

Why would that work?

The only place where this is used is

     "if (atomic_read(&tbl->entries) < tbl->gc_thresh1)"
Quoted 8 lines

tbl->entries is related to how many entries there are in the neighbour table.

The only way I think this would work, is if this is raised so high that garbage collection does not occur.

That is not the intention.

It does not solve the race condition between the timer_handler and the periodic_work.

I don't think it is a race.

You can try increasing gc_staletime to hold each entry based on last usage. Plus, you can "confirm" neighbors by MSG_CONFIRM.

Note that if the number of entries becomes high, "forced GC" will drop valid, "not connected" entries as well.

--yoshfuji

Quoted 95 lines

BR
Ulf Samuelsson

Quoted 89 lines

--yoshfuji

Quoted 86 lines
Signed-off-by Ulf Samuelsson <[email protected]>
---
  net/Kconfig          |   32 ++++++++++++++++++++++++++++++++
  net/core/neighbour.c |   15 ++++++++++++---
  2 files changed, 44 insertions(+), 3 deletions(-)
net/Kconfig +32 -0
index 44dd578..099a5dd 100644
@@ -77,6 +77,38 @@ config INET
        Short answer: say Y.
  if INET
+
+#
+# Core Network configuration
+#
+
+config ARP_GC_APPLY_GUARDBAND
+    bool "IP: ARP: Avoid garbage collection directly after state change"
+    default n
+    ---help---
+      With this item selected, an entry in the neighbour table
+      will not be garbage collected directly after the ARP state
+      has changed to STALE of FAILED
+      This allows an application program change the state to something valid
+      before garbage colllection occurs.
+
+      If unsure, say N.
+
+config ARP_GC_GUARDBAND
+    int "Guardband time on garbage collection"
+    depends on ARP_GC_APPLY_GUARDBAND
+    default 10 if HZ_100
+    default 25 if HZ_250
+    default 30 if HZ_300
+    default 100 if HZ_1000
+    default 100
+
+    ---help---
+      The number of ticks to delay garbage collection
+      after the neighbour entry has been updated
+      A delay of 100 ms is reasonable.
+      With CONFIG_HZ = 250, this value should be 25
+
  source "net/ipv4/Kconfig"
  source "net/ipv6/Kconfig"
  source "net/netlabel/Kconfig"
net/core/neighbour.c +12 -3
index 70fe9e1..194195d 100644
@@ -786,13 +786,23 @@ static void neigh_periodic_work(struct work_struct *work)
              state = n->nud_state;
              if (state & (NUD_PERMANENT | NUD_IN_TIMER)) {
-                write_unlock(&n->lock);
                  goto next_elt;
              }
              if (time_before(n->used, n->confirmed))
                  n->used = n->confirmed;
+#if defined(CONFIG_ARP_GC_APPLY_GUARDBAND)
+            /* Do not garbage collect directly after we
+             * updated n->state to allow applications to
+             * react to the event
+             */
+            if (time_before(jiffies,
+                    n->updated + CONFIG_ARP_GC_GUARDBAND)) {
+                goto next_elt;
+            }
+#endif
+
              if (atomic_read(&n->refcnt) == 1 &&
                  (state == NUD_FAILED ||
                   time_after(jiffies, n->used + NEIGH_VAR(n->parms, GC_STALETIME)))) {

@@ -802,9 +812,8 @@ static void neigh_periodic_work(struct work_struct *work)
                  neigh_cleanup_and_release(n);
                  continue;
              }
-            write_unlock(&n->lock);
-
  next_elt:
+            write_unlock(&n->lock);
              np = &n->next;
          }
          /*
Signature · 2 lines
吉藤英明 <[email protected]>
ミラクル・リナックス株式会社 技術本部 サポート部
#4 lore raw

Den 2015-03-16 05:57, YOSHIFUJI Hideaki/吉藤英明 skrev:

Quoted 65 lines

Hello.

Quoted 62 lines · Ulf Samuelsson

Den 2015-03-15 09:27, YOSHIFUJI Hideaki skrev:

Quoted 9 lines

Hello.

Quoted 6 lines · Ulf Samuelsson

From: Ulf Samuelsson <[email protected]>

The neighbour state is changed in the ARP timer handler. If the state is changed to NUD_STALE, then the neighbour entry becomes a candidate for garbage collection.

Quoted 7 lines
Quoted 6 lines

The garbage collection is handled by a "periodic work" routine.

When :

* noone refers to the entry
* the state is no longer valid (I.E: NUD_STALE).

NUD_STALE is still valid.

Yes, my fault.
The condition which causes garbage collection to be skipped is.

if (state & (NUD_PERMANENT | NUD_IN_TIMER)) {

NUD_STALE is not part of that, so GC will not be skipped, and therefore the patch is needed if you want to be able to use the API to modify the neigh statemachine..

Quoted 21 lines
Quoted 21 lines

* the timeout value has been reached or state is FAILED

the "periodic work" routine will notify
the stack that the entry should be deleted.

A user application monitoring and controlling the neighbour table using NETLINK may fail, if the "period work" routine is run directly after the state has been changed to NUD_STALE, but before the user application has had a chance to change the state to something valid.

The "period work" routine will detect the NUD_STALE state and if the timeout value has been reached, it will notify the stack that the entry should be deleted.

The patch adds a check in the periodic work routine which will skip test for garbage collection unless a number of ticks has passed since the last time the neighbour entry state was changed.

Quoted 7 lines
Quoted 5 lines

The feature is controlled through Kconfig

The featuree is enabled by setting ARP_GC_APPLY_GUARDBAND The guardband time (in ticks) is set in ARP_GC_GUARDBAND Default time is 100 ms if HZ_### is set.

We have "lower limit" not to start releasing neighbour entries. Try increasing gc_thresh1.

Why would that work?

The only place where this is used is

     "if (atomic_read(&tbl->entries) < tbl->gc_thresh1)"

tbl->entries is related to how many entries there are in the neighbour table.

Quoted 8 lines
Quoted 6 lines

The only way I think this would work, is if this is raised so high that garbage collection does not occur.

That is not the intention.

It does not solve the race condition between the timer_handler and the periodic_work.

I don't think it is a race.

And I think you are wrong and my logging shows that it is.

Quoted 7 lines

You can try increasing gc_staletime to hold each entry based on last usage. Plus, you can "confirm" neighbors by MSG_CONFIRM.

Note that if the number of entries becomes high, "forced GC" will drop valid, "not connected" entries as well.

I can try increasing gc_staletime, but its a waste of time, because it is not the last usage which is interesting. What is interesting is the time when the entry state was updated by the timer handler.

Pls explain further MSG_CONFIRM.

The problem occurs when the periodic_work routine runs immediately after the timer handler has changes the state to NUD_STALE and the entry has reached the expiry time.

I suggest you read the code, and you will understand what I mean.

Ulf

Quoted 104 lines

--yoshfuji

Quoted 100 lines

BR
Ulf Samuelsson

Quoted 94 lines

--yoshfuji

Quoted 91 lines
Signed-off-by Ulf Samuelsson <[email protected]>
---
  net/Kconfig          |   32 ++++++++++++++++++++++++++++++++
  net/core/neighbour.c |   15 ++++++++++++---
  2 files changed, 44 insertions(+), 3 deletions(-)
net/Kconfig +7 -0
index 44dd578..099a5dd 100644
@@ -77,6 +77,38 @@ config INET
        Short answer: say Y.
  if INET
+
+#
+# Core Network configuration
+#
+
+config ARP_GC_APPLY_GUARDBAND
+    bool "IP: ARP: Avoid garbage collection directly after state

change"

net/Kconfig +6 -0
+    default n
+    ---help---
+      With this item selected, an entry in the neighbour table
+      will not be garbage collected directly after the ARP state
+      has changed to STALE of FAILED
+      This allows an application program change the state to

something valid

net/Kconfig +19 -0
+      before garbage colllection occurs.
+
+      If unsure, say N.
+
+config ARP_GC_GUARDBAND
+    int "Guardband time on garbage collection"
+    depends on ARP_GC_APPLY_GUARDBAND
+    default 10 if HZ_100
+    default 25 if HZ_250
+    default 30 if HZ_300
+    default 100 if HZ_1000
+    default 100
+
+    ---help---
+      The number of ticks to delay garbage collection
+      after the neighbour entry has been updated
+      A delay of 100 ms is reasonable.
+      With CONFIG_HZ = 250, this value should be 25
+
  source "net/ipv4/Kconfig"
  source "net/ipv6/Kconfig"
  source "net/netlabel/Kconfig"
net/core/neighbour.c +12 -3
index 70fe9e1..194195d 100644
@@ -786,13 +786,23 @@ static void neigh_periodic_work(struct 
 work_struct *work)
              state = n->nud_state;
              if (state & (NUD_PERMANENT | NUD_IN_TIMER)) {
-                write_unlock(&n->lock);
                  goto next_elt;
              }
              if (time_before(n->used, n->confirmed))
                  n->used = n->confirmed;
+#if defined(CONFIG_ARP_GC_APPLY_GUARDBAND)
+            /* Do not garbage collect directly after we
+             * updated n->state to allow applications to
+             * react to the event
+             */
+            if (time_before(jiffies,
+                    n->updated + CONFIG_ARP_GC_GUARDBAND)) {
+                goto next_elt;
+            }
+#endif
+
              if (atomic_read(&n->refcnt) == 1 &&
                  (state == NUD_FAILED ||
                   time_after(jiffies, n->used + 
 NEIGH_VAR(n->parms, GC_STALETIME)))) {

@@ -802,9 +812,8 @@ static void neigh_periodic_work(struct 
 work_struct *work)
                  neigh_cleanup_and_release(n);
                  continue;
              }
-            write_unlock(&n->lock);
-
  next_elt:
+            write_unlock(&n->lock);
              np = &n->next;
          }

          /*
Signature · 4 lines
Best Regards
Ulf Samuelsson
[email protected]
+46 722 427437
#5 lore raw

Hello,

Quoted 69 lines · Ulf Samuelsson

Den 2015-03-16 05:57, YOSHIFUJI Hideaki/吉藤英明 skrev:

Quoted 68 lines

Hello.

Quoted 65 lines · Ulf Samuelsson

Den 2015-03-15 09:27, YOSHIFUJI Hideaki skrev:

Quoted 9 lines

Hello.

Quoted 6 lines · Ulf Samuelsson

From: Ulf Samuelsson <[email protected]>

The neighbour state is changed in the ARP timer handler. If the state is changed to NUD_STALE, then the neighbour entry becomes a candidate for garbage collection.

Quoted 7 lines
Quoted 6 lines

The garbage collection is handled by a "periodic work" routine.

When :

* noone refers to the entry
* the state is no longer valid (I.E: NUD_STALE).

NUD_STALE is still valid.

Yes, my fault.
The condition which causes garbage collection to be skipped is.

if (state & (NUD_PERMANENT | NUD_IN_TIMER)) {

NUD_STALE is not part of that, so GC will not be skipped, and therefore the patch is needed if you want to be able to use the API to modify the neigh statemachine..

Quoted 21 lines
Quoted 21 lines

* the timeout value has been reached or state is FAILED

the "periodic work" routine will notify
the stack that the entry should be deleted.

A user application monitoring and controlling the neighbour table using NETLINK may fail, if the "period work" routine is run directly after the state has been changed to NUD_STALE, but before the user application has had a chance to change the state to something valid.

The "period work" routine will detect the NUD_STALE state and if the timeout value has been reached, it will notify the stack that the entry should be deleted.

The patch adds a check in the periodic work routine which will skip test for garbage collection unless a number of ticks has passed since the last time the neighbour entry state was changed.

Quoted 7 lines
Quoted 5 lines

The feature is controlled through Kconfig

The featuree is enabled by setting ARP_GC_APPLY_GUARDBAND The guardband time (in ticks) is set in ARP_GC_GUARDBAND Default time is 100 ms if HZ_### is set.

We have "lower limit" not to start releasing neighbour entries. Try increasing gc_thresh1.

Why would that work?

The only place where this is used is

     "if (atomic_read(&tbl->entries) < tbl->gc_thresh1)"

tbl->entries is related to how many entries there are in the neighbour table.

The only way I think this would work, is if this is raised so high that garbage collection does not occur.

Quoted 6 lines
Quoted 5 lines
Quoted 3 lines

That is not the intention.

It does not solve the race condition between the timer_handler and the periodic_work.

I don't think it is a race.

And I think you are wrong and my logging shows that it is.

We do not gurantee holding "stale" entries more than gc_thresh1, so it shall not be called as a race at all.

Quoted 6 lines
Quoted 6 lines

You can try increasing gc_staletime to hold each entry based on last usage. Plus, you can "confirm" neighbors by MSG_CONFIRM.

Quoted 7 lines
Quoted 2 lines

Note that if the number of entries becomes high, "forced GC" will drop valid, "not connected" entries as well.

I can try increasing gc_staletime, but its a waste of time, because it is not the last usage which is interesting. What is interesting is the time when the entry state was updated by the timer handler.

Pls explain further MSG_CONFIRM.

Typically, base reachable time is set to 30sec and gc_staletime is set to 60sec. So, entries are expected to be held for a while after it has become "stale", no?

MSG_CONFIRM is a sendmsg() flag. It allows user-space application to confirm reachability of neighbor. It refreshes "confirmed" time. In neigh_periodic_work(), "used" time is updated to "confirmed" time if "used" time is before "confirmed" time.

ping(8), ping6(8), tftpd(8) use that flag, for example.

Quoted 3 lines

The problem occurs when the periodic_work routine runs immediately after the timer handler has changes the state to NUD_STALE and the entry has reached the expiry time.

It is what we expect.

In neigh_periodic_work(), you may try to release non-STALE entries first, and then STALE entries if the number of entries is still high.

--yoshfuji

author
#6 lore raw

Den 2015-03-17 13:31, YOSHIFUJI Hideaki skrev:

Quoted 74 lines

Hello,

Quoted 71 lines · Ulf Samuelsson

Den 2015-03-16 05:57, YOSHIFUJI Hideaki/吉藤英明 skrev:

Quoted 70 lines

Hello.

Quoted 67 lines · Ulf Samuelsson

Den 2015-03-15 09:27, YOSHIFUJI Hideaki skrev:

Quoted 9 lines

Hello.

Quoted 6 lines · Ulf Samuelsson

From: Ulf Samuelsson <[email protected]>

The neighbour state is changed in the ARP timer handler. If the state is changed to NUD_STALE, then the neighbour entry becomes a candidate for garbage collection.

Quoted 7 lines
Quoted 6 lines

The garbage collection is handled by a "periodic work" routine.

When :

* noone refers to the entry
* the state is no longer valid (I.E: NUD_STALE).

NUD_STALE is still valid.

Yes, my fault.
The condition which causes garbage collection to be skipped is.

if (state & (NUD_PERMANENT | NUD_IN_TIMER)) {

NUD_STALE is not part of that, so GC will not be skipped, and therefore the patch is needed if you want to be able to use the API to modify the neigh statemachine..

Quoted 21 lines
Quoted 21 lines

* the timeout value has been reached or state is FAILED

the "periodic work" routine will notify
the stack that the entry should be deleted.

A user application monitoring and controlling the neighbour table using NETLINK may fail, if the "period work" routine is run directly after the state has been changed to NUD_STALE, but before the user application has had a chance to change the state to something valid.

The "period work" routine will detect the NUD_STALE state and if the timeout value has been reached, it will notify the stack that the entry should be deleted.

The patch adds a check in the periodic work routine which will skip test for garbage collection unless a number of ticks has passed since the last time the neighbour entry state was changed.

Quoted 7 lines
Quoted 5 lines

The feature is controlled through Kconfig

The featuree is enabled by setting ARP_GC_APPLY_GUARDBAND The guardband time (in ticks) is set in ARP_GC_GUARDBAND Default time is 100 ms if HZ_### is set.

We have "lower limit" not to start releasing neighbour entries. Try increasing gc_thresh1.

Why would that work?

The only place where this is used is

     "if (atomic_read(&tbl->entries) < tbl->gc_thresh1)"

tbl->entries is related to how many entries there are in the neighbour table.

The only way I think this would work, is if this is raised so high that garbage collection does not occur.

That is not the intention.

Quoted 7 lines
Quoted 4 lines
Quoted 3 lines
Quoted 1 line

It does not solve the race condition between the timer_handler and the periodic_work.

I don't think it is a race.

And I think you are wrong and my logging shows that it is.

We do not gurantee holding "stale" entries more than gc_thresh1, so it shall not be called as a race at all.

And that is a problem, which results in traffic loss.

Quoted 15 lines
Quoted 7 lines
Quoted 7 lines

You can try increasing gc_staletime to hold each entry based on last usage. Plus, you can "confirm" neighbors by MSG_CONFIRM.

Quoted 7 lines
Quoted 2 lines

Note that if the number of entries becomes high, "forced GC" will drop valid, "not connected" entries as well.

I can try increasing gc_staletime, but its a waste of time, because it is not the last usage which is interesting. What is interesting is the time when the entry state was updated by the timer handler.

Pls explain further MSG_CONFIRM.

Quoted 9 lines

Typically, base reachable time is set to 30sec and gc_staletime is set to 60sec. So, entries are expected to be held for a while after it has become "stale", no?

MSG_CONFIRM is a sendmsg() flag. It allows user-space application to confirm reachability of neighbor. It refreshes "confirmed" time. In neigh_periodic_work(), "used" time is updated to "confirmed" time if "used" time is before "confirmed" time.

May work, if the application was totally redesigned,

Quoted 7 lines

ping(8), ping6(8), tftpd(8) use that flag, for example.

Quoted 3 lines

The problem occurs when the periodic_work routine runs immediately after the timer handler has changes the state to NUD_STALE and the entry has reached the expiry time.

Quoted 5 lines

It is what we expect.

In neigh_periodic_work(), you may try to release non-STALE entries first, and then STALE entries if the number of entries is still high.

That sounds much more complex than the proposed fix.

Quoted 2 lines

--yoshfuji