[3/3] bonding: send ARP requests on interfaces other than the primary for tlb/alb

From: Andy Gospodarek <andy@greyhouse.net>

This patch sends ARP request on the correct destination output interface
rather than always sending them on the primary interface.  I've also
added some bits to make sure that the source and destination address in
the ARP header are correct since simply changing the source MAC and
output interface will not be that helpful.

Signed-off-by: Andy Gospodarek <andy@greyhouse.net>
Signed-off-by: Jay Vosburgh <fubar@us.ibm.com>

---
 drivers/net/bonding/bond_alb.c |   45 ++++++++++++++++++---------------------
 1 files changed, 21 insertions(+), 24 deletions(-)

Jay Vosburgh a écrit :
> From: Andy Gospodarek <andy@greyhouse.net>
> 
> This patch sends ARP request on the correct destination output interface
> rather than always sending them on the primary interface.  I've also
> added some bits to make sure that the source and destination address in
> the ARP header are correct since simply changing the source MAC and
> output interface will not be that helpful.
> 
> Signed-off-by: Andy Gospodarek <andy@greyhouse.net>
> Signed-off-by: Jay Vosburgh <fubar@us.ibm.com>
> 
> ---
>  drivers/net/bonding/bond_alb.c |   45 ++++++++++++++++++---------------------
>  1 files changed, 21 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/net/bonding/bond_alb.c b/drivers/net/bonding/bond_alb.c
> index 5cd0400..9148c75 100644
> --- a/drivers/net/bonding/bond_alb.c
> +++ b/drivers/net/bonding/bond_alb.c
> @@ -726,35 +726,32 @@ static struct slave *rlb_arp_xmit(struct sk_buff *skb, struct bonding *bond)
>  	struct arp_pkt *arp = arp_pkt(skb);
>  	struct slave *tx_slave = NULL;
>  
> -	if (arp->op_code == htons(ARPOP_REPLY)) {
> -		/* the arp must be sent on the selected
> -		* rx channel
> -		*/
> -		tx_slave = rlb_choose_channel(skb, bond);
> -		if (tx_slave) {
> -			memcpy(arp->mac_src,tx_slave->dev->dev_addr, ETH_ALEN);
> -		}
> -		pr_debug("Server sent ARP Reply packet\n");
> -	} else if (arp->op_code == htons(ARPOP_REQUEST)) {
> -		/* Create an entry in the rx_hashtbl for this client as a
> -		 * place holder.
> -		 * When the arp reply is received the entry will be updated
> -		 * with the correct unicast address of the client.
> -		 */
> -		rlb_choose_channel(skb, bond);
> +	/* Choose an output channel for the ARP frame */
> +	tx_slave = rlb_choose_channel(skb, bond);
>  
> -		/* The ARP relpy packets must be delayed so that
> -		 * they can cancel out the influence of the ARP request.
> -		 */

COmments should have the following form :
/*
 * This is a fine comment
 */

> +	/* If a valid interface is returned, make sure the sender and target MAC
> +	 * addresses are correct based on the interface that will be transmitting
> +	 * the frame. */
> +	if (tx_slave) {
> +		/* If sender mac is the bond's address, rewrite */
> +		if (!compare_ether_addr_64bits(arp->mac_src,bond->dev->dev_addr))
> +			memcpy(arp->mac_src,tx_slave->dev->dev_addr,bond->dev->addr_len);

Hmm, you use compare_ether_addr_64bits(), implying a fix ETH_ALEN (6 bytes) address, then
you memcpy( ..., ..., bond->dev->addr_len);
Why not use ETH_ALEN to help compiler ?
Also add a space after a comma

> +
> +		/* If target mac is the bond's address, rewrite */
> +		if (!compare_ether_addr_64bits(arp->mac_dst,bond->dev->dev_addr))
> +			memcpy(arp->mac_dst,tx_slave->dev->dev_addr,bond->dev->addr_len);
> +
> +	} else if (arp->op_code == htons(ARPOP_REQUEST)) {
> +		/* if tx_slave is NULL, the periodic ARP replies must
> +		 * be delayed so they can cancel out the influence of
> +		 * the ARP request. */
>  		bond->alb_info.rlb_update_delay_counter = RLB_UPDATE_DELAY;
>  
> -		/* arp requests are broadcast and are sent on the primary
> -		 * the arp request will collapse all clients on the subnet to
> +		/* ARP requests are broadcast and are sent on the primary
> +		 * the ARP request will collapse all clients on the subnet to
>  		 * the primary slave. We must register these clients to be

Could you reformulate this comment, it is not readable as is :

 /*
  * ARP requests are broadcast and are sent on the primary.
  * The ARP request will collapse all clients on the subnet to
  * the primary slave. We must register these clients to be
  * updated with their assigned MAC.
  */


> -		 * updated with their assigned mac.
> -		 */
> +		 * updated with their assigned MAC. */
>  		rlb_req_update_subnet_clients(bond, arp->ip_src);
> -		pr_debug("Server sent ARP Request packet\n");
>  	}
>  
>  	return tx_slave;

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

On 30-09-2009 07:20, Eric Dumazet wrote:
...
>> +	/* Choose an output channel for the ARP frame */
>> +	tx_slave = rlb_choose_channel(skb, bond);
>>  
>> -		/* The ARP relpy packets must be delayed so that
>> -		 * they can cancel out the influence of the ARP request.
>> -		 */
> 
> COmments should have the following form :
> /*
>  * This is a fine comment
>  */

It's "The preferred style for long (multi-line) comments [...]".

Jarek P.
--
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

Jarek Poplawski a écrit :
> On 30-09-2009 07:20, Eric Dumazet wrote:
> ...
>>> +	/* Choose an output channel for the ARP frame */
>>> +	tx_slave = rlb_choose_channel(skb, bond);
>>>  
>>> -		/* The ARP relpy packets must be delayed so that
>>> -		 * they can cancel out the influence of the ARP request.
>>> -		 */
>> COmments should have the following form :
>> /*
>>  * This is a fine comment
>>  */
> 
> It's "The preferred style for long (multi-line) comments [...]".

Yes I agree.

We keep old comments as they are, but for new code submission, we
try to follow common practices.
I know it seems odd, but in the end it helps code readability.

Of course, I would have not point this without another more interesting 
stuff in the review process :)


--
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: Eric Dumazet <eric.dumazet@gmail.com>
Date: Wed, 30 Sep 2009 07:20:23 +0200

> Jay Vosburgh a écrit :
>> -		/* The ARP relpy packets must be delayed so that
>> -		 * they can cancel out the influence of the ARP request.
>> -		 */
> 
> COmments should have the following form :
> /*
>  * This is a fine comment
>  */

I definitely prefer what is used in the patch, whereas your suggestion
wastes a precious line on the screen and doesn't look markedly better
at all.

Look at any TCP protocol source file and you'll see the consistent
application of:

	/* This form of
	 * comment.
	 */

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

David Miller a écrit :
> From: Eric Dumazet <eric.dumazet@gmail.com>
> Date: Wed, 30 Sep 2009 07:20:23 +0200
> 
>> Jay Vosburgh a écrit :
>>> -		/* The ARP relpy packets must be delayed so that
>>> -		 * they can cancel out the influence of the ARP request.
>>> -		 */
>> COmments should have the following form :
>> /*
>>  * This is a fine comment
>>  */
> 
> I definitely prefer what is used in the patch, whereas your suggestion
> wastes a precious line on the screen and doesn't look markedly better
> at all.
> 
> Look at any TCP protocol source file and you'll see the consistent
> application of:
> 
> 	/* This form of
> 	 * comment.
> 	 */
> 
> there.

No problem David, I dont want to waste time to discuss this kind of stuff,
but you know my own opinion, and I know yours as well !

As a non native, some things that are painful details for you definitly help
my brain to decode English sentences. Uppercase on first letter, final point,
spelling, ...

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