From mboxrd@z Thu Jan 1 00:00:00 1970 From: Joachim Nilsson Subject: Re: [RFC PATCH] net: bridge: multicast querier per VLAN support Date: Wed, 18 Apr 2018 15:07:19 +0200 Message-ID: <20180418130718.GA16044@troglobit> References: <20180418120713.GA10742@troglobit> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: netdev@vger.kernel.org, Stephen Hemminger , roopa To: Nikolay Aleksandrov Return-path: Received: from mail-lf0-f50.google.com ([209.85.215.50]:45895 "EHLO mail-lf0-f50.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752617AbeDRNHX (ORCPT ); Wed, 18 Apr 2018 09:07:23 -0400 Received: by mail-lf0-f50.google.com with SMTP id q5-v6so2532479lff.12 for ; Wed, 18 Apr 2018 06:07:22 -0700 (PDT) Content-Disposition: inline In-Reply-To: Sender: netdev-owner@vger.kernel.org List-ID: 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