[PATCH v2] neighbour: confirm neigh entries when ARP packet is received

[PATCH v2] neighbour: confirm neigh entries when ARP packet is received

[Vasily Khoruzhick]

Update 'confirmed' timestamp when ARP packet is received. It shouldn't
affect locktime logic and anyway entry can be confirmed by any higher-layer
protocol. Thus it makes to sense not to confirm it when ARP packet is
received.

Fixes: 77d7123342 ("neighbour: update neigh timestamps iff update is
effective")

Signed-off-by: Vasily Khoruzhick <vasilykh@arista.com>
---
v2: - update comment to match new code.

 net/core/neighbour.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/net/core/neighbour.c b/net/core/neighbour.c
index aa19d86937af..56a554597db5 100644
--- a/net/core/neighbour.c
+++ b/net/core/neighbour.c
@@ -1180,6 +1180,12 @@ int neigh_update(struct neighbour *neigh, const u8 *lladdr, u8 new,
 		lladdr = neigh->ha;
 	}
 
+	/* Update confirmed timestamp for neighbour entry after we
+	 * received ARP packet even if it doesn't change IP to MAC binding.
+	 */
+	if (new & NUD_CONNECTED)
+		neigh->confirmed = jiffies;
+
 	/* If entry was valid and address is not changed,
 	   do not change entry state, if new one is STALE.
 	 */
@@ -1201,15 +1207,12 @@ int neigh_update(struct neighbour *neigh, const u8 *lladdr, u8 new,
 		}
 	}
 
-	/* Update timestamps only once we know we will make a change to the
+	/* Update timestamp only once we know we will make a change to the
 	 * neighbour entry. Otherwise we risk to move the locktime window with
 	 * noop updates and ignore relevant ARP updates.
 	 */
-	if (new != old || lladdr != neigh->ha) {
-		if (new & NUD_CONNECTED)
-			neigh->confirmed = jiffies;
+	if (new != old || lladdr != neigh->ha)
 		neigh->updated = jiffies;
-	}
 
 	if (new != old) {
 		neigh_del_timer(neigh);
-- 
2.18.0

Re: [PATCH v2] neighbour: confirm neigh entries when ARP packet is received

[Stephen Hemminger]

On Tue, 11 Sep 2018 11:04:06 -0700
Vasily Khoruzhick <vasilykh@arista.com> wrote:

> diff --git a/net/core/neighbour.c b/net/core/neighbour.c
> index aa19d86937af..56a554597db5 100644
> --- a/net/core/neighbour.c
> +++ b/net/core/neighbour.c
> @@ -1180,6 +1180,12 @@ int neigh_update(struct neighbour *neigh, const u8 *lladdr, u8 new,
>  		lladdr = neigh->ha;
>  	}
>  
> +	/* Update confirmed timestamp for neighbour entry after we
> +	 * received ARP packet even if it doesn't change IP to MAC binding.
> +	 */
> +	if (new & NUD_CONNECTED)
> +		neigh->confirmed = jiffies;

You might want to do:
	if ((new & NUD_CONNECTED) && neigh->confirmed != jiffies)
		neigh->confirmed = jiffies;

This avoid poisoning the cacheline with unnecessary write.

Re: [PATCH v2] neighbour: confirm neigh entries when ARP packet is received

[Vasiliy Khoruzhick]

On Tue, Sep 11, 2018 at 11:12 AM, Stephen Hemminger
<stephen@networkplumber.org> wrote:
> On Tue, 11 Sep 2018 11:04:06 -0700
> Vasily Khoruzhick <vasilykh@arista.com> wrote:
>
>> diff --git a/net/core/neighbour.c b/net/core/neighbour.c
>> index aa19d86937af..56a554597db5 100644
>> --- a/net/core/neighbour.c
>> +++ b/net/core/neighbour.c
>> @@ -1180,6 +1180,12 @@ int neigh_update(struct neighbour *neigh, const u8 *lladdr, u8 new,
>>               lladdr = neigh->ha;
>>       }
>>
>> +     /* Update confirmed timestamp for neighbour entry after we
>> +      * received ARP packet even if it doesn't change IP to MAC binding.
>> +      */
>> +     if (new & NUD_CONNECTED)
>> +             neigh->confirmed = jiffies;
>
> You might want to do:
>         if ((new & NUD_CONNECTED) && neigh->confirmed != jiffies)
>                 neigh->confirmed = jiffies;
>
> This avoid poisoning the cacheline with unnecessary write.

Sorry for duplicate - this time in plain text, so it should get
through lkml filter:

I don't think that it's performance-critical path, so this
optimization is unnecessary
and it doesn't improve code readability.

Re: [PATCH v2] neighbour: confirm neigh entries when ARP packet is received

[Sergei Shtylyov]

Hello!

On 9/11/2018 9:04 PM, Vasily Khoruzhick wrote:

> Update 'confirmed' timestamp when ARP packet is received. It shouldn't
> affect locktime logic and anyway entry can be confirmed by any higher-layer
> protocol. Thus it makes to sense not to confirm it when ARP packet is

    "Makes sense" or "makes no sense"?

> received.
> 
> Fixes: 77d7123342 ("neighbour: update neigh timestamps iff update is
> effective")
> 
> Signed-off-by: Vasily Khoruzhick <vasilykh@arista.com>
[...]

MBR, Sergei

Re: [PATCH v2] neighbour: confirm neigh entries when ARP packet is received

[David Miller]

From: Vasily Khoruzhick <vasilykh@arista.com>
Date: Tue, 11 Sep 2018 11:04:06 -0700

> Update 'confirmed' timestamp when ARP packet is received. It shouldn't
> affect locktime logic and anyway entry can be confirmed by any higher-layer
> protocol. Thus it makes to sense not to confirm it when ARP packet is
> received.
> 
> Fixes: 77d7123342 ("neighbour: update neigh timestamps iff update is
> effective")
> 
> Signed-off-by: Vasily Khoruzhick <vasilykh@arista.com>
> ---
> v2: - update comment to match new code.

Please fix the wording in this commit message, as per Sergei's
feedback.

Also, the Fixes: tag should be all one line (people grep for these
strings in the repository) and with no empty lines between it and the
rest of the tags such as Signed-off-by:

Thanks.