Re: [RFC PATCH] net: bridge: multicast querier per VLAN support

[Joachim Nilsson <troglobit@gmail.com>]

On Wed, Apr 18, 2018 at 03:31:57PM +0300, Nikolay Aleksandrov wrote:
> On 18/04/18 15:07, Joachim Nilsson wrote:
> > - First of all, is this patch useful to anyone
> Obviously to us as it's based on our patch. :-)
> We actually recently discussed what will be needed to make it acceptable to upstream.

Great! :)

> > - The current br_multicast.c is very complex.  The support for both IPv4
> >    and IPv6 is a no-brainer, but it also has #ifdef VLAN_FILTERING and
> >    'br->vlan_enabled' ... this has likely been discussed before, but if
> >    we could remove those code paths I believe what's left would be quite
> >    a bit easier to read and maintain.
> br->vlan_enabled has a wrapper that can be used without ifdefs, as does br_vlan_find()
> so in short - you can remove the ifdefs and use the wrappers,  they'll degrade to always
> false/null when vlans are disabled.

Thanks, I'll have a look at that and prepare an RFC v2!

> > - Many per-bridge specific multicast sysfs settings may need to have a
> >    corresponding per-VLAN setting, e.g. snooping, query_interval, etc.
> >    How should we go about that? (For status reporting I have a proposal)
> We'll have to add more to the per-vlan context, but yes it has to happen.
> It will be only netlink interface for config/retrieval, no sysfs.

Some settings are possible to do with sysfs, like multicast_query_interval
and ...

> > - Dito per-port specific multicast sysfs settings, e.g. multicast_router
> I'm not sure I follow this one, there is per-port mcast router config now ?

Sorry no, I meant we may want to add more per-VLAN settings when we get
this base patch merged.  Like router ports, we may want to be able to
set them per VLAN.

> Thanks for the effort, I see that you have done some of the required cleanups
> for this to be upstreamable, but as you've noted above we need to make it
> complete (with the per-vlan contexts and all).

There's definitely more work to be done.  Agreeing on a base set of changes
to start with is maybe the most important, as well as making it complete.

> I will review this patch in detail later and come back if there's anything.

Thank you so much for the quick feedback so far! :)

Cheers
 /Joachim