active-backup/bonding with drivers not supporting set_mac_address()

active-backup/bonding with drivers not supporting set_mac_address()

[Or Gerlitz]

Looking on the linux bonding driver, it seems to unconditionally (*)
assume that the enslaved device supports the set_mac_address call.

>From reading the doc (Documentation/networking/bonding.txt) i understand that
it is **not** a must prerequisite for the active-backup mode, this is since
there is at most one active slave at each point of time and as the doc states:

	when a failover occurs in active-backup mode, bonding will issue
	one or more gratuitous ARPs on the newly active slave.

I'd like to get feedback from people here if you think it makes sense
to change the bonding driver to actually allow for active-backup/bonding
with network device drivers which does not support set_mac_address.

thanks,

Or Gerlitz.

(*) in the sense that it checks for each enslaved device and reports
an error if dev->set_mac_address is not supported and later use
dev_set_mac_address elsewhere in the code.

Re: active-backup/bonding with drivers not supporting set_mac_address()

[David Miller]

From: Or Gerlitz <ogerlitz@voltaire.com>
Date: Mon, 10 Jul 2006 16:43:36 +0300 (IDT)

> Looking on the linux bonding driver, it seems to unconditionally (*)
> assume that the enslaved device supports the set_mac_address call.
> 
> >From reading the doc (Documentation/networking/bonding.txt) i understand that
> it is **not** a must prerequisite for the active-backup mode, this is since
> there is at most one active slave at each point of time and as the doc states:
> 
> 	when a failover occurs in active-backup mode, bonding will issue
> 	one or more gratuitous ARPs on the newly active slave.
> 
> I'd like to get feedback from people here if you think it makes sense
> to change the bonding driver to actually allow for active-backup/bonding
> with network device drivers which does not support set_mac_address.

Blindly dereferencing the ->set_mac_address method is wrong, it
does seem.

Regardless of whether we add support for active-backup/bonding for
devices without this method, in the mean time the bonding
driver should check for whether that method is NULL and disallow
the bond configuration if it is.

Re: active-backup/bonding with drivers not supporting set_mac_address()

[Jay Vosburgh]

Or Gerlitz <ogerlitz@voltaire.com> wrote:

>Looking on the linux bonding driver, it seems to unconditionally (*)
>assume that the enslaved device supports the set_mac_address call.
>
>From reading the doc (Documentation/networking/bonding.txt) i understand that
>it is **not** a must prerequisite for the active-backup mode, this is since
>there is at most one active slave at each point of time and as the doc states:
>
>	when a failover occurs in active-backup mode, bonding will issue
>	one or more gratuitous ARPs on the newly active slave.

	I think you're misreading the documentation a bit.  Some
specific modes (balance-alb, for example) require that the slave device
driver support set_mac_address while the device is up.  Many device
drivers will only allow MAC changes while the device is down; those
drivers won't work with the alb mode.

	The active-backup and other modes have always needed a set_mac
functionality, but they don't require that the device be up when
changing the MAC (those modes set the mac for a slave one time, during
the enslavement process, and the device is down when this is done).

	The purpose of the gratuitous ARP is to update switch forwarding
tables (i.e., announce that the device has moved from one port to
another), not to announce a change of MAC address.

>I'd like to get feedback from people here if you think it makes sense
>to change the bonding driver to actually allow for active-backup/bonding
>with network device drivers which does not support set_mac_address.

	What network device drivers are there that don't permit ever
changing the device MAC address?

	I think changing active-backup (as an example here) to cause a
MAC change during failover will have some side effects and secondary
requirements that aren't necessarily obvious.  For example, the MAC of
the bond itself will change during failover; I'm not sure right offhand
what effect if any this might have on third parties.

	-J

---
	-Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com

Re: active-backup/bonding with drivers not supporting set_mac_address()

[Jay Vosburgh]

David Miller <davem@davemloft.net> wrote:
[...]
>Regardless of whether we add support for active-backup/bonding for
>devices without this method, in the mean time the bonding
>driver should check for whether that method is NULL and disallow
>the bond configuration if it is.

	That's what it does; if a slave lacks a set_mac_address
function, the enslavement fails.  Changes to the MAC are done via
dev_set_mac_address.

	-J

---
	-Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com

Re: active-backup/bonding with drivers not supporting set_mac_address()

[David Miller]

From: Jay Vosburgh <fubar@us.ibm.com>
Date: Mon, 10 Jul 2006 11:29:14 -0700

> 	What network device drivers are there that don't permit ever
> changing the device MAC address?

Good question.

Look at a driver such as 3c509.c, for example.

Sure, it will get the default eth_mac_addr() software implementation,
but this is not going to do what you expect.

It may change the dev->dev_addr[] value, and change the MAC address
used on outgoing packets, but it is unlikely to make the device
actually receive packets with the MAC address in non-promiscuous
mode.

If the driver isn't setting up an explicit ->set_mac_address handler,
it isn't going to be programming the RX MAC of the chip and therefore
not recognize packets to that new MAC as destined for it.

Re: active-backup/bonding with drivers not supporting set_mac_address()

[David Miller]

From: Jay Vosburgh <fubar@us.ibm.com>
Date: Mon, 10 Jul 2006 11:31:48 -0700

> David Miller <davem@davemloft.net> wrote:
> [...]
> >Regardless of whether we add support for active-backup/bonding for
> >devices without this method, in the mean time the bonding
> >driver should check for whether that method is NULL and disallow
> >the bond configuration if it is.
> 
> 	That's what it does; if a slave lacks a set_mac_address
> function, the enslavement fails.  Changes to the MAC are done via
> dev_set_mac_address.

Ok, please see my other comments about the ramifications of
devices that end up with the default eth_mac_addr() software
implementation.

Re: active-backup/bonding with drivers not supporting set_mac_address()

[Jay Vosburgh]

David Miller <davem@davemloft.net> wrote:

>Look at a driver such as 3c509.c, for example.
[...]
>If the driver isn't setting up an explicit ->set_mac_address handler,
>it isn't going to be programming the RX MAC of the chip and therefore
>not recognize packets to that new MAC as destined for it.

	If I'm not mistaken, it looks like 3c509.c sets the MAC on the
chip from dev->dev_addr in el3_up():

	/* Set the station address in window 2 each time opened. */
	EL3WINDOW(2);

	for (i = 0; i < 6; i++)
		outb(dev->dev_addr[i], ioaddr + i);

	I don't have the spec for 3c509 handy, but 3c59x.c does the same
thing in vortex_up(), and its spec says the first 6 bytes of register
window 2 are the "StationAddress".

	So, I'm still wondering what drivers really never let you change
the MAC address.

	-J

---
	-Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com

Re: active-backup/bonding with drivers not supporting set_mac_address()

[Or Gerlitz]

Jay Vosburgh wrote:
> Or Gerlitz <ogerlitz@voltaire.com> wrote:
> 
>> Looking on the linux bonding driver, it seems to unconditionally (*)
>> assume that the enslaved device supports the set_mac_address call.
>>
>>From reading the doc (Documentation/networking/bonding.txt) i understand that
>> it is **not** a must prerequisite for the active-backup mode, this is since
>> there is at most one active slave at each point of time and as the doc states:
>>
>> 	when a failover occurs in active-backup mode, bonding will issue
>> 	one or more gratuitous ARPs on the newly active slave.
> 
> 	I think you're misreading the documentation a bit.  Some
> specific modes (balance-alb, for example) require that the slave device
> driver support set_mac_address while the device is up.  Many device
> drivers will only allow MAC changes while the device is down; those
> drivers won't work with the alb mode.
> 
> 	The active-backup and other modes have always needed a set_mac
> functionality, but they don't require that the device be up when
> changing the MAC (those modes set the mac for a slave one time, during
> the enslavement process, and the device is down when this is done).

Thanks for clarifying and sharpening this point; however thinking about 
set_mac_address for active-backup mode i could not convince myself that 
there is a **need** for that. However, at high level it does make some 
(much) sense that the bond net device exposes a mac which is not changed 
when the active slave changes.

> 	The purpose of the gratuitous ARP is to update switch forwarding
> tables (i.e., announce that the device has moved from one port to
> another), not to announce a change of MAC address.

I see, but it will have the affect of MAC address change announcement.

> 	I think changing active-backup (as an example here) to cause a
> MAC change during failover will have some side effects and secondary
> requirements that aren't necessarily obvious.  For example, the MAC of
> the bond itself will change during failover; I'm not sure right offhand
> what effect if any this might have on third parties.

I will be glad to know if you have more concrete ideas what would be the 
possible problems with such impl.

Or.

Re: active-backup/bonding with drivers not supporting set_mac_address()

[Or Gerlitz]

Jay Vosburgh wrote:
> Or Gerlitz <ogerlitz@voltaire.com> wrote:

> 	What network device drivers are there that don't permit ever
> changing the device MAC address?

I am thinking about what would it take to support active-backup/bonding 
for IP over Infiniband (see Documentation/infiniband/ipoib.txt).

The IPoIB MAC address (defined in RFC 4391 section 9.1.1) is 20 bytes 
long and made of the IB port GID (16 bytes), the IB QPN (Queue-Pair 
number) used by this device (3 bytes) plus 1 reserved byte.

The IPoIB MAC address is made of two unique hardware resource 
identifiers: the GID which identifies the HCA IP port and the number of 
the QP opened by the driver on this HCA/port.  Currently the driver does 
not support changing this MAC and i am also looking into whether its 
possible and what does it would take to have some abstraction layer 
implemented in the driver that would allow for external setting of the 
MAC address; that is an intermediate data structure that would translate 
from the MAC address exposed in the device and neigh structures to the 
actual IB hardware address.

However, again, if you would ACK an intension to patch the bonding 
driver such that it will not force usage of dev->set_mac_address for 
active-backup/bonding i would prefer to first produce and test a patch 
which does not change the IPoIB driver (drivers/infiniband/ulp/ipoib)

Or.

Below is the addr info on a node named dill with two ipoib devices ib0 
(192.168.10.118) and ib1 (not up) and the neigh info for an ipoib 
endpoint whose address is 192.168.10.57

so

the GID of the port associated with ib0 is 
fe:80:00:00:00:00:00:00:00:08:f1:04:03:96:51:dd

the GID of the port associated with ib1 is
fe:80:00:00:00:00:00:00:00:08:f1:04:03:96:51:de

the qpn used by ib0 is 00:04:04 (0x404)
the qpn used by ib1 is 00:04:05 (0x405)

the remote GID in this example is 
fe:80:00:00:00:00:00:00:00:08:f1:04:03:97:07:99

and the remote qpn is 0x404

dill # ip addr show ib0
6: ib0: <BROADCAST,MULTICAST,UP,10000> mtu 2044 qdisc pfifo_fast qlen 128
     link/[32] 00:00:04:04:fe:80:00:00:00:00:00:00:00:08:f1:04:03:96:51:dd
       brd 00:ff:ff:ff:ff:12:40:1b:ff:ff:00:00:00:00:00:00:ff:ff:ff:ff
     inet 192.168.10.118/24 brd 192.168.10.255 scope global ib0
     inet6 fe80::208:f104:396:51dd/64 scope link
        valid_lft forever preferred_lft forever

dill# ip addr show ib1
7: ib1: <BROADCAST,MULTICAST> mtu 2044 qdisc noop qlen 128
     link/[32] 00:00:04:05:fe:80:00:00:00:00:00:00:00:08:f1:04:03:96:51:de
       brd 00:ff:ff:ff:ff:12:40:1b:ff:ff:00:00:00:00:00:00:ff:ff:ff:ff

dill# ip neigh show 192.168.10.57
192.168.10.57 dev ib0 lladdr 
00:00:04:04:fe:80:00:00:00:00:00:00:00:08:f1:04:03:97:07:99 nud reachable