Re: [ofa-general] Re: Re: Re: [PATCH V3 0/7] net/bonding: ADD IPoIB support for the bonding driver
It's always wrong to copy symbols from another module without referencing it. Michael, It seems like the preferred approach is to prevent ib_ipoib from being unloaded while bonding is on top it, right? It seems like it would handle all safety issues (not just neigh cleanup). - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [ofa-general] Re: Re: Re: [PATCH V3 0/7] net/bonding: ADD IPoIB support for the bonding driver
Quoting Moni Shoua [EMAIL PROTECTED]: Subject: Re: [ofa-general] Re: Re: Re: [PATCH V3 0/7] net/bonding: ADD IPoIB support for?the bonding driver It's always wrong to copy symbols from another module without referencing it. Michael, It seems like the preferred approach is to prevent ib_ipoib from being unloaded while bonding is on top it, right? It seems like it would handle all safety issues (not just neigh cleanup). Donnu about preferred, but that'll work I think. -- MST - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [ofa-general] Re: [PATCH V3 0/7] net/bonding: ADD IPoIB support for the bonding driver
Roland Dreier wrote: 1. When bonding enslaves an IPoIB device the bonding neighbor holds a reference to a cleanup function in the IPoIB drives. This makes it unsafe to unload the IPoIB module if there are bonding neighbors in the air. So, to avoid this race one must unload bonding before unloading IPoIB. I think we really want to resolve this somehow. Getting an oops by doing modprobe -r ipoib isn't that friendly. You are right and we want to resolve that. One way is to clean the neigh destructor function from all IPoIB neighs. The other way is to prevent ipoib unload if device is a slave or is referenced from somewhere else. I guess I would like an advice here. Also, what happened to the problem of having an address handle belonging to the wrong device on bond failover? Did you figure out a way to fix that one? This is what patch 2 handles. - R. ___ general mailing list [EMAIL PROTECTED] http://lists.openfabrics.org/cgi-bin/mailman/listinfo/general To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Re: [PATCH V3 0/7] net/bonding: ADD IPoIB support for the bonding driver
Quoting Moni Shoua [EMAIL PROTECTED]: Subject: Re: Re: [PATCH V3 0/7] net/bonding: ADD IPoIB support for?the bonding driver Roland Dreier wrote: 1. When bonding enslaves an IPoIB device the bonding neighbor holds a reference to a cleanup function in the IPoIB drives. This makes it unsafe to unload the IPoIB module if there are bonding neighbors in the air. So, to avoid this race one must unload bonding before unloading IPoIB. I think we really want to resolve this somehow. Getting an oops by doing modprobe -r ipoib isn't that friendly. You are right and we want to resolve that. One way is to clean the neigh destructor function from all IPoIB neighs. The other way is to prevent ipoib unload if device is a slave or is referenced from somewhere else. I guess I would like an advice here. I had this idea: Maybe we could use hard_header_cache/header_cache_update methods instead of neighbour cleanup calls. To do this, it is possible that we'll have to switch from storing pointers inside the neighbour to keeping an index there, but I expect the performance impact to be minimal. As a result, bonding would not have to copy pointers into ipoib module and module removal would get fixed. -- MST - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Re: Re: [PATCH V3 0/7] net/bonding: ADD IPoIB support for the bonding driver
Quoting Or Gerlitz [EMAIL PROTECTED]: Subject: Re: Re: Re: [PATCH V3 0/7] net/bonding: ADD IPoIB support for?the bonding driver Michael S. Tsirkin wrote: Maybe we could use hard_header_cache/header_cache_update methods instead of neighbour cleanup calls. To do this, it is possible that we'll have to switch from storing pointers inside the neighbour to keeping an index there, but I expect the performance impact to be minimal. As a result, bonding would not have to copy pointers into ipoib module and module removal would get fixed. To be precise, bonding will copy all the symbols it copies today from the slave module (ipoib), see bond_setup_by_slave() in patch 3/7, except for the neighbour cleanup callback which is copied through coping the neigh_setup function. Not really. This copying of symbols is something that you added, isn't it? So with this approach, it won't be needed. It's always wrong to copy symbols from another module without referencing it. -- MST - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [ofa-general] Re: Re: [PATCH V3 0/7] net/bonding: ADD IPoIB support for the bonding driver
Michael S. Tsirkin wrote: Maybe we could use hard_header_cache/header_cache_update methods instead of neighbour cleanup calls. To do this, it is possible that we'll have to switch from storing pointers inside the neighbour to keeping an index there, but I expect the performance impact to be minimal. As a result, bonding would not have to copy pointers into ipoib module and module removal would get fixed. To be precise, bonding will copy all the symbols it copies today from the slave module (ipoib), see bond_setup_by_slave() in patch 3/7, except for the neighbour cleanup callback which is copied through coping the neigh_setup function. Or. - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V3 0/7] net/bonding: ADD IPoIB support for the bonding driver
Michael S. Tsirkin wrote:
Quoting Or Gerlitz [EMAIL PROTECTED]:
To be precise, bonding will copy all the symbols it copies today from
the slave module (ipoib), see bond_setup_by_slave() in patch 3/7
Not really.
This copying of symbols is something that you added, isn't it?
So with this approach, it won't be needed.
It's always wrong to copy symbols from another module without
referencing it.
Its the --first-- time you make this comment, please suggest a different
approach, the relevant code is below.
+static void bond_setup_by_slave(struct net_device *bond_dev,
+ struct net_device *slave_dev)
+{
+ bond_dev-hard_header= slave_dev-hard_header;
+ bond_dev-rebuild_header= slave_dev-rebuild_header;
+ bond_dev-hard_header_cache = slave_dev-hard_header_cache;
+ bond_dev-header_cache_update = slave_dev-header_cache_update;
+ bond_dev-hard_header_parse = slave_dev-hard_header_parse;
+
+ bond_dev-neigh_setup = slave_dev-neigh_setup;
+
+ bond_dev-type = slave_dev-type;
+ bond_dev-hard_header_len = slave_dev-hard_header_len;
+ bond_dev-addr_len = slave_dev-addr_len;
+
+ memcpy(bond_dev-broadcast, slave_dev-broadcast,
+ slave_dev-addr_len);
+}
+
/* enslave device slave to bond device master */
int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
{
@@ -1351,6 +1371,24 @@ int bond_enslave(struct net_device *bond
goto err_undo_flags;
}
+ /* set bonding device ether type by slave - bonding netdevices are
+* created with ether_setup, so when the slave type is not ARPHRD_ETHER
+* there is a need to override some of the type dependent attribs/funcs.
+*
+* bond ether type mutual exclusion - don't allow slaves of dissimilar
+* ether type (eg ARPHRD_ETHER and ARPHRD_INFINIBAND) share the same
bond
+*/
+ if (bond-slave_cnt == 0) {
+ if (slave_dev-type != ARPHRD_ETHER)
+ bond_setup_by_slave(bond_dev, slave_dev);
+ } else if (bond_dev-type != slave_dev-type) {
+ printk(KERN_ERR DRV_NAME : %s ether type (%d) is different from
+ other slaves (%d), can not enslave it.\n,
slave_dev-name,
+ slave_dev-type, bond_dev-type);
+ res = -EINVAL;
+ goto err_undo_flags;
+ }
+
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V3 0/7] net/bonding: ADD IPoIB support for the bonding driver
Quoting Or Gerlitz [EMAIL PROTECTED]:
Subject: Re: [PATCH V3 0/7] net/bonding: ADD IPoIB support for?the?bonding
driver
Michael S. Tsirkin wrote:
Quoting Or Gerlitz [EMAIL PROTECTED]:
To be precise, bonding will copy all the symbols it copies today from
the slave module (ipoib), see bond_setup_by_slave() in patch 3/7
Not really.
This copying of symbols is something that you added, isn't it?
So with this approach, it won't be needed.
It's always wrong to copy symbols from another module without
referencing it.
Its the --first-- time you make this comment,
It's really a well known fact. That's where the crash
with modprobe -r comes from, right?
please suggest a different approach,
I don't know, really - if you want to access a module, you really must get
a reference to it, or to the device.
How about adding the module pointer to struct net_device?
the relevant code is below.
+static void bond_setup_by_slave(struct net_device *bond_dev,
+ struct net_device *slave_dev)
+{
+ bond_dev-hard_header = slave_dev-hard_header;
+ bond_dev-rebuild_header= slave_dev-rebuild_header;
+ bond_dev-hard_header_cache = slave_dev-hard_header_cache;
+ bond_dev-header_cache_update = slave_dev-header_cache_update;
+ bond_dev-hard_header_parse = slave_dev-hard_header_parse;
+
+ bond_dev-neigh_setup = slave_dev-neigh_setup;
+
+ bond_dev-type = slave_dev-type;
+ bond_dev-hard_header_len = slave_dev-hard_header_len;
+ bond_dev-addr_len = slave_dev-addr_len;
+
+ memcpy(bond_dev-broadcast, slave_dev-broadcast,
+ slave_dev-addr_len);
+}
+
Hmm, it seems that switching to hard_header_cache as I suggested won't help at
all.
I wonder: is bonding currently broken with devices that implement
hard_header_cache/header_cache_update?
--
MST
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V3 0/7] net/bonding: ADD IPoIB support for the bonding driver
Michael S. Tsirkin wrote:
Quoting Or Gerlitz [EMAIL PROTECTED]:
Michael S. Tsirkin wrote:
It's always wrong to copy symbols from another module without
referencing it.
Its the --first-- time you make this comment,
It's really a well known fact. That's where the crash
with modprobe -r comes from, right?
no, the crash --only-- comes from the neighbour cleanup function being
called while ipoib is now probed out of the kernel. The other symbols
are not problematic. I got positive feedback that this --is-- the
problem in the previous posts and from Roland during my Sonoma presentation.
please suggest a different approach,
I don't know, really - if you want to access a module, you really must get
a reference to it, or to the device.
How about adding the module pointer to struct net_device?
I think there used to be there owner field of type struct module and it
was removed... we will check that.
the relevant code is below.
+static void bond_setup_by_slave(struct net_device *bond_dev,
+ struct net_device *slave_dev)
+{
+ bond_dev-hard_header= slave_dev-hard_header;
+ bond_dev-rebuild_header= slave_dev-rebuild_header;
+ bond_dev-hard_header_cache = slave_dev-hard_header_cache;
+ bond_dev-header_cache_update = slave_dev-header_cache_update;
+ bond_dev-hard_header_parse = slave_dev-hard_header_parse;
+
+ bond_dev-neigh_setup = slave_dev-neigh_setup;
+
+ bond_dev-type = slave_dev-type;
+ bond_dev-hard_header_len = slave_dev-hard_header_len;
+ bond_dev-addr_len = slave_dev-addr_len;
+
+ memcpy(bond_dev-broadcast, slave_dev-broadcast,
+ slave_dev-addr_len);
+}
+
Hmm, it seems that switching to hard_header_cache as I suggested won't help at
all.
why? please clarify.
I wonder: is bonding currently broken with devices that implement
hard_header_cache/header_cache_update?
I don't think so. Note that bond_setup_by_slave is only called for
slaves whose ether type is --not-- Ethernet.
Or.
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH V3 0/7] net/bonding: ADD IPoIB support for the bonding driver
This patch series is the third version (see below link to V2) of the suggested changes to the bonding driver so it would be able to support non ARPHRD_ETHER netdevices for its High-Availability (active-backup) mode. The motivation is to enable the bonding driver on its HA mode to work with the IP over Infiniband (IPoIB) driver. With these patches I was able to enslave IPoIB netdevices and run TCP, UDP, IP (UDP) Multicast and ICMP traffic with fail-over and fail-back working fine. The working environment was the net-2.6 git. More over, as IPoIB is also the IB ARP provider for the RDMA CM driver which is used by native IB ULPs whose addressing scheme is based on IP (e.g. iSER, SDP, Lustre, NFSoRDMA, RDS), bonding support for IPoIB devices **enables** HA for these ULPs. This holds as when the ULP is informed by the IB HW on the failure of the current IB connection, it just need to reconnect, where the bonding device will now issue the IB ARP over the active IPoIB slave. This series also includes patches to the IPoIB driver that fix some fix some neighboring related issues. There are still 2 open issues here: 1. When bonding enslaves an IPoIB device the bonding neighbor holds a reference to a cleanup function in the IPoIB drives. This makes it unsafe to unload the IPoIB module if there are bonding neighbors in the air. So, to avoid this race one must unload bonding before unloading IPoIB. 2. Patch No. 7 is a workaround to a problem where gratuitous were not sent quite often. I didn't find something better that fixes this and I would appreciate advices and comments regarding it. However, this doesn't seem to me as an issue related exclusively to IPoIB. Links to earlier discussion: 1. A discussion in netdev about bonding support for IPoIB. http://lists.openwall.net/netdev/2006/11/30/46 2. A discussion in openfabrics regarding changes in the IPoIB that enable using it as a slave for bonding. http://lists.openfabrics.org/pipermail/general/2007-March/034033.html - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V3 0/7] net/bonding: ADD IPoIB support for the bonding driver
1. When bonding enslaves an IPoIB device the bonding neighbor holds a reference to a cleanup function in the IPoIB drives. This makes it unsafe to unload the IPoIB module if there are bonding neighbors in the air. So, to avoid this race one must unload bonding before unloading IPoIB. I think we really want to resolve this somehow. Getting an oops by doing modprobe -r ipoib isn't that friendly. Also, what happened to the problem of having an address handle belonging to the wrong device on bond failover? Did you figure out a way to fix that one? - R. - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
