[openib-general] [PATCH] IB/ipoib get net_device from ipoib_neigh instead of linux neighbour

[Michael S. Tsirkin]

> >>------------------------------------------------------------------------------
> >>IPoIB uses a two layer neighboring scheme, such that for each struct neighbour
> >>whose device is an ipoib one, there is a struct ipoib_neigh buddy which is
> >>created on demand at the tx flow by an ipoib_neigh_alloc(skb->dst->neighbour)
> >>call.
> >>
> >>When using the bonding driver, neighbours are created by the net stack on behalf
> >>of the bonding (master) device. On the tx flow the bonding code gets an skb such
> >>that skb->dev points to the master device, it changes this skb to point on the
> >>slave device and calls the slave hard_start_xmit function.
> >>
> >>Combing these two flows, there is a hole if some code at ipoib
> >>(ipoib_neigh_destructor) assumes that for each struct neighbour it gets, n->dev
> >>is an ipoib device so for example netdev_priv(n->dev) would be of type struct
> >>ipoib_dev_priv.
> > 
> > 
> > Could you plese elaborate how ipoib_neigh_destructor comes to be called at all?
> > At what point does ipoib_neigh_setup_dev get called?
> > 
> > 
> The bond device uses its slave's neigh_setup function.
> Please look at line 19 below from the bonding code.
> static void bond_setup_by_slave(struct net_device *bond_dev,
>      11 +               struct net_device *slave_dev)
>      12 +{
>      13 +   bond_dev->hard_header           = slave_dev->hard_header;
>      14 +   bond_dev->rebuild_header        = slave_dev->rebuild_header;
>      15 +   bond_dev->hard_header_cache = slave_dev->hard_header_cache;
>      16 +   bond_dev->header_cache_update   = slave_dev->header_cache_update;
>      17 +   bond_dev->hard_header_parse = slave_dev->hard_header_parse;
>      18 +
>      19 +   bond_dev->neigh_setup       = slave_dev->neigh_setup;
>      20 +
>      21 +   bond_dev->type          = slave_dev->type;
>      22 +   bond_dev->hard_header_len   = slave_dev->hard_header_len;
>      23 +   bond_dev->addr_len      = slave_dev->addr_len;
>      24 +
>      25 +   memcpy(bond_dev->broadcast, slave_dev->broadcast,
>      26 +       slave_dev->addr_len);
>      27 +}

Another concern: assume that one device goes away (e.g. hotplug).
It seems that neighbours whose dev field point to another device, will not be destroyed.
Correct?

Therefore in your design, it seems that to_ipoib_neigh()->dev
will get us a pointer to device that has been removed already.

> >>To fix it, this patch adds a dev field to struct ipoib_neigh which is used
> >>instead of the struct neighbour dev one.
> > 
> > 
> > What I am concerned with is - if the master is not an IPoIB device,
> > what guarantee do we have that to_ipoib_neigh will return 0
> > and not part of an actual hardware address?
> > 
> > Without bonding, the reason is that dev points to an ipoib device,
> > so we know hw address is 20 bytes.
> > 
> 
> I guess you meant "if the slave is not an IPoIB device"...

Yes.

> The bond device doesn't allow devices of different types to be grouped
> together as its slaves.

I see.

> Furthermore, bond_setup_by_slave is called only for non
> Ethernet devices (we consider to change the logic to "called only for
> IPoIB devices just for safety).

Why is this necessary, BTW?

-- 
MST