Re: [patch net-next v2 06/10] bridge: introduce fdb offloading via switchdev

[Roopa Prabhu <roopa@cumulusnetworks.com>]

On 11/10/14, 9:30 AM, Andy Gospodarek wrote:
> On Mon, Nov 10, 2014 at 01:51:00PM +0000, Thomas Graf wrote:
>> On 11/10/14 at 09:15am, Jiri Pirko wrote:
>>> There are few problems in re-using this. It is netlink based so for calling
>>> it from bridge code, we would have to construct netlink message. But
>>> that could be probably changed.
>>> As you can see from the list of parameters, this is no longer about fdb (addr,
>>> vlanid) but this has been extended to something else. See vxlan code for
>>> what this is used for. I believe that fdb_add/del should be renamed to
>>> something else, perhaps l2neigh_add/del or something like that.
>>> The other problem is that fdb_add/del is currently used by various
>>> drivers for different purpose (adding macs to unicast list).
>> Can you elaborate a bit on the intended semantic differences between
>> the existing ndo_fdb_add() and ndo_sw_port_fdb_add()? I'm not sure we
>> need the sw_ prefix for this specific ndo.
>>
>> I completely agree that relying on Netlink is wrong because we'll have
>> in-kernel users of the API but I believe that existing ndo_fdb_add()
>> implementations in i40e, ixgbe, qlcnic and macvlan could use the new
>> API you propose.
> I also think the same API could be used quite easily on the current
> drivers that use it.
>
> I was looking at this earlier today and there are only 5 drivers
> (outside the bridge code) that support ndo_fdb_add.  The 3 hardware
> drivers and vxlan driver seem like they could use this new API.  The
> macvlan code appears to simply set the uc and mc lists, which seems like
> it could be done other ways -- confirmation from John Fastabend, Roopa,
> and mst would be good.
yes, that is correct. The macvlan code, when not set for passthru mode, 
seems to just program the uc-mc lists,
  which again get synched to the lowerdev (and possibly from lowerdev to 
hw in some cases).

I agree that it would be really nice if the existing api's can be made 
to work.
>> How about we rename the existing ndo_fdb_add() to ndo_neigh_add() as
>> you propose and convert vxlan over to it and have all others which don't
>> even depend on the Netlink attributes being passed in (i40e, ixgbe,
>> qlcnic, macvlan) use ndo_fdb_add() which would have the behaviour of your
>> proposed ndo_sw_port_fdb_add()?
> I would much rather see something like Thomas proposes here.  I know you
> would like to see these patches get included (I'm anxious to see better
> in-kernel offload support too!), but separate, possibly unnecessary
> APIs like this can get painful for driver maintainers (upstream and
> distro maintainers).
>
Ack!.