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

[Paritosh Potukuchi]

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?