Message-ID: <556F4A51.3030908@cumulusnetworks.com>
Date:	Wed, 03 Jun 2015 11:41:21 -0700
From:	roopa <roopa@...ulusnetworks.com>
To:	Toshiaki Makita <toshiaki.makita1@...il.com>
CC:	Scott Feldman <sfeldma@...il.com>,
	Jamal Hadi Salim <jhs@...atatu.com>,
	David Miller <davem@...emloft.net>,
	Toshiaki Makita <makita.toshiaki@....ntt.co.jp>,
	Netdev <netdev@...r.kernel.org>,
	Jiří Pírko <jiri@...nulli.us>,
	"simon.horman@...ronome.com" <simon.horman@...ronome.com>
Subject: Re: [PATCH net-next 5/5] rocker: remove support for legacy VLAN ndo
 ops

On 6/3/15, 8:43 AM, Toshiaki Makita wrote:
> On 15/06/03 (水) 4:01, Scott Feldman wrote:
>> On Tue, Jun 2, 2015 at 9:58 AM, roopa <roopa@...ulusnetworks.com> wrote:
>>> On 6/2/15, 7:30 AM, Scott Feldman wrote:
>>>>
>>>> On Tue, Jun 2, 2015 at 4:43 AM, Jamal Hadi Salim <jhs@...atatu.com> 
>>>> wrote:
>>>>>
>>>>> On 06/02/15 03:10, Scott Feldman wrote:
>>>>>
>>>>>
>>>>>> Actually, we're now consistent with bridge man page which says 
>>>>>> master
>>>>>> is the default.
>>>>>>
>>>>>> Want we want, I believe, is to adjust what the man page says (and 
>>>>>> the
>>>>>> bridge vlan command itself), by making the default master and self.
>>>>>> The kernel and driver are fine, it's the default in the bridge 
>>>>>> command
>>>>>> that needs adjusting.  Once we do this, we'll be back to transparent
>>>>>> with software-only bridge.
>>>>>>
>>>>> Question to ask when looking at something of this nature:
>>>>> Will it work with no suprises if you used today's unmodified app?
>>>>> The default behavior shouldnt change and unfortunately it does here.
>>>>
>>>> The default behavior does change, yes, but there shouldn't be any
>>>> surprises even if using today's unmodified app.  The reason why is no
>>>> in-kernel driver is using ndo_bridge_setlink for VLAN setup.  The
>>>> three drivers that have ndo_bridge_setlink use if to set hwmode to
>>>> VEBA|VEB.  For VLAN setup, they use the (default master) bridge's
>>>> ndo_bridge_setlink->ndo_vlan_rx_add_vid.  If the default changes from
>>>> master to master|self, the bridge's
>>>> ndo_bridge_setlink->ndo_vlan_rx_add_vid is still called for those
>>>> driver's using ndo_vlan_rx_add_vid, and if they implement
>>>> ndo_bridge_setlink, they'll get called a second time but will noop
>>>> because there will be no IFLA_BRIDGE_MODE (hwmode) attr to process.
>>>>
>>>> So it comes down to two choices:
>>>>
>>>> 1) break ABI, which is inconsequential for in-kernel drivers and
>>>> preserve (iproute2) command transparency, or
>>>>
>>>> 2) embrace existing behavior which is consistent with man pages but
>>>> breaks command transparency for any driver implementing
>>>> ndo_bridge_setlink for VLAN setup, which currently is just rocker.  I
>>>> can see the DSA going down this path also based on another concurrent
>>>> thread.
>>>>
>>>> We're at option 2) right now.
>>>>
>>>>> It is not just iproute2 - since this is breaking ABI expectations.
>>>>> Looking at some app i wrote a while back based on analyzing kernel
>>>>> expectations at the time, I see the following logic:
>>>>>
>>>>> user can set master or self on command line.
>>>>> ...
>>>>> ....
>>>>> if (user DID NOT set master_on || user set self on)
>>>>>      then set self to on
>>>>>
>>>>> iow, current behavior:
>>>>>    01: master is only set if user explicitly asked.
>>>>>    11: master|self when user explicitly sets both
>>>>>    10: self is on by default when the user doesnt specify anything
>>>>>    00: and the last option is to have none set which is not
>>>>>        possible since we have defaults.
>>>>>
>>>>> cheers,
>>>>> jamal
>>>>>
>>>>>
>>>>> So this is very similar to iproute2 - if nothing is set
>>>>> it defaults to self.
>>>>
>>>> Ha, you're giving the behavior for "bridge fdb" command, where self is
>>>> the default.
>>>
>>>
>>> Oh...i did not realize this was the case either. Thats unfortunate.
>>>
>>>>
>>>> For "bridge link" and "bridge vlan", the default is master. The user
>>>> must explicitly specify "self" to act on the device side of the port.
>>>>
>>>> It's unfortunate the iproute2 defaults aren't consistent between
>>>> commands.  Maybe someone knows the history here and can explain.
>>>>
>>>>
>>>
>>> scott, this brings back the discussion you and i had over the revert 
>>> of my
>>> patches.. (commit id's at the end of this email)...
>>> which used to seamlessly offload to switchdev from bridge driver if 
>>> the port
>>> was a switch port (similar to stp state offload).
>>
>> Your patch tried to do the same thing that the bridge's
>> ndo_bridge_setlink/dellink is doing which is using the handler for
>> MASTER to also set SELF stuff, when SELF was not specified.  I don't
>> feel we should be overriding the application defaults in the kernel;
>> instead, we should change the application if we want different
>> behavior.  The kernel should treat the two sides of the port
>> independent (that's the basic algo in rtnetlink.c handlers for
>> MASTER/SELF things).  When you start doing kernel SELF things in the
>> MASTER path, the application has lost the ability to address each side
>> of the port independently.
>>
>>> 'self' used to exist before switchdev infra came in. My suggestion 
>>> was to
>>> use it where required...but not build the switchdev api on the 
>>> presence of
>>> 'self'. switchdev layer should be consistent across...all fib/fdb/neigh
>>> layers.
>>
>> I don't understand why you're bringing up fib/neigh because there is
>> no master|self form for those.
>>
>> The master|self objects are bridge fdb, settings, and vlans.  To be
>> clear, they are PF_BRIDGE handlers for:
>>
>> PF_BRIDGE:RTM_NEWNEIGH: add fdb entry
>> PF_BRIDGE:RTM_DELNEIGH: del fdb entry
>> PF_BRIDGE:RTM_SETLINK: set bridge setting or add VLAN
>> PF_BRIDGE:RTM_DELLINK: del VLAN
>>
>> The net/core/rtnetlink.c code for these _is_ consistent right now.
>> They all perform this same basic algorithm:
>>
>>      handler()
>>          if (!flags || flags & MASTER)
>>              if (master && master->op->foo)
>>                  master->op->foo();
>>          if (flags & SELF)
>>              if (port->op->foo)
>>                  port->op->foo();
>>
>> This lets the application set MASTER and/or SELF to address one or
>> both sides of the port.  The kernel only provides the mechanism; the
>> application decides which sides to address.
>>
>> Where we got into trouble is in the case of
>> PF_BRIDGE:RTM_SETLINK/RTM_DELLINK where the master->op->foo handler
>> calls into the member port's ndo_vlan_rx_add_vid(), which is really a
>> SELF operation because it's setting the VLAN for the device-side of
>> the port.  Setting the VLAN on the device side should have only been
>> done if SELF was specified.
>
> Bridge's vlan_filtering is handled in master->op->foo(), not in 
> port->op->foo().
> Can't we introduce another switchdev handler that performs MASTER 
> operation instead of calling SELF operation?
>
> br_afspec()
>  nbp_vlan_add()
>   netdev_switch_port_vlan_add()
>    rocker->ndo_switch_port_vlan_add() <- only used for MASTER operation
>
> I'm wondering why SELF operation (rocker->ndo_bridge_setlink()) does 
> what should be done in MASTER operation.
yes, this is what i have been alluding to (and I had commits which did 
this but got reverted).
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists