Nikolay Aleksandrov <razor@xxxxxxxxxxxxx> writes: > On 26/01/2023 19:01, Petr Machata wrote: >> Note that the per-port-VLAN mcast_max_groups value gets reset when VLAN >> snooping is enabled. The reason for this is that while VLAN snooping is >> disabled, permanent entries can be added above the limit imposed by the >> configured maximum. Under those circumstances, whatever caused the VLAN >> context enablement, would need to be rolled back, adding a fair amount of >> code that would be rarely hit and tricky to maintain. At the same time, >> the feature that this would enable is IMHO not interesting: I posit that >> the usefulness of keeping mcast_max_groups intact across >> mcast_vlan_snooping toggles is marginal at best. >> > > Hmm, I keep thinking about this one and I don't completely agree. It > would be more user-friendly if the max count doesn't get reset when > mcast snooping is toggled. Imposing order of operations (first enable > snooping, then config max entries) isn't necessary and it makes sense > for someone to first set the limit and then enable vlan snooping. If you are talking about mcast_snooping, that can be disabled while mcast_vlan_snooping is enabled. So you can configure everything, then turn snooping on. If you are talking about configuring max while mcast_vlan_snooping is off, then I assumed one shouldn't touch the VLAN context if br_multicast_port_ctx_vlan_disabled(). So we would need to track the n and max in some other entity than in the multicast context. But maybe I'm wrong. > Also it would be consistent with port max entries, I'd prefer if we > have the same behaviour for port and vlan pmctxs. If we allow to set > any maximum at any time we don't need to rollback anything, also we > already always lookup vlans in br_multicast_port_vid_to_port_ctx() to > check if snooping is enabled so we can keep the count correct > regardless, the same as it's done for the ports. Keeping both limits > with consistent semantics seems better to me. The idea of requiring max >= current felt so natural to me that I didn't even check what mcast_hash_max was doing. Sure -- let's be consistent. This will incidentally make all the rollbacks go away, and happily makes sense WRT locking, too: since the relation between max and n is somewhat loose, we don't need to worry too much about sequencing inc-/dec-n vs. set-max.
