On 18/04/18 16:07, Joachim Nilsson wrote:

On Wed, Apr 18, 2018 at 03:31:57PM +0300, Nikolay Aleksandrov wrote:

quoted

On 18/04/18 15:07, Joachim Nilsson wrote:

quoted

- 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! :)

quoted

quoted

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

quoted

quoted

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

We want to avoid sysfs in general, all of networking config and stats
are moving to netlink. It is better controlled and structured for such
changes, also provides nice interfaces for automatic  type checks etc.

Also (but a minor reason) there is no tree/entity in sysfs for the vlans
where to add this. It will either have to be a file which does some
format string hack (like us currently) or will need to add new tree for
them which I'd really like to avoid for the bridge.

quoted

quoted

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

Sure, that can be done easily via netlink. br_afspec() can decode any
additional per-vlan attributes and can be fairly easily extended.
Also after my vlan rhastable change, we have per-vlan context even today
(e.g. per-vlan stats use it) so we'll just extend that.

quoted

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

quoted

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