From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jay Vosburgh Subject: Re: [PATCH] net/bonding: Send more than one gratuitous ARP when slave takes over Date: Thu, 10 Apr 2008 13:57:49 -0700 Message-ID: <25865.1207861069@death> References: <47FE2D2D.2040504@voltaire.com> Cc: monis@voltaire.com, "netdev" , "Olga Stern" , "Or Gerlitz" To: "Waskiewicz Jr, Peter P" Return-path: Received: from e35.co.us.ibm.com ([32.97.110.153]:50371 "EHLO e35.co.us.ibm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756862AbYDJU5x (ORCPT ); Thu, 10 Apr 2008 16:57:53 -0400 Received: from d03relay04.boulder.ibm.com (d03relay04.boulder.ibm.com [9.17.195.106]) by e35.co.us.ibm.com (8.13.8/8.13.8) with ESMTP id m3AKvpw6004412 for ; Thu, 10 Apr 2008 16:57:51 -0400 Received: from d03av03.boulder.ibm.com (d03av03.boulder.ibm.com [9.17.195.169]) by d03relay04.boulder.ibm.com (8.13.8/8.13.8/NCO v8.7) with ESMTP id m3AKvorh194224 for ; Thu, 10 Apr 2008 14:57:50 -0600 Received: from d03av03.boulder.ibm.com (loopback [127.0.0.1]) by d03av03.boulder.ibm.com (8.12.11.20060308/8.13.3) with ESMTP id m3AKvohP023577 for ; Thu, 10 Apr 2008 14:57:50 -0600 In-reply-to: Sender: netdev-owner@vger.kernel.org List-ID: Waskiewicz Jr, Peter P wrote: >> @@ -104,6 +105,8 @@ struct bond_params bonding_defaults; >> >> module_param(max_bonds, int, 0); >> MODULE_PARM_DESC(max_bonds, "Max number of bonded devices"); >> +module_param(num_grat_arp, int, 0644); >> +MODULE_PARM_DESC(num_grat_arp, "Number of gratuitous ARP packet to >sned on failover"); > >Check your spelling here. s/sned/send/ > >Also, num_grat_arp is declared int, where send_grat_arp is s8. What >happens if you overflow send_grat_arp? Either your module parameter >needs to change to match what's in the bonding struct, or you needs some >bounds checking to validate your inputs. I'd vote for bounds checking, myself. I think a legal range of 0 - 255 is appropriate. >> @@ -1109,14 +1112,16 @@ void bond_change_active_slave(struct bon >> if (new_active && bond->params.fail_over_mac) >> memcpy(bond->dev->dev_addr, >new_active->dev->dev_addr, >> new_active->dev->addr_len); >> + bond->send_grat_arp = num_grat_arp; >> if (bond->curr_active_slave && >> test_bit(__LINK_STATE_LINKWATCH_PENDING, >> - >&bond->curr_active_slave->dev->state)) { >> + >&bond->curr_active_slave->dev->state)) >> dprintk("delaying gratuitous arp on %s\n", >> bond->curr_active_slave->dev->name); >> - bond->send_grat_arp = 1; >> - } else >> + else { >> bond_send_gratuitous_arp(bond); >> + bond->send_grat_arp--; >> + } > >Don't modify the if () else () formatting here. Kernel coding >guidelines read if any part of your if/else statement requires braces, >all sections need braces. So it still should be, with your changes: Agreed. [...] >> @@ -2144,7 +2149,7 @@ static int __bond_mii_monitor(struct bon >> dprintk("sending delayed gratuitous arp on on >%s\n", >> bond->curr_active_slave->dev->name); >> bond_send_gratuitous_arp(bond); >> - bond->send_grat_arp = 0; >> + bond->send_grat_arp--; > >Can this ever cause send_grat_arp to become less than zero? What will >happen if it does drop below zero? I think some bounds checking might >be needed here. I don't believe that send_grat_arp can go negative, because this block is entered on an "if (bond->send_grat_arp)" that's just above the context in the patch. The patch as a whole also needs a sysfs entry to permit modification and inspection of the num_grat_arp parameter, as is done for other parameters. -J --- -Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com