neighbour.c: Avoid GC directly after state change
Subsystems (2)
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.
--- net/Kconfig | 32 ++++++++++++++++++++++++++++++++ net/core/neighbour.c | 15 ++++++++++++--- 2 files changed, 44 insertions(+), 3 deletions(-)
net/Kconfig +32 -0
@@ -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
@@ -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
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_STALEstate 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
KconfigThe featuree is enabled by setting
ARP_GC_APPLY_GUARDBANDThe guardband time (in ticks) is set inARP_GC_GUARDBANDDefault 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@@ -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@@ -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; }/*
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_STALEis 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_STALEstate 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
KconfigThe featuree is enabled by setting
ARP_GC_APPLY_GUARDBANDThe guardband time (in ticks) is set inARP_GC_GUARDBANDDefault 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@@ -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@@ -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
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_STALEis still valid.Yes, my fault.
The condition which causes garbage collection to be skipped is.if (state & (
NUD_PERMANENT|NUD_IN_TIMER)) {
NUD_STALEis 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_STALEstate 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
KconfigThe featuree is enabled by setting
ARP_GC_APPLY_GUARDBANDThe guardband time (in ticks) is set inARP_GC_GUARDBANDDefault 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->entriesis 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_handlerand theperiodic_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 SamuelssonQuoted 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@@ -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@@ -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]> ミラクル・リナックス株式会社 技術本部 サポート部
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_STALEis still valid.Yes, my fault.
The condition which causes garbage collection to be skipped is.if (state & (
NUD_PERMANENT|NUD_IN_TIMER)) {
NUD_STALEis 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_STALEstate 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
KconfigThe featuree is enabled by setting
ARP_GC_APPLY_GUARDBANDThe guardband time (in ticks) is set inARP_GC_GUARDBANDDefault 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->entriesis 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_handlerand theperiodic_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_staletimeto hold each entry based on last usage. Plus, you can "confirm" neighbors byMSG_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 SamuelssonQuoted 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@@ -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 statechange"
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 tosomething 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@@ -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
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_STALEis still valid.Yes, my fault.
The condition which causes garbage collection to be skipped is.if (state & (
NUD_PERMANENT|NUD_IN_TIMER)) {
NUD_STALEis 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_STALEstate 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
KconfigThe featuree is enabled by setting
ARP_GC_APPLY_GUARDBANDThe guardband time (in ticks) is set inARP_GC_GUARDBANDDefault 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->entriesis 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_handlerand theperiodic_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_staletimeto hold each entry based on last usage. Plus, you can "confirm" neighbors byMSG_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_workroutine runs immediately after the timer handler has changes the state toNUD_STALEand 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
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_STALEis still valid.Yes, my fault.
The condition which causes garbage collection to be skipped is.if (state & (
NUD_PERMANENT|NUD_IN_TIMER)) {
NUD_STALEis 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_STALEstate 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
KconfigThe featuree is enabled by setting
ARP_GC_APPLY_GUARDBANDThe guardband time (in ticks) is set inARP_GC_GUARDBANDDefault 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->entriesis 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_handlerand theperiodic_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_staletimeto hold each entry based on last usage. Plus, you can "confirm" neighbors byMSG_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_staletimeis set to 60sec. So, entries are expected to be held for a while after it has become "stale", no?
MSG_CONFIRMis asendmsg()flag. It allows user-space application to confirm reachability of neighbor. It refreshes "confirmed" time. Inneigh_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_workroutine runs immediately after the timer handler has changes the state toNUD_STALEand 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