[v2] ipv4: support for request type gratuitous ARP

On Sunday 17 January 2010 15:45:45 you wrote:

> 	May be I'm missing something but the description and
> the changed code do not match. You claim this patch now supports
> dest mac ff:ff:ff:ff:ff:ff while without the patch the
> 'skb->pkt_type != PACKET_HOST' check usually updates the
> existing entry with state NUD_STALE (neigh_update). What
> exactly is not handled correctly?
> 
> 	I think, the existing entry should be updated
> properly even without the patch, when sip=tip. May be
> there is unwanted ARP reply? It is not clear from this email. Can
> you give example why we need to override ip_route_input? What is
> the case that there is existing entry for sip that we want
> to update but for some reason we do not like ip_route_input
> result and entry is not updated? I don't think, ip_route_input
> will do something different when sip=tip=unicast address.
> This is not much different from the case when tip is IP
> from the same subnet as sip. There are only 2 'goto out' but
> both places update the entry depending on some policy.
> 
> 	Also, you call neigh_event_ns which creates new entry while
> RFC talks about updating existing entry: "if the receiving node
> has an entry for that IP address already in its ARP cache".
> We don't want to create new entry. After neigh_event_ns you
> jump again to update (2nd unneeded update).
> 
> 	As for arp_accept, it only allows creating new ARP
> entries on received reply, but when disabled it does not
> prevent updates for existing entries.
> 

Hi Julian,

Thanks for looking at this patch ! You are indeed correct, we don't need to do anything special when we want to update an existing ARP entry.

But I also want to be able to create a new ARP entry not only update an existing one (and we can do that with Linux today, but only with response type grat arp).

How about this new version?

[PATCH] ipv4: allow warming up the ARP cache with request type gratuitous ARP

If the per device ARP_ACCEPT option is enabled, currently we only allow
creating new ARP cache entries for response type gratuitous ARP.

Allowing gratuitous ARP to create new ARP entries (not only to update
existing ones) is useful when we want to avoid unnecessary delays for
the first packet of a stream.

This patch allows request type gratuitous ARP to create new ARP cache
entries as well. This is useful when we want to warm-up the ARP cache
entries for a large number of hosts on the same LAN.

Signed-off-by: Octavian Purdila <opurdila@ixiacom.com>
---
 net/ipv4/arp.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

Hello,

On Sun, 17 Jan 2010, Octavian Purdila wrote:

> On Sunday 17 January 2010 15:45:45 you wrote:
> 
> > 	May be I'm missing something but the description and
> > the changed code do not match. You claim this patch now supports
> > dest mac ff:ff:ff:ff:ff:ff while without the patch the
> > 'skb->pkt_type != PACKET_HOST' check usually updates the
> > existing entry with state NUD_STALE (neigh_update). What

> Hi Julian,
> 
> Thanks for looking at this patch ! You are indeed correct, we don't need to do anything special when we want to update an existing ARP entry.
> 
> But I also want to be able to create a new ARP entry not only update an existing one (and we can do that with Linux today, but only with response type grat arp).
> 
> How about this new version?

	Looks correct to me. You will save some CPU cycles
if the 'arp->ar_op == htons(ARPOP_REQUEST)' check is not added.
May be you also need to change arp_accept in 
Documentation/networking/ip-sysctl.txt to show that we do not 
differentiate Gratuitous ARP requests from replies.

> [PATCH] ipv4: allow warming up the ARP cache with request type gratuitous ARP
> 
> If the per device ARP_ACCEPT option is enabled, currently we only allow
> creating new ARP cache entries for response type gratuitous ARP.
> 
> Allowing gratuitous ARP to create new ARP entries (not only to update
> existing ones) is useful when we want to avoid unnecessary delays for
> the first packet of a stream.
> 
> This patch allows request type gratuitous ARP to create new ARP cache
> entries as well. This is useful when we want to warm-up the ARP cache
> entries for a large number of hosts on the same LAN.
> 
> Signed-off-by: Octavian Purdila <opurdila@ixiacom.com>
> ---
>  net/ipv4/arp.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
> 
> diff --git a/net/ipv4/arp.c b/net/ipv4/arp.c
> index 0787092..1940b4d 100644
> --- a/net/ipv4/arp.c
> +++ b/net/ipv4/arp.c
> @@ -907,7 +907,8 @@ static int arp_process(struct sk_buff *skb)
>  		   devices (strip is candidate)
>  		 */
>  		if (n == NULL &&
> -		    arp->ar_op == htons(ARPOP_REPLY) &&
> +		    (arp->ar_op == htons(ARPOP_REPLY) ||
> +		     (arp->ar_op == htons(ARPOP_REQUEST) && tip == sip)) &&
>  		    inet_addr_type(net, sip) == RTN_UNICAST)
>  			n = __neigh_lookup(&arp_tbl, &sip, dev, 1);
>  	}

Regards

--
Julian Anastasov <ja@ssi.bg>
--
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

From: Julian Anastasov <ja@ssi.bg>
Date: Sun, 17 Jan 2010 22:43:54 +0200 (EET)

> 	Looks correct to me. You will save some CPU cycles
> if the 'arp->ar_op == htons(ARPOP_REQUEST)' check is not added.
> May be you also need to change arp_accept in 
> Documentation/networking/ip-sysctl.txt to show that we do not 
> differentiate Gratuitous ARP requests from replies.

Octavian, please make the documentation fix for 'arp_accept'
and resubmit your patch.

It otherwise looks fine to me now.

Thanks.
--
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