Re: [net-next PATCH v1 1/7] net: add generic PF_BRIDGE:RTM_ FDB hooks

[John Fastabend <john.r.fastabend@intel.com>]

On 4/11/2012 9:05 AM, Ben Hutchings wrote:
> On Wed, 2012-04-11 at 07:45 -0700, John Fastabend wrote:
>> On 4/10/2012 8:23 PM, Ben Hutchings wrote:
>>> On Mon, 2012-04-09 at 15:00 -0700, John Fastabend wrote:
>>> [...]
>>>> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
>>>> index 1f77540..05822e5 100644
>>>> --- a/include/linux/netdevice.h
>>>> +++ b/include/linux/netdevice.h
>>> [...]
>>>> @@ -905,6 +906,19 @@ struct netdev_fcoe_hbainfo {
>>>>   *	feature set might be less than what was returned by ndo_fix_features()).
>>>>   *	Must return >0 or -errno if it changed dev->features itself.
>>>>   *
>>>> + * int (*ndo_fdb_add)(struct ndmsg *ndm, struct net_device *dev,
>>>> + *		      unsigned char *addr, u16 flags)
>>>> + *	Adds an FDB entry to dev for addr. The ndmsg contains flags to indicate
>>>> + *	if the dev->master FDB should be updated or the devices internal FDB.
>>>
>>> I don't think the second sentence is helpful, as rtnl_fdb_add() will
>>> take care of those flags.
>>>
>>>> + * int (*ndo_fdb_del)(struct ndmsg *ndm, struct net_device *dev,
>>>> + *		      unsigned char *addr)
>>>> + *	Deletes the FDB entry from dev coresponding to addr. The ndmsg
>>>> + *	contains flags to indicate if the dev->master FDB should be
>>>> + *	updated or the devices internal FDB.
>>>
>>> Similarly here.
>>
>> agreed neither seem particularly helpful I'll remove them.
>>
>>>
>>>> + * int (*ndo_fdb_dump)(struct sk_buff *skb, struct netlink_callback *cb,
>>>> + *		       struct net_device *dev, int idx)
>>>> + *	Used to add FDB entries to dump requests. Implementers should add
>>>> + *	entries to skb and update idx with the number of entries.
>>>>   */
>>> [...
>>>> --- a/net/core/rtnetlink.c
>>>> +++ b/net/core/rtnetlink.c
>>> [...]
>>>> +static int rtnl_fdb_add(struct sk_buff *skb, struct nlmsghdr *nlh, void *arg)
>>>> +{
>>> [...]
>>>> +	err = -EOPNOTSUPP;
>>>
>>> So if NTF_MASTER and NTF_SELF are both set, we can quietly fall back to
>>> just setting one FDB?  Not sure that's really desirable though 
>>>
>>
>> It makes it easier to keep an embedded agent and the sw bridge in
>> sync if setting both flags adds the entry to both the SW bridge and
>> embedded bridge.
> 
> Yes, I agree with that.
> 
> [...]
>>> Wonder what we should do on error here if we've already successfully
>>> called ndo_fdb_add on the master?  Should we try to roll back the first
>>> addition?
>>>
>>
>> The problem with rolling back is the table is likely already updated and
>> traffic may already be being forwarded. So I think in this case the user
>> will have to query the device to learn what failed. It seems like the
>> simplest way to handle this. I think it is unwanted to have traffic being
>> forwarded one way for a short period of time then rolled back.
>>
>> The other idea I just had is we could clear the NTF_ bit in ndm_flags after
>> the successful add, del command. I believe the nlmsg gets sent back to the
>> user on error I would need to check on this.
> [...]
> 
> That sounds like a good way of doing it, assuming there's no
> compatibility issue.
> 
> Ben.
> 

I don't see any compat issues. Current applications shouldn't be setting
any flags and shouldn't be expecting any flags in the ack.

Also I think I need to add some notify hooks to help management. The
bridge code has fdb_notify() so I'll add a similar rtnl_fdb_notify() hook
here. Caught this while implementing some user space daemon code.

Thanks! v2 coming soon.