Re: [PATCH v2 2/2] bonding: reuse neigh_setup from slave neigh_parms

Hi Kuniyuki,

The real user is qeth_l3_main.c only and it's not compiled
on 99% host.

We usually bail out at if (!slave_ops->ndo_neigh_setup),
which is called after your neigh_parms_lookup_dev(), and
there is no need to do O(n) traversal.

With 2K netns, it could incur unnecessary 4K traversal (lo
+ another dev) for each neigh creation, which is done under
RTNL or from interrupt context.

So, it will be a problem.

That makes sense. Thanks for the insight.

It does not help.  Just populating fields does not change the
loop detection logic.

True. Just populating fields would not suffice. Do you think
it would a good solution to modify the ndo_neigh_setup 
function?

Currently it takes a (netdev, parms) pair as an argument.

The way the ndo_neigh_setup is being used , both in stacked/
virtual devices and slave devices, is to expose the underlying
netdev's local neigh_setup function, through the parms argument.

There is a TODO in the bond_main.c file that suggests the 
following:

 /* TODO: find another way [1] to implement this.
  * Passing a zeroed structure is fragile,
  * but at least we do not pass garbage.
  *
  * [1] One way would be that ndo_neigh_setup() never touch
  *     struct neigh_parms, but propagate the new neigh_setup()
  *     back to ___neigh_create() / neigh_parms_alloc()
  */

As the TODO suggests, would it be a good idea to modify the 
ndo_neigh_setup function to be able to return the netdev's
local neigh_setup function.
This would solve the problem of passing a dummy parms structure.

typedef int (*neigh_setup_fn_t)(struct neighbour *);

static neigh_setup_fn_t ndo_neigh_setup(struct net_device *dev)
{
    return qeth_l3_neigh_setup_noarp;
}


Though the problem with this approach is that ndo_neigh_setup 
loses the flexibility to modify other fields in the parms 
structure.

What is your opinion on this?