Message ID | 1254269731-7341-4-git-send-email-fubar@us.ibm.com |
---|---|
State | Deferred, archived |
Delegated to: | David Miller |
Headers | show |
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
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. - */ + /* 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); + + /* 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 - * 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;