neighbour.c: Avoid GC directly after state change

I hate changes like this.

By making this a Kconfig options it cannot be dynamic, and every distribution is going to have to scratch their head and decide what to set this to.

That's seriously suboptimal.

If you want to change behavior based upon whether userspace is managing the damn table, make it so the user doing so has to ask for the new behavior at _RUN TIME_ via the netlink interface or similar.

Picking the guard time itself at compile time is also undesirable.

And you don't even want a damn timer, what you want is for the state of the entry to be frozen and for the user to "release" it by either adjusting the state to something else or marking in some other way to allow it to be unfrozen and released again.

Why put it to chance with some timeout? Make things explicit.

I'm not applying this patch.

--
To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Den 2015-03-12 19:26, David Miller skrev:

Quoted 23 lines

I hate changes like this.

By making this a Kconfig options it cannot be dynamic, and every distribution is going to have to scratch their head and decide what to set this to.

That's seriously suboptimal.

If you want to change behavior based upon whether userspace is managing the damn table, make it so the user doing so has to ask for the new behavior at _RUN TIME_ via the netlink interface or similar.

Picking the guard time itself at compile time is also undesirable.

And you don't even want a damn timer, what you want is for the state of the entry to be frozen and for the user to "release" it by either adjusting the state to something else or marking in some other way to allow it to be unfrozen and released again.

Why put it to chance with some timeout? Make things explicit.

I'm not applying this patch.

Sounds reasonable comments to me.

Would this approach work?

2 new message types are defined, to enable/disable the garbage collection functionality.

RTM_ENANEIGHGC
RTM_DISANEIGHGC

When the functionality is disabled, the stack will not garbage collect, and the external application will have to send netlink messages to delete unwanted entries.

1 new message is defined to move an entry from STALE to DELAY. RTM_NEIGHRECOVER

I hate changes like this.

By making this a Kconfig options it cannot be dynamic, and every distribution is going to have to scratch their head and decide what to set this to.

That's seriously suboptimal.

If you want to change behavior based upon whether userspace is managing the damn table, make it so the user doing so has to ask for the new behavior at _RUN TIME_ via the netlink interface or similar.

Picking the guard time itself at compile time is also undesirable.

And you don't even want a damn timer, what you want is for the state of the entry to be frozen and for the user to "release" it by either adjusting the state to something else or marking in some other way to allow it to be unfrozen and released again.

Why put it to chance with some timeout? Make things explicit.

I'm not applying this patch.

--
To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 13 lines · David Miller

I hate changes like this.

By making this a Kconfig options it cannot be dynamic, and every distribution is going to have to scratch their head and decide what to set this to.

That's seriously suboptimal.

If you want to change behavior based upon whether userspace is managing the damn table, make it so the user doing so has to ask for the new behavior at _RUN TIME_ via the netlink interface or similar.

Quoted 8 lines

Picking the guard time itself at compile time is also undesirable.

And you don't even want a damn timer, what you want is for the state of the entry to be frozen and for the user to "release" it by either adjusting the state to something else or marking in some other way to allow it to be unfrozen and released again.

Why put it to chance with some timeout? Make things explicit.

The desired functionality is that if communication stops, you want to send out ARP probes, before the entry is deleted.

The current (pseudo) code of the neigh timer is:

    if (state & NUD_REACHABLE) {
        if (now <= "confirmed + "reachable_time")) {
                    ... /* We are OK */
        } else if (now < "used" + DELAY_PROBE_TIME) {    /* Never 
happens */
                    state = NUD_DELAY;
        } else {
            state = NUD_STALE;
            notify = 1;
        }

We never see the state beeing changed from REACHABLE to DELAY, so the probes are not beeing sent out, instead you always go from REACHABLE to STALE.

DELAY_PROBE_TIME is set to (5 x HZ) and "used"
seems to be only set by the periodic_work routine
when the neigh entry is in STALE state, and then it is too late. It is also set by "arp_find" which is used by "broken" devices.

In practice, the second condition: "(now < "used" + DELAY_PROBE_TIME)" is never used. What is the intention of this test?

By adding a new test + parameter, we would get the desired functionality, and no need to listen for notifications or doing ARP state updates from applications.

        if (now <= "confirmed + "reachable_time")) {
                    ... /* We are OK */

(quoted diff) +2 -0

+        else if (now <= "confirmed + "reprobe_time")) {
+                   state <= NUD_DELAY;
        } else if (now < "used" + DELAY_PROBE_TIME))) {    /* Never 

happens */

                    state <= NUD_DELAY;
        } else {
            state = NUD_STALE;
            notify = 1;
        }

This way the entry would remain in REACHABLE while normal communication occurs, then it would enter DELAY state to probe, and if that fails, it goes to STALE state.

Alternatively, we just change the second test:

        if (now <= "confirmed + "reachable_time")) {
                    ... /* We are OK */
-        } else if (now < "used" + DELAY_PROBE_TIME))) {    /* Never 
happens */
+       } else if (now < "confirmed" + DELAY_PROBE_TIME))) {
                    state <= NUD_DELAY;
        } else {
            state = NUD_STALE;
            notify = 1;
        }

The DELAY_PROBE_TIME, should preferrably be a kernel Kconfig parameter.

Best Regards,
Ulf Samuelsson

Quoted 6 lines

I'm not applying this patch.

Signature · 3 lines

To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Signature · 3 lines

To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

The desired functionality is that if communication stops, you want to send out ARP probes, before the entry is deleted.

The current (pseudo) code of the neigh timer is:

    if (state & NUD_REACHABLE) {
        if (now <= "confirmed + "reachable_time")) {
                    ... /* We are OK */
        } else if (now < "used" + DELAY_PROBE_TIME) {    /* Never happens */
                    state = NUD_DELAY;
        } else {
            state = NUD_STALE;
            notify = 1;
        }

We never see the state beeing changed from REACHABLE to DELAY, so the probes are not beeing sent out, instead you always go from REACHABLE to STALE.

DELAY_PROBE_TIME is set to (5 x HZ) and "used"
seems to be only set by the periodic_work routine
when the neigh entry is in STALE state, and then it is too late. It is also set by "arp_find" which is used by "broken" devices.

In practice, the second condition: "(now < "used" + DELAY_PROBE_TIME)" is never used. What is the intention of this test?

By adding a new test + parameter, we would get the desired functionality, and no need to listen for notifications or doing ARP state updates from applications.

        if (now <= "confirmed + "reachable_time")) {
                    ... /* We are OK */

(quoted diff) +2 -0

+        else if (now <= "confirmed + "reprobe_time")) {
+                   state <= NUD_DELAY;
        } else if (now < "used" + DELAY_PROBE_TIME))) {    /* Never happens */
                    state <= NUD_DELAY;
        } else {
            state = NUD_STALE;
            notify = 1;
        }

This way the entry would remain in REACHABLE while normal communication occurs, then it would enter DELAY state to probe, and if that fails, it goes to STALE state.

From RFC2461:

Hi,

Quoted 15 lines · Ulf Samuelsson

The desired functionality is that if communication stops, you want to send out ARP probes, before the entry is deleted.

The current (pseudo) code of the neigh timer is:

     if (state & NUD_REACHABLE) {
         if (now <= "confirmed + "reachable_time")) {
                     ... /* We are OK */
         } else if (now < "used" + DELAY_PROBE_TIME) {    /* Never happens */
                     state = NUD_DELAY;
         } else {
             state = NUD_STALE;
             notify = 1;
         }

Quoted 3 lines

We never see the state beeing changed from REACHABLE to DELAY, so the probes are not beeing sent out, instead you always go from REACHABLE to STALE.

That's right.

Quoted 7 lines

DELAY_PROBE_TIME is set to (5 x HZ) and "used"
seems to be only set by the periodic_work routine
when the neigh entry is in STALE state, and then it is too late. It is also set by "arp_find" which is used by "broken" devices.

In STALE state, neigh->used is set by neigh_event_send(), called by neigh_resolve_output() via neigh->output().

Quoted 2 lines

In practice, the second condition: "(now < "used" + DELAY_PROBE_TIME)" is never used. What is the intention of this test?

That's right. It is NOT used in normal condition unless reachable time is too short.

Quoted 14 lines

By adding a new test + parameter, we would get the desired functionality, and no need to listen for notifications or doing ARP state updates from applications.

         if (now <= "confirmed + "reachable_time")) {
                     ... /* We are OK */

(quoted diff) +2 -0

+        else if (now <= "confirmed + "reprobe_time")) {
+                   state <= NUD_DELAY;
         } else if (now < "used" + DELAY_PROBE_TIME))) {    /* Never happens */
                     state <= NUD_DELAY;
         } else {
             state = NUD_STALE;
             notify = 1;
         }

Quoted 2 lines

This way the entry would remain in REACHABLE while normal communication occurs, then it would enter DELAY state to probe, and if that fails, it goes to STALE state.

No, it is not what REACHABLE and DELAY mean.

From RFC2461:

|      REACHABLE   Roughly speaking, the neighbor is known to have been
|                  reachable recently (within tens of seconds ago).
:
|      STALE       The neighbor is no longer known to be reachable but
|                  until traffic is sent to the neighbor, no attempt
|                  should be made to verify its reachability.
|      DELAY       The neighbor is no longer known to be reachable, and
|                  traffic has recently been sent to the neighbor.
|                  Rather than probe the neighbor immediately, however,
|                  delay sending probes for a short while in order to
|                  give upper layer protocols a chance to provide
|                  reachability confirmation.

Quoted 16 lines

From RFC2461:

|      REACHABLE   Roughly speaking, the neighbor is known to have been
|                  reachable recently (within tens of seconds ago).
:
|      STALE       The neighbor is no longer known to be reachable but
|                  until traffic is sent to the neighbor, no attempt
|                  should be made to verify its reachability.
|      DELAY       The neighbor is no longer known to be reachable, and
|                  traffic has recently been sent to the neighbor.
|                  Rather than probe the neighbor immediately, however,
|                  delay sending probes for a short while in order to
|                  give upper layer protocols a chance to provide
|                  reachability confirmation.

It is all depending on the meaning of the word "recently".
You imply, that if timeouts have been triggered, then it is no longer "recent", but that is not the only interpretation, it is up to the implementer to decide what is "recently".

Therefore, if a timeout occurs due to no traffic, they must be probed before they are garbage collected.

If this is not acceptable, how do you propose to solve the problem that you cannot make remote units inaccessible for more than a fraction of a second?

Hi,

Quoted 16 lines · Ulf Samuelsson

Quoted 16 lines

From RFC2461:

|      REACHABLE   Roughly speaking, the neighbor is known to have been
|                  reachable recently (within tens of seconds ago).
:
|      STALE       The neighbor is no longer known to be reachable but
|                  until traffic is sent to the neighbor, no attempt
|                  should be made to verify its reachability.
|      DELAY       The neighbor is no longer known to be reachable, and
|                  traffic has recently been sent to the neighbor.
|                  Rather than probe the neighbor immediately, however,
|                  delay sending probes for a short while in order to
|                  give upper layer protocols a chance to provide
|                  reachability confirmation.

Quoted 4 lines

It is all depending on the meaning of the word "recently".
You imply, that if timeouts have been triggered, then it is no longer "recent", but that is not the only interpretation, it is up to the implementer to decide what is "recently".

That quoted text is just a "brief" description. The document has detailed state machine.

Quoted 2 lines

Therefore, if a timeout occurs due to no traffic, they must be probed before they are garbage collected.

It is what we do in PROBE state.

Quoted 3 lines

If this is not acceptable, how do you propose to solve the problem that you cannot make remote units inaccessible for more than a fraction of a second?

How many neighbors do you want to maintain?
I guess you have to increase the number of gc_thresh1.

Quoted 2 lines

How many neighbors do you want to maintain?
I guess you have to increase the number of gc_thresh1.

The current use cases have up to 2048 entries. This is expected to grow in the future.
The 3.4 kernel used in the system today is limited to 1024, but that has been raised to about 10k.

The gc_thresh1 test is not implemented in 3.4 but can be backported, but still not convinced it is a good idea.

To complicate things, one requirement is that for some interfaces you always want to keep things alive, if connected, but for other interfaces you want things to be removed
to conserve memory.
Actually you would want to do this selection on a subnet level.

Internal discussions resulted in a proposal to change the patch, so that you have a "keepalive" flag which is tested after it has been decided to exit the REACHABLE state.

if the "keepalive" flag is set, you always go to DELAY state from REACHABLE.

Quoted 9 lines · Ulf Samuelsson

Quoted 2 lines

How many neighbors do you want to maintain?
I guess you have to increase the number of gc_thresh1.

The current use cases have up to 2048 entries. This is expected to grow in the future.
The 3.4 kernel used in the system today is limited to 1024, but that has been raised to about 10k.

The gc_thresh1 test is not implemented in 3.4 but can be backported, but still not convinced it is a good idea.

Why?

Quoted 5 lines

To complicate things, one requirement is that for some interfaces you always want to keep things alive, if connected, but for other interfaces you want things to be removed
to conserve memory.
Actually you would want to do this selection on a subnet level.

If you want to introduce per-interface parameter, I am okay with it.

Quoted 5 lines

Internal discussions resulted in a proposal to change the patch, so that you have a "keepalive" flag which is tested after it has been decided to exit the REACHABLE state.

if the "keepalive" flag is set, you always go to DELAY state from REACHABLE.

No.

Quoted 10 lines · YOSHIFUJI Hideaki

Quoted 9 lines · Ulf Samuelsson

Quoted 2 lines

How many neighbors do you want to maintain?
I guess you have to increase the number of gc_thresh1.

The current use cases have up to 2048 entries. This is expected to grow in the future.
The 3.4 kernel used in the system today is limited to 1024, but that has been raised to about 10k.

The gc_thresh1 test is not implemented in 3.4 but can be backported, but still not convinced it is a good idea.

Quoted 2 lines

Why?

A good solution makes sure that:
* equipment which is connected NEVER IS garbage collected
* equipment which is disconnected IS garbage collected.

The threshold idea does not meet the criteria for a good solution.

With this solution you keep unnecessary entries in the table. If you ever pass the limit, then equipment which should not be garbage collected may be.
It relies on someone keeping track of traffic loss, so needs more maintenance by the SysOp.try pr

The ARP probes should be considered to be NECESSARY traffic to maintain a quality link.
Obviously not everyone would want to make this trade-off.

Quoted 7 lines

Quoted 5 lines

To complicate things, one requirement is that for some interfaces you always want to keep things alive, if connected, but for other interfaces you want things to be removed
to conserve memory.
Actually you would want to do this selection on a subnet level.

If you want to introduce per-interface parameter, I am okay with it.

Quoted 7 lines

Quoted 5 lines

Internal discussions resulted in a proposal to change the patch, so that you have a "keepalive" flag which is tested after it has been decided to exit the REACHABLE state.

if the "keepalive" flag is set, you always go to DELAY state from REACHABLE.

No.

And why is it a bad idea to have a high quality connection?

Quoted 13 lines · Ulf Samuelsson

Quoted 12 lines · YOSHIFUJI Hideaki

Quoted 9 lines · Ulf Samuelsson

Quoted 2 lines

How many neighbors do you want to maintain?
I guess you have to increase the number of gc_thresh1.

The current use cases have up to 2048 entries. This is expected to grow in the future.
The 3.4 kernel used in the system today is limited to 1024, but that has been raised to about 10k.

The gc_thresh1 test is not implemented in 3.4 but can be backported, but still not convinced it is a good idea.

Why?

Quoted 5 lines

A good solution makes sure that:
* equipment which is connected NEVER IS garbage collected
* equipment which is disconnected IS garbage collected.

The threshold idea does not meet the criteria for a good solution.

We try providing "good solution" if you have less than gc_thresh1 entries only. Otherwise, we try hard to protect ourselves.

Quoted 23 lines

With this solution you keep unnecessary entries in the table. If you ever pass the limit, then equipment which should not be garbage collected may be.
It relies on someone keeping track of traffic loss, so needs more maintenance by the SysOp.try pr

The ARP probes should be considered to be NECESSARY traffic to maintain a quality link.
Obviously not everyone would want to make this trade-off.

Quoted 11 lines

Quoted 5 lines

To complicate things, one requirement is that for some interfaces you always want to keep things alive, if connected, but for other interfaces you want things to be removed
to conserve memory.
Actually you would want to do this selection on a subnet level.

If you want to introduce per-interface parameter, I am okay with it.

Quoted 4 lines

Internal discussions resulted in a proposal to change the patch, so that you have a "keepalive" flag which is tested after it has been decided to exit the REACHABLE state.

Quoted 4 lines

Quoted 3 lines

Quoted 1 line

if the "keepalive" flag is set, you always go to DELAY state from REACHABLE.

No.

And why is it a bad idea to have a high quality connection?

We reclaim neighbor entries as much as possible to protect ourselves if the number is below gc_thresh1. We could stop purging entries, but the idea was rejected AFAIK. That is our design.

Again, you should increase gc_thresh1, first.

Quoted 14 lines · YOSHIFUJI Hideaki

Quoted 13 lines · Ulf Samuelsson

Quoted 12 lines · YOSHIFUJI Hideaki

Quoted 9 lines · Ulf Samuelsson

Quoted 2 lines

How many neighbors do you want to maintain?
I guess you have to increase the number of gc_thresh1.

The current use cases have up to 2048 entries. This is expected to grow in the future.
The 3.4 kernel used in the system today is limited to 1024, but that has been raised to about 10k.

The gc_thresh1 test is not implemented in 3.4 but can be backported, but still not convinced it is a good idea.

Why?

Quoted 7 lines

Quoted 5 lines

A good solution makes sure that:
* equipment which is connected NEVER IS garbage collected
* equipment which is disconnected IS garbage collected.

The threshold idea does not meet the criteria for a good solution.

We try providing "good solution" if you have less than gc_thresh1 entries only. Otherwise, we try hard to protect ourselves.

Protect against what?

Quoted 23 lines

Quoted 23 lines

With this solution you keep unnecessary entries in the table. If you ever pass the limit, then equipment which should not be garbage collected may be.
It relies on someone keeping track of traffic loss, so needs more maintenance by the SysOp.try pr

The ARP probes should be considered to be NECESSARY traffic to maintain a quality link.
Obviously not everyone would want to make this trade-off.

Quoted 11 lines

Quoted 5 lines

To complicate things, one requirement is that for some interfaces you always want to keep things alive, if connected, but for other interfaces you want things to be removed
to conserve memory.
Actually you would want to do this selection on a subnet level.

If you want to introduce per-interface parameter, I am okay with it.

Quoted 4 lines

Internal discussions resulted in a proposal to change the patch, so that you have a "keepalive" flag which is tested after it has been decided to exit the REACHABLE state.

Quoted 8 lines

Quoted 4 lines

Quoted 3 lines

Quoted 1 line

if the "keepalive" flag is set, you always go to DELAY state from REACHABLE.

No.

And why is it a bad idea to have a high quality connection?

We reclaim neighbor entries as much as possible to protect ourselves if the number is below gc_thresh1. We could stop purging entries, but the idea was rejected AFAIK. That is our design.

No you keep disconnected entries which can be removed
in the table as long as you are below gc_tresh1, so you waste memory.

This is a trade-off to reduce communication over the wire at the expense of quality of service.

If you can live with the drawback of extra bandwidth usage, then doing probes before you remove an entry will reduce memory usage and improve the quality of the communication.

Why do you want to decide which trade-off is best? Why not let the system designer make that decision?

I am not asking to stop purging entries. Entries which are not needed *should* be purged. But entries which are needed should not be purged, because that means traffic loss.
This design does not differentiate between the two providing unnecessary risks and wastes memory.

We know beforehand that if a neighbour is connected
to a certain interface, it should always be present in the table. You do not want us to use that knowledge when configuring the kernel.

I think that the idea that was rejected must have been something different.

Quoted 3 lines

Again, you should increase gc_thresh1, first.

To what number?
When equipment is delivered from a factory, it is not known how many entries are needed, and if it becomes known, it can later grow. Why add extra maintenance to the system?

Best Regards,
Ulf Samuelsson

Signature · 3 lines

To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html