From: Brian Haley <brian.haley@hp•com>
To: Jay Vosburgh <fubar@us•ibm.com>
Cc: Ben Hutchings <bhutchings@solarflare•com>,
David Miller <davem@davemloft•net>,
Andy Gospodarek <andy@greyhouse•net>,
Patrick McHardy <kaber@trash•net>,
netdev@vger•kernel.org
Subject: Re: [PATCH net-next-2.6 3/3] bonding,ipv4,ipv6,vlan: Handle NETDEV_BONDING_FAILOVER like NETDEV_NOTIFY_PEERS
Date: Fri, 15 Apr 2011 21:51:35 -0400 [thread overview]
Message-ID: <4DA8F627.5090109@hp.com> (raw)
In-Reply-To: <22334.1302913805@death>
On 04/15/2011 08:30 PM, Jay Vosburgh wrote:
> Ben Hutchings <bhutchings@solarflare•com> wrote:
>
>> It is undesirable for the bonding driver to be poking into higher
>> level protocols, and notifiers provide a way to avoid that. This does
>> mean removing the ability to configure reptitition of gratuitous ARPs
>> and unsolicited NAs.
>
> In principle I think this is a good thing (getting rid of some
> of those dependencies, duplicated code, etc).
>
> However, the removal of the multiple grat ARP and NAs may be an
> issue for some users. I don't know that we can just remove this (along
> with its API) without going through the feature removal process.
Right, I don't know how many people are using these, they might not be
happy, especially since specifying an unknown parameter will cause a
module load to fail:
--> modprobe bonding foobar=27
FATAL: Error inserting bonding (/lib/modules/2.6.32-31-generic/kernel/drivers/net/bonding/bonding.ko): Unknown symbol in module, or unknown parameter (see dmesg)
When these params are stuffed in /etc/modprobe.d/options, a reboot to
a kernel without them will cause some swearing :)
BTW, if this is accepted you need to update the documentation as well.
> As I recall, the multiple gratuitous ARP stuff was added for
> Infiniband, because it is dependent on the grat ARP for a smooth
> failover.
>
> There is also currently logic to check the linkwatch link state
> to wait for the link to go up prior to sending a grat ARP; this is also
> for IB.
>
> Brian Haley added the unsolicited NAs; I've added him to the cc
> so perhaps he (or somebody else) can comment on the necessity of keeping
> the ability to send multiple NAs.
I added it because in an IPv6-only environment I was seeing really long
failover times on bonds. I believe this was a customer-reported issue, so
there *might* be someone setting it, but I think my testing always showed
one was enough to wake-up the switch.
Is it useful to call netdev_bonding_change() multiple times from within
bond_change_active_slave(), like MAX(arp, na) times?
One comment below...
>> -/*
>> - * Kick out a gratuitous ARP for an IP on the bonding master plus one
>> - * for each VLAN above us.
>> - *
>> - * Caller must hold curr_slave_lock for read or better
>> - */
>> -static void bond_send_gratuitous_arp(struct bonding *bond)
>> -{
>> - struct slave *slave = bond->curr_active_slave;
>> - struct vlan_entry *vlan;
>> - struct net_device *vlan_dev;
>> -
>> - pr_debug("bond_send_grat_arp: bond %s slave %s\n",
>> - bond->dev->name, slave ? slave->dev->name : "NULL");
>> -
>> - if (!slave || !bond->send_grat_arp ||
>> - test_bit(__LINK_STATE_LINKWATCH_PENDING, &slave->dev->state))
>> - return;
>> -
>> - bond->send_grat_arp--;
>> -
>> - if (bond->master_ip) {
>> - bond_arp_send(slave->dev, ARPOP_REPLY, bond->master_ip,
>> - bond->master_ip, 0);
>> - }
>> -
>> - if (!bond->vlgrp)
>> - return;
>> -
>> - list_for_each_entry(vlan, &bond->vlan_list, vlan_list) {
>> - vlan_dev = vlan_group_get_device(bond->vlgrp, vlan->vlan_id);
>> - if (vlan->vlan_ip) {
>> - bond_arp_send(slave->dev, ARPOP_REPLY, vlan->vlan_ip,
>> - vlan->vlan_ip, vlan->vlan_id);
>> - }
>> - }
>> -}
Does your change also cover this case with multiple VLAN IDs? Is that covered in
the vlan.c code below?
>> diff --git a/net/8021q/vlan.c b/net/8021q/vlan.c
>> index b2ff70f..969e700 100644
>> --- a/net/8021q/vlan.c
>> +++ b/net/8021q/vlan.c
>> @@ -501,13 +501,14 @@ static int vlan_device_event(struct notifier_block *unused, unsigned long event,
>> return NOTIFY_BAD;
>>
>> case NETDEV_NOTIFY_PEERS:
>> + case NETDEV_BONDING_FAILOVER:
>> /* Propagate to vlan devices */
>> for (i = 0; i < VLAN_N_VID; i++) {
>> vlandev = vlan_group_get_device(grp, i);
>> if (!vlandev)
>> continue;
>>
>> - call_netdevice_notifiers(NETDEV_NOTIFY_PEERS, vlandev);
>> + call_netdevice_notifiers(event, vlandev);
>> }
>> break;
>> }
Thanks,
-Brian
next prev parent reply other threads:[~2011-04-16 1:51 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2011-04-15 23:47 [PATCH net-next-2.6 3/3] bonding,ipv4,ipv6,vlan: Handle NETDEV_BONDING_FAILOVER like NETDEV_NOTIFY_PEERS Ben Hutchings
2011-04-16 0:30 ` Jay Vosburgh
2011-04-16 1:51 ` Brian Haley [this message]
2011-04-16 2:53 ` Ben Hutchings
2011-04-18 19:09 ` Ben Hutchings
2011-04-19 1:32 ` Brian Haley
2011-04-19 14:56 ` Ben Hutchings
2011-04-19 19:12 ` Jay Vosburgh
2011-04-19 19:34 ` Brian Haley
2011-04-22 4:13 ` David Miller
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4DA8F627.5090109@hp.com \
--to=brian.haley@hp$(echo .)com \
--cc=andy@greyhouse$(echo .)net \
--cc=bhutchings@solarflare$(echo .)com \
--cc=davem@davemloft$(echo .)net \
--cc=fubar@us$(echo .)ibm.com \
--cc=kaber@trash$(echo .)net \
--cc=netdev@vger$(echo .)kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox