From: roopa <roopa@cumulusnetworks•com>
To: Toshiaki Makita <toshiaki.makita1@gmail•com>
Cc: "Scott Feldman" <sfeldma@gmail•com>,
"Jamal Hadi Salim" <jhs@mojatatu•com>,
"David Miller" <davem@davemloft•net>,
"Toshiaki Makita" <makita.toshiaki@lab•ntt.co.jp>,
Netdev <netdev@vger•kernel.org>, "Jiří Pírko" <jiri@resnulli•us>,
"simon.horman@netronome•com" <simon.horman@netronome•com>
Subject: Re: [PATCH net-next 5/5] rocker: remove support for legacy VLAN ndo ops
Date: Wed, 03 Jun 2015 11:41:21 -0700 [thread overview]
Message-ID: <556F4A51.3030908@cumulusnetworks.com> (raw)
In-Reply-To: <556F209A.6090304@gmail.com>
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@cumulusnetworks•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@mojatatu•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).
next prev parent reply other threads:[~2015-06-03 18:41 UTC|newest]
Thread overview: 25+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-06-01 18:39 [PATCH net-next 0/5] rocker: enable by default untagged VLAN support sfeldma
2015-06-01 18:39 ` [PATCH net-next 1/5] rocker: zero allocate ports array sfeldma
2015-06-01 18:39 ` [PATCH net-next 2/5] rocker: cleanup vlan table on error adding vlan sfeldma
2015-06-01 18:39 ` [PATCH net-next 3/5] rocker: install untagged VLAN (vid=0) support for each port sfeldma
2015-06-01 18:39 ` [PATCH net-next 4/5] rocker: install/remove router MAC for untagged VLAN when joining/leaving bridge sfeldma
2015-06-01 18:39 ` [PATCH net-next 5/5] rocker: remove support for legacy VLAN ndo ops sfeldma
2015-06-02 4:51 ` Toshiaki Makita
2015-06-02 5:24 ` David Miller
2015-06-02 6:47 ` Toshiaki Makita
2015-06-02 7:10 ` Scott Feldman
2015-06-02 11:43 ` Jamal Hadi Salim
2015-06-02 14:30 ` Scott Feldman
2015-06-02 16:58 ` roopa
2015-06-02 19:01 ` Scott Feldman
2015-06-03 15:43 ` Toshiaki Makita
2015-06-03 18:41 ` roopa [this message]
2015-06-04 15:04 ` Toshiaki Makita
2015-06-04 15:09 ` roopa
2015-06-04 6:05 ` Scott Feldman
2015-06-04 14:35 ` Toshiaki Makita
2015-06-03 15:44 ` roopa
2015-06-03 12:08 ` Jamal Hadi Salim
2015-06-11 13:00 ` Jamal Hadi Salim
2015-06-11 18:25 ` Scott Feldman
2015-06-02 0:01 ` [PATCH net-next 0/5] rocker: enable by default untagged VLAN support David Miller
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=556F4A51.3030908@cumulusnetworks.com \
--to=roopa@cumulusnetworks$(echo .)com \
--cc=davem@davemloft$(echo .)net \
--cc=jhs@mojatatu$(echo .)com \
--cc=jiri@resnulli$(echo .)us \
--cc=makita.toshiaki@lab$(echo .)ntt.co.jp \
--cc=netdev@vger$(echo .)kernel.org \
--cc=sfeldma@gmail$(echo .)com \
--cc=simon.horman@netronome$(echo .)com \
--cc=toshiaki.makita1@gmail$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox