Re: Possible to use both dev_mc_sync and __dev_mc_sync?

[Jay Vosburgh <jay.vosburgh@canonical.com>]

Alexander Duyck <alexanderduyck@fb.com> wrote:

>> -----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.

	Pretty much, yes.  The _sync_multiple versions are for the case
of a device cloning its entire ucast and/or mcast address set to
multiple subordinate devices, e.g., a bond or team to its interfaces.
I've not poked at this in a while, but if memory serves the bond / team
itself is one reference, and then each subordinate device adds a
refcount and a sync_cnt, so the usual case is refcount == sync_cnt + 1.

	I believe this test in __hw_addr_sync_multiple:

		if (ha->sync_cnt == ha->refcount) {
			__hw_addr_unsync_one(to_list, from_list, ha, addr_len);

	is for the "removed a HW address from bond / team, then resync
to subordinate interfaces" case. I.e., the bond / team's refcount has
been released, and the correct action is to remove the "no longer on
bond / team" HW address from the subordinate.

	-J

>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.

---
	-Jay Vosburgh, jay.vosburgh@canonical.com