From: Jiri Pirko <jiri@resnulli•us>
To: "Arad, Ronen" <ronen.arad@intel•com>
Cc: Roopa Prabhu <roopa@cumulusnetworks•com>,
Scott Feldman <sfeldma@gmail•com>,
Netdev <netdev@vger•kernel.org>,
"David S. Miller" <davem@davemloft•net>
Subject: Re: [PATCH net-next] rocker: check for BRIDGE_FLAGS_SELF in bridge setlink handler
Date: Mon, 9 Mar 2015 17:07:54 +0100 [thread overview]
Message-ID: <20150309160754.GA2169@nanopsycho.lan> (raw)
In-Reply-To: <E4CD12F19ABA0C4D8729E087A761DC35067E6A8D@ORSMSX101.amr.corp.intel.com>
Mon, Mar 09, 2015 at 04:59:13PM CET, ronen.arad@intel•com wrote:
>
>
>>-----Original Message-----
>>From: netdev-owner@vger•kernel.org [mailto:netdev-owner@vger•kernel.org] On
>>Behalf Of Jiri Pirko
>>Sent: Monday, March 09, 2015 6:41 AM
>>To: Roopa Prabhu
>>Cc: Scott Feldman; Netdev; David S. Miller
>>Subject: Re: [PATCH net-next] rocker: check for BRIDGE_FLAGS_SELF in bridge
>>setlink handler
>>
>>Mon, Mar 09, 2015 at 01:12:47AM CET, roopa@cumulusnetworks•com wrote:
>>>On Sun, Mar 8, 2015 at 4:17 PM, Scott Feldman <sfeldma@gmail•com> wrote:
>>>
>>>> On Sun, Mar 8, 2015 at 7:19 AM, roopa <roopa@cumulusnetworks•com> wrote:
>>>> > On 3/6/15, 1:52 AM, Scott Feldman wrote:
>>>>
>>>> [cut]
>>>>
>>>> >> I'm saying port driver's ndo_bridge_setlink shouldn't be called unless
>>>> >> SELF is set. The port driver shouldn't have to check if it's SELF,
>>>> >> that's my main beef with this patch. If the app (iproute2) wants to
>>>> >> mirror an attribute, then it can set both MASTER and SELF. This is
>>>> >> consistent with ndo_fdb ops, where MASTER targets the bridge's FDB and
>>>> >> SELF targets the device's FDB.
>>>> >
>>>> >
>>>> > bridge setlink/dellink ndo sets/clears bridge port flags, vlans.
>>>> >
>>>> > 4 ways to the ndo op gets called (value in brackets is the bridge flags):
>>>> >
>>>> > 1. ndo_bridge_setlink ()
>>>> > 2. ndo_bridge_setlink (master)
>>>> > 3. ndo_bridge_setlink (self)
>>>> > 4. ndo_bridge_setlink (master | self)
>>>> >
>>>> >
>>>> > in case of 1) and 2) today, bridge driver also calls into the port driver
>>>> > (as part of NETIF_F_HW_SWITCH_OFFLOAD) because vlans and other port flags
>>>> > need to be offloaded if they are set on master (I understand that rocker
>>>> > does not use bridge setlink ndo op for vlans. But it is completely valid
>>>> for
>>>> > a switch driver to use the ndo setlink op for offloading vlan config)
>>>> >
>>>> > One thing i can do is the NETIF_F_HW_SWITCH_OFFLOAD case only applies in
>>>> > case 1 above. ie when bridge flags is zero.
>>>> >
>>>> > which leads to the following commands to disable learning in the bridge
>>>> > driver and enable it in hw,
>>>> >
>>>> > 1. bridge link learning on - enable learning in both rocker and bridge
>>>> > driver due to the NETIF_F_HW_SWITCH_OFFLOAD flag
>>>> > 2. bridge link learning off master - learning disable only in the bridge
>>>> > driver
>>>> >
>>>> > With this i think we hit a compromise, rocker setlink can be called when
>>>> > flags is zero, but we still get a valid sequence of commands to
>>>> > enable/disable certain port flags only in the bridge driver or port
>>>> driver
>>>> > without having an explicit self check in rocker.
>>>> >
>>>> > (my work laptop is broken..., I will re-post the patch tomorrow if you
>>>> are
>>>> > ok with the plan)
>>>>
>>>> I'm not OK with the plan; it doesn't address the issue. I'm saying
>>>> port driver's ndo_bridge_setlink shouldn't be called unless SELF is
>>>> set.
>>>>
>>>
>>>but, why scott ?. With l2, l3 and all other offloads, we try not to change
>>>user commands.
>>>Before switchdev 'self' was always used to go to nic drivers directly to
>>>program fdb.
>>>l2 never used the bridge driver.
>>
>>
>>I also think that is is fine that in case either master or self are not set,
>>driver ndo setlink is called as well (in case the offload bit is on). It
>>makes it transparent for user in case he does not care what in under. In
>>case he cares, he can specify master or self to achieve exactly what he
>>needs. I like it that way.
>>
>>
>It would be nice to have switch driver implementation that could work
>Consistently with and without the bridge driver. I expect use case where a bridge is only used when L3 is needed or to leverage L2 protocols that are implemented by the bridge driver (e.g. STP, IGMP snooping).
>
>If my driver can process setlink requests using SELF flag today, I'd like it
>to work the same when setlink is propagated down from a bridge master or
>from a team/bond master.
I just want to note that the name of the op is "ndo_bridge_setlink" and
it is specific for in-bridge use. Using it for team/bond seems very
wrong to me.
>
>There is current issue with notification.
>When SELF flag is set, the bridge driver does not offload to the port driver
>and notification is issued by rtnetlink.c by calling the port getlink ndo.
>This call, however, is done with zero filter_mask such that VLAN information
>is not included in the notification.
>When SELF flag is not set, bridge driver offloads to the port and issues the
>notification. In that case it sets the filter to VLAN_COMPRESSED.
>I don't see how I can get my driver to behave consistently with and without
>a bridge.
>The closest I can get notification with and without a bridge is for the
>driver to examine the flags. If SELF is set, the driver knows it got invoked
>directly from rtnetlink and it should notify VLAN setting as the subsequent
>notification triggered by rtnetlink won't.
>When the switch driver does not see SELF set it knows that it was invoked by
>The bridge driver which already takes care of notification including VLAN
>information.
>
>>
>>>
>>>with switchdev l2 offloads, we are using the bridge driver.
>>>But there is a need to turn off/on some attributes in hw independently of
>>>attributes in the kernel bridge driver. To that i had suggested we continue
>>>to use 'self'
>>>for those attributes without introducing a new hw mode.
>>>
>>>
>>>>
>>>> We have this currently, looking from the user's cmd:
>>>>
>>>> bridge link set calls into bridge and port
>>>> driver
>>>> bridge link set master calls into bridge and port driver
>>>> bridge link set self calls into port driver
>>>> bridge link set self master calls into bridge and port driver
>>>>
>>>> Your proposal above changes it to:
>>>>
>>>> bridge link set calls into bridge and port
>>>> driver
>>>> bridge link set master calls into bridge
>>>> bridge link set self calls into port driver
>>>> bridge link set self master calls into bridge and port driver
>>>>
>>>> Which is still no good because the default case (neither self or
>>>> master explicitly set) calls into the port driver. Before all of
>>>> these changes, we had:
>>>>
>>>> bridge link set calls into bridge
>>>> bridge link set master calls into bridge
>>>> bridge link set hwmode veb|vepa calls into port driver
>>>>
>>>> hwmode was a way to set SELF. We wanted to add "self" flag to command
>>>> to be more like bridge fdb cmd. I assumed the default case (bridge
>>>> set link) would continue to work as before. So we'd have:
>>>>
>>>> bridge link set calls into bridge
>>>> bridge link set master calls into bridge
>>>> bridge link set self calls into port driver
>>>> bridge link set self master calls into bridge and port driver
>>>>
>>>> But that's not what we got. Your changes changed the default case
>>>> (and the bridge link set master case) to call into both bridge and
>>>> port driver even though user didn't specify self.
>>>>
>>>
>>>I have made sure that no default case has changed. It works this way only
>>>when
>>>the driver advertises offload using NETIF_F_HW_SWITCH_OFFLOAD.
>>>Which means the driver is interested in involving the bridge driver in all
>>>l2 offloads
>>>(And this includes all bridge ndo ops setlink/dellink/stp/fdb).
>>>
>>>http://www.spinics.net/lists/netdev/msg314407.html
>>>
>>>
>>>
>>>
>>>>
>>>> How hard would it be to get back to original default behavior?
>>>>
>>>
>>>Am not sure which default behavior you are talking about. If you are
>>>talking about default behavior before switchdev,
>>>that has not changed. For switch devices which want the bridge driver
>>>involved and who advertise the NETIF_F_HW_SWITCH_OFFLOAD flag, this is new
>>>behavior. And rocker does advertise this flag today.
>>--
>>To unsubscribe from this list: send the line "unsubscribe netdev" in
>>the body of a message to majordomo@vger•kernel.org
>>More majordomo info at http://vger.kernel.org/majordomo-info.html
next prev parent reply other threads:[~2015-03-09 16:07 UTC|newest]
Thread overview: 38+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-03-04 0:15 [PATCH net-next] rocker: check for BRIDGE_FLAGS_SELF in bridge setlink handler roopa
2015-03-04 4:15 ` John Fastabend
2015-03-04 7:02 ` Scott Feldman
2015-03-04 8:51 ` roopa
2015-03-04 16:24 ` Scott Feldman
2015-03-05 0:31 ` roopa
2015-03-05 8:02 ` Jiri Pirko
2015-03-05 14:55 ` roopa
2015-03-05 20:06 ` Scott Feldman
2015-03-05 20:43 ` roopa
2015-03-05 21:40 ` roopa
2015-03-06 9:52 ` Scott Feldman
2015-03-08 14:19 ` roopa
2015-03-08 23:17 ` Scott Feldman
2015-03-09 0:20 ` roopa
[not found] ` <CAJieiUhHdXOZjWkb4s_GviLwzq5Gct-1o8xv8b-JeM46S4e-dg@mail.gmail.com>
2015-03-09 6:40 ` Jiri Pirko
2015-03-09 15:59 ` Arad, Ronen
2015-03-09 16:07 ` Jiri Pirko [this message]
2015-03-10 0:51 ` Arad, Ronen
2015-03-10 6:39 ` Jiri Pirko
2015-03-10 8:02 ` Arad, Ronen
2015-03-10 8:28 ` Jiri Pirko
2015-03-16 22:01 ` John Fastabend
2015-03-17 7:00 ` Jiri Pirko
2015-03-17 14:31 ` John Fastabend
2015-03-17 20:27 ` roopa
2015-03-18 0:16 ` John Fastabend
2015-03-18 6:29 ` roopa
2015-03-18 15:24 ` John Fastabend
2015-03-18 16:55 ` John Fastabend
2015-03-19 5:03 ` roopa
2015-03-19 5:49 ` Scott Feldman
2015-03-19 13:29 ` roopa
2015-03-19 13:59 ` John Fastabend
[not found] ` <CAJieiUhcdfGitY7rbG11Vt_Beemz8dy3=gKtvbyVLS8O0DkgNw@mail.gmail.com>
2015-03-09 23:23 ` Roopa Prabhu
2015-03-05 8:36 ` Jiri Pirko
2015-03-05 15:01 ` roopa
2015-03-05 15:09 ` roopa
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=20150309160754.GA2169@nanopsycho.lan \
--to=jiri@resnulli$(echo .)us \
--cc=davem@davemloft$(echo .)net \
--cc=netdev@vger$(echo .)kernel.org \
--cc=ronen.arad@intel$(echo .)com \
--cc=roopa@cumulusnetworks$(echo .)com \
--cc=sfeldma@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