Re: [PATCH] neighbour.c: Avoid GC directly after state change

[Ulf Samuelsson <ulf.samuelsson@ericsson.com>]

No reply on this....

in net/core/neighbour.c: neigh_timer_handler I see:

     if (state & NUD_REACHABLE) {
         if (time_before_eq(now,
                    neigh->confirmed + neigh->parms->reachable_time)) {
             neigh_dbg(2, "neigh %p is still alive\n", neigh);
             next = neigh->confirmed + neigh->parms->reachable_time;
         } else if (time_before_eq(now,
                       neigh->used +
                       NEIGH_VAR(neigh->parms, DELAY_PROBE_TIME))) {
             neigh_dbg(2, "neigh %p is delayed\n", neigh);
             neigh->nud_state = NUD_DELAY;
             neigh->updated = jiffies;
             neigh_suspect(neigh);
             next = now + NEIGH_VAR(neigh->parms, DELAY_PROBE_TIME);
         } else {
             neigh_dbg(2, "neigh %p is suspected\n", neigh);
             neigh->nud_state = NUD_STALE;
             neigh->updated = jiffies;
             neigh_suspect(neigh);
             notify = 1;
         }
     } else ...

Why is the second test there in the first place?

In the normal case, "neigh->used" does not get updated until the entry 
is found STALE
in the periodic work.

Why not use "neigh->confirmed" and another sysctl variable?

     if (state & NUD_REACHABLE) {
         if (time_before_eq(now,
                    neigh->confirmed + neigh->parms->reachable_time)) {
             neigh_dbg(2, "neigh %p is still alive\n", neigh);
             next = neigh->confirmed + neigh->parms->reachable_time;
         } else if (time_before_eq(now,
-                     neigh->used +
-                      NEIGH_VAR(neigh->parms, DELAY_PROBE_TIME))) {
+                    neigh->confirmed +
+                     NEIGH_VAR(neigh->parms, DELAY_REPROBE_TIME))) {
             neigh_dbg(2, "neigh %p is delayed\n", neigh);
             neigh->nud_state = NUD_DELAY;
             neigh->updated = jiffies;
             neigh_suspect(neigh);
             next = now + NEIGH_VAR(neigh->parms, DELAY_PROBE_TIME);
         } else {
             neigh_dbg(2, "neigh %p is suspected\n", neigh);
             neigh->nud_state = NUD_STALE;
             neigh->updated = jiffies;
             neigh_suspect(neigh);
             notify = 1;
         }
     } else ...

if DELAY_REPROBE_TIME is larger than "reachable_time", then
     the kernel will send out ARP probes when it is about
     to lose communication with a remote machine.
     This is what we need.

If it is smaller, then
     it will go from REACHABLE to STALE.

The initial value of DELAY_REPROBE_TIME needs to be settable in Kconfig
to allow the selection of functionality.

I am told that setting stuff using sysctl has a performance penalty, when
interfaces are dynamically created and deleted in hundreds.

Best Regards,
Ulf Samuelsson
  

On 04/10/2015 10:26 AM, Ulf Samuelsson wrote:
>
> On 03/12/2015 07:26 PM, David Miller wrote:
>> 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.
>
> 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 */
> +        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
>
>
>> I'm not applying this patch.
>>
>> -- 
>> To unsubscribe from this list: send the line "unsubscribe netdev" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
> -- 
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html