Re: [RFC PATCH 0/4] Faster ndo_fdb_dump for drivers with shared FDB

[Roopa Prabhu <roopa@nvidia.com>]

On 9/24/21 3:03 AM, Nikolay Aleksandrov wrote:
> On 24/09/2021 01:49, Vladimir Oltean wrote:
>>
[snip]
>> We don't, that's the premise, you didn't miss anything there. I mean in
>> the switchdev world, I see that an interrupt is the primary mechanism,
>> and we do have DSA switches which are theoretically capable of notifying
>> of new addresses, but not through interrupts but over Ethernet, of
>> course. Ocelot for example supports sending "learn frames" to the CPU -
>> these are just like flooded frames, except that a "flooded" frame is one
>> with unknown MAC DA, and a "learn" frame is one with unknown MAC SA.
>> I've been thinking whether it's worth adding support for learn frames
>> coming from Ocelot/Felix in DSA, and it doesn't really look like it.
>> Anyway, when the DSA tagging protocol receives a "learn" frame, it needs
>> to consume_skb() it, then trigger some sort of switchdev atomic notifier
>> for that MAC SA (SWITCHDEV_FDB_ADD_TO_BRIDGE), but sadly that is only
>> the beginning of a long series of complications, because we also need to
>> know when the hardware has fogotten it, and it doesn't look like
>> "forget" frames are a thing. So because the risk of having an address
>> expire in hardware while it is still present in software is kind of
>> high, the only option left is to make the hardware entry static, and
>> (a) delete it manually when the software entry expires
>> (b) set up a second alert which triggers a MAC SA has been received on a
>>      port other than the destination port which is pointed towards by an
>>      existing FDB entry. In short, "station migration alert". Because the
>>      FDB entry is static, we need to migrate it by hand, in software.
>> So it all looks kind of clunky. Whereas what we do now is basically
>> assume that the amount of frames with unknown MAC DA reaching the CPU
>> via flooding is more or less equal and equally distributed with the
>> frames with unknown MAC SA reaching the CPU. I have not yet encountered
>> a use case where the two are tragically different, in a way that could
>> be solved only with SWITCHDEV_FDB_ADD_TO_BRIDGE events and in no other way.
>>
>>
>> Anyway, huge digression, the idea was that DSA doesn't synchronize FDBs
>> and that is the reason in the first place why we have an .ndo_fdb_dump
>> implementation. Theoretically if all hardware drivers were perfect,
>> you'd only have the .ndo_fdb_dump implementation done for the bridge,
>> vxlan, things like that. So that is why I asked Roopa whether hacking up
>> rtnl_fdb_dump in this way, transforming it into a state machine even
>> more complicated than it already was, is acceptable. We aren't the
>> primary use case of it, I think.
>>
> Hi Vladimir,
> I glanced over the patches and the obvious question that comes first is have you
> tried pushing all of this complexity closer to the driver which needs it?
> I mean rtnl_fdb_dump() can already "resume" and passes all the necessary resume indices
> to ndo_fdb_dump(), so it seems to me that all of this can be hidden away. I doubt
> there will be a many users overall, so it would be nice to avoid adding the complexity
> as you put it and supporting it in the core. I'd imagine a hacked driver would simply cache
> the dump for some time while needed (that's important to define well, more below) and just
> return it for the next couple of devices which share it upon request, basically you'd have the
> same type of solution you've done here, just have the details hidden in the layer which needs it.
>
> Now the hard part seems to be figuring out when to finish in this case. Prepare should be a simple
> check if a shared fdb list is populated, finish would need to be inferred. One way to do that is
> based on a transaction/dump id which is tracked for each shared device and the last dump. Another
> is if you just pass a new argument to ndo_fdb_dump() if it's dumping a single device or doing a
> full dump, since that's the case that's difficult to differentiate. If you're doing a single
> dump - obviously you do a normal fdb dump without caching, if you're doing a full dump (all devices)
> then you need to check if the list is populated, if it is and this is the last device you need to free
> the cached shared list (finish phase). That would make the core change very small and would push the
> complexity to be maintained where it's needed. Actually you have the netlink_callback struct passed
> down to ndo_fdb_dump() so probably that can be used to infer if you're doing a single or multi- device
> dump already and there can be 0 core changes.
>
> Also one current downside with this set is the multiple walks over the net device list for a single dump,
> especially for setups with thousands of net devices. Actually, I might be missing something, but what
> happens if a dump is terminated before it's finished? Looks like the finish phase will never be reached.
> That would be a problem for both solutions, you might have to add a netlink ->done() callback anyway.

+1, the series conceptually looks good, but core fdb dump is already 
unnecessarily complex. moving this to the driver with indicators passed 
as flags

is preferred (you can possibly also mark a port as designated for shared 
fdb operations. again in the driver)