RE: Possible to use both dev_mc_sync and __dev_mc_sync?

[Alexander Duyck <alexanderduyck@fb.com>]

> -----Original Message-----
> From: Vladimir Oltean <olteanv@gmail.com>
> Sent: Monday, March 21, 2022 11:45 AM
> To: Alexander Duyck <alexanderduyck@fb.com>
> Cc: Jakub Kicinski <kuba@kernel.org>; Jiri Pirko <jiri@nvidia.com>; Florian
> Fainelli <f.fainelli@gmail.com>; netdev@vger.kernel.org
> Subject: Re: Possible to use both dev_mc_sync and __dev_mc_sync?
> 
> On Mon, Mar 21, 2022 at 08:42:59PM +0200, Vladimir Oltean wrote:
> > On Mon, Mar 21, 2022 at 06:37:05PM +0000, Alexander Duyck wrote:
> > > > -----Original Message-----
> > > > From: Vladimir Oltean <olteanv@gmail.com>
> > > > Sent: Monday, March 21, 2022 9:32 AM
> > > > To: Alexander Duyck <alexanderduyck@fb.com>; Jakub Kicinski
> > > > <kuba@kernel.org>; Jiri Pirko <jiri@nvidia.com>; Florian Fainelli
> > > > <f.fainelli@gmail.com>
> > > > Cc: netdev@vger.kernel.org
> > > > Subject: Possible to use both dev_mc_sync and __dev_mc_sync?
> > > I hadn't intended it to work this way. The expectation was that
> > > __dev_mc_sync would be used by hardware devices whereas
> dev_mc_sync
> > > was used by stacked devices such as vlan or macvlan.
> >
> > Understood, thanks for confirming.
> >
> > > Probably the easiest way to address it is to split things up so that
> > > you are using __dev_mc_sync if the switch supports mc filtering and
> > > have your dsa_slave_sync/unsync_mc call also push it down to the
> > > lower device, and then call dev_mc_sync after that so that if it
> > > hasn't already been pushed to the lower device it gets pushed.
> >
> > Yes, I have a patch with that change, just wanted to make sure I'm not
> > missing something. It's less efficient because now we need to check
> > whether dsa_switch_supports_uc_filtering() for each address, whereas
> > before we checked only once, before calling __dev_uc_add(). Oh well.
> >
> > > The assumption is that the lower device and the hardware would be
> > > synced in the same way. If we can't go that route we may have to
> > > look at implementing a different setup in terms of the reference
> > > counting such as what is done in __hw_addr_sync_multiple.
> >
> > So as mentioned, I haven't really understood the internals of the
> > reference/sync counting schemes being used here. But why are there
> > different implementations for dev_mc_sync() and
> dev_mc_sync_multiple()?
> 
> And on the same not of me not quite understanding what goes on, I wonder
> why some multicast addresses get passed both to the lower dev and to
> dsa_slave_sync_mc (which is why I didn't notice the problem in the first
> place), while others don't.

It all depends on the complexity of the setup. The standard __hw_addr_sync basically assumes you are operating in one of two states.
Sync: sync_cnt == 0, refcount == 1 -> sync_cnt = 1, refcount++
Unsync: sync_cnt == 1, refcount == 1 -> sync_cnt = 0, entry deleted

I myself am not all that familiar with the multiple approach either, however it seems to operate on the idea that the reference count should always be greater than the sync count. So the device will hold one reference and it will sync the address as long as it doesn't already exist in the lower devices address table based on the rules in __hw_addr_add_ex.

Also this might explain why some were synching while others weren't. It is possible that the lower dev already had the address present and as such was rejected for not being an exclusive address for this device.