From: Hans S <schultz.hans@gmail.com>
To: Vladimir Oltean <olteanv@gmail.com>
Cc: "David S. Miller" <davem@davemloft.net>,
Jakub Kicinski <kuba@kernel.org>,
netdev@vger.kernel.org,
Hans Schultz <schultz.hans+netdev@gmail.com>,
Andrew Lunn <andrew@lunn.ch>,
Vivien Didelot <vivien.didelot@gmail.com>,
Florian Fainelli <f.fainelli@gmail.com>,
Eric Dumazet <edumazet@google.com>,
Paolo Abeni <pabeni@redhat.com>, Jiri Pirko <jiri@resnulli.us>,
Ivan Vecera <ivecera@redhat.com>, Roopa Prabhu <roopa@nvidia.com>,
Nikolay Aleksandrov <razor@blackwall.org>,
Shuah Khan <shuah@kernel.org>,
Daniel Borkmann <daniel@iogearbox.net>,
Ido Schimmel <idosch@nvidia.com>,
linux-kernel@vger.kernel.org, bridge@lists.linux-foundation.org,
linux-kselftest@vger.kernel.org
Subject: Re: [PATCH V3 net-next 3/4] net: dsa: mv88e6xxx: mac-auth/MAB implementation
Date: Wed, 6 Jul 2022 17:38:27 +0200 [thread overview]
Message-ID: <CAKUejP6NG_X-Bh_xeA2y4Wru2=pxgHaRMdsvMu8NATNxPVeQ7A@mail.gmail.com> (raw)
In-Reply-To: <20220706143339.iuwi23ktk53ihhb6@skbuf>
On Wed, Jul 6, 2022 at 4:33 PM Vladimir Oltean <olteanv@gmail.com> wrote:
>
> > >> @@ -919,6 +920,9 @@ static void mv88e6xxx_mac_link_down(struct dsa_switch *ds, int port,
> > >> if (err)
> > >> dev_err(chip->dev,
> > >> "p%d: failed to force MAC link down\n", port);
> > >> + else
> > >> + if (mv88e6xxx_port_is_locked(chip, port, true))
> > >> + mv88e6xxx_atu_locked_entry_flush(ds, port);
> > >
> > >This is superfluous, is it not? The bridge will transition a port whose
> > >link goes down to BR_STATE_DISABLED, which will make dsa_port_set_state()
> > >fast-age the dynamic FDB entries on the port, which you've already
> > >handled below.
> >
> > I removed this code, but then on link down the locked entries were not
> > cleared out. Something not as thought?
>
> What was the port's STP state before the link down event, and did it
> change after the link down?
The stp state is FORWARDING.
>
> If the STP state wasn't LEARNING or FORWARDING, there weren't supposed
> to be dynamic FDB entries on the port in the first place, so DSA says
> there's nothing to flush, and doesn't call dsa_port_fast_age().
> Are there dynamic FDB entries being installed on a port that isn't
> in a state that's supposed to learn? I guess the answer is yes.
> Is that what you want, or should the locked entries be recorded only in
> the LEARNING or FORWARDING states, otherwise discarded?
>
Learning is off as has been discussed, and I do want the locked
entries to be dynamic in the sense that the driver removes them after
the system ageing time has passed.
>
> What you actually want to say is: "mv88e6xxx_port_set_lock() is also
> called when the DSA port joins a bridge, due to the switchdev attribute
> replay logic present in dsa_port_switchdev_sync_attrs()".
>
> Which, by the way, is logic that you've added yourself, in commit
> b9e8b58fd2cb ("net: dsa: Include BR_PORT_LOCKED in the list of synced
> brport flags") ;)
>
> You are free to return early from mv88e6xxx_port_set_lock() if nothing has
> changed. The DSA layer doesn't keep track of the locked state of the
> port so it cannot deduce whether propagating to the switch driver is
> necessary or not.
>
I think I can safely call mv88e6xxx_atu_locked_entry_flush() from
mv88e6xxx_port_set_lock() when locked is off as the port setup for the
respective port must have been completed successfully.
> > When added they are added with bridge FDB flags: extern_learn offload
> > locked, with a SWITCHDEV_FDB_ADD_TO_BRIDGE event. So they are owned by
> > the driver.
> > When the driver deletes the locked entry the bridge FDB entry is
> > removes by the SWITCHDEV_FDB_DEL_TO_BRIDGE event from the driver. That
> > seems quite fair?
>
> I'm just pointing out that you left other (probably unintended) code
> paths for which the SWITCHDEV_FDB_DEL_TO_BRIDGE notifier is quite
> useless. I haven't yet looked at your newest revision to see what
> changed there.
>
I guess I should add a boolean to tell if
mv88e6xxx_atu_locked_entry_purge() should send a notification or not.
So that port_fdb_del() will not cause a SWITCHDEV_FDB_DEL_TO_BRIDGE
event.
> > > > > Why is the rtnl_unlock() outside the switch statement but the rtnl_lock() inside?
> > > > > Not to mention, the dsa_port_to_bridge_port() call needs to be under rtnl_lock().
> > > >
> > > > Just a small optimization as I also have another case of the switch
> > > > (only one switch case if
> > > > you didn't notice) belonging to the next patch set regarding dynamic
> > > > ATU entries.
> > >
> > > What kind of optimization are you even talking about? Please get rid of
> > > coding patterns like this, sorry.
> > >
> > Right!
>
> Right what? I'm genuinely curious what optimization are you talking about.
>
I am just confirming that what you wrote is correct, e.g. the
"Right!". So I have fixed that. :-)
>
> Just out of curiosity, are you even trying, are you looking at the
> difference using a monospace font?
>
> > Another issue...
> >
> > I have removed the timers as they are superfluous and now just use the
> > worker and jiffies. But I have found that the whole ageing time seems
> > to be broken on the 5.17 kernel I am running. I don't know if it has
> > been fixed, but the ageing timeout is supposed to be given in seconds.
> > Here is the output from various functions after the command "ip link
> > set dev br0 type bridge ageing_time 1500" (that is nominally 1500
> > seconds according to man page!):
> >
> > dsa_switch_ageing_time: ageing_time 10000, ageing_time_min 1000,
> > ageing_time_max 3825000
> > mv88e6xxx_set_ageing_time: set ageing time to 10000
> > br0: failed (err=-34) to set attribute (id=6)
> > dsa_switch_ageing_time: ageing_time 15000, ageing_time_min 1000,
> > ageing_time_max 3825000
> > mv88e6xxx_set_ageing_time: set ageing time to 15000
> >
> > The 15000 set corresponds to 150 seconds! (I hardcoded the dsa
> > ageing_time_min to 1000)
>
> Are you talking about this known problem, that the ageing time values in
> seconds need to be scaled up by a factor of USER_HZ when passed to the
> kernel?
> https://www.spinics.net/lists/netdev/msg672070.html
> https://www.spinics.net/lists/netdev/msg567332.html
It might be so, but there is another factor 10 which might be
regarding topology change as I understand. If I want a ageing timeout
of say 15 or 30 seconds, that hardly seems possible?
next prev parent reply other threads:[~2022-07-06 15:45 UTC|newest]
Thread overview: 54+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-05-24 15:21 [PATCH V3 net-next 0/4] Extend locked port feature with FDB locked flag (MAC-Auth/MAB) Hans Schultz
2022-05-24 15:21 ` [PATCH V3 net-next 1/4] net: bridge: add fdb flag to extent locked port feature Hans Schultz
2022-05-24 15:39 ` Nikolay Aleksandrov
2022-05-24 16:08 ` Hans Schultz
2022-05-24 16:21 ` Hans Schultz
2022-05-25 8:06 ` Nikolay Aleksandrov
2022-05-25 8:34 ` Hans Schultz
2022-05-25 8:38 ` Nikolay Aleksandrov
2022-05-25 9:11 ` Hans Schultz
2022-05-25 10:18 ` Nikolay Aleksandrov
2022-07-06 18:13 ` Vladimir Oltean
2022-07-06 19:38 ` Nikolay Aleksandrov
2022-07-06 20:21 ` Vladimir Oltean
2022-07-06 21:01 ` Nikolay Aleksandrov
2022-07-07 14:08 ` Nikolay Aleksandrov
2022-07-07 17:15 ` Vladimir Oltean
2022-07-07 17:26 ` Nikolay Aleksandrov
2022-07-08 6:38 ` Hans S
2022-05-26 14:13 ` Ido Schimmel
2022-05-27 8:52 ` Hans Schultz
2022-05-27 9:58 ` Ido Schimmel
2022-05-27 16:00 ` Hans Schultz
2022-05-31 9:34 ` Hans Schultz
2022-05-31 14:23 ` Ido Schimmel
2022-05-31 15:49 ` Hans Schultz
2022-06-02 9:17 ` Hans Schultz
2022-06-02 9:33 ` Nikolay Aleksandrov
2022-06-02 10:17 ` Hans Schultz
2022-06-02 10:30 ` Nikolay Aleksandrov
2022-06-02 10:39 ` Ido Schimmel
2022-06-02 11:36 ` Hans Schultz
2022-06-02 11:55 ` Ido Schimmel
2022-06-02 12:08 ` Hans Schultz
2022-06-02 12:18 ` Ido Schimmel
2022-06-02 13:27 ` Hans S
2022-05-24 15:21 ` [PATCH V3 net-next 2/4] net: switchdev: add support for offloading of fdb locked flag Hans Schultz
2022-06-27 16:06 ` Vladimir Oltean
2022-05-24 15:21 ` [PATCH V3 net-next 3/4] net: dsa: mv88e6xxx: mac-auth/MAB implementation Hans Schultz
2022-05-24 21:36 ` kernel test robot
2022-06-27 12:58 ` Hans S
2022-06-27 18:05 ` Vladimir Oltean
2022-06-28 12:26 ` Hans S
2022-07-05 15:05 ` Hans S
2022-07-06 13:28 ` Vladimir Oltean
2022-07-06 13:48 ` Hans S
2022-07-06 8:55 ` Vladimir Oltean
2022-07-06 10:12 ` Hans S
2022-07-06 14:23 ` Hans S
2022-07-06 14:33 ` Vladimir Oltean
2022-07-06 15:38 ` Hans S [this message]
2022-07-07 6:54 ` Hans S
2022-05-24 15:21 ` [PATCH V3 net-next 4/4] selftests: forwarding: add test of MAC-Auth Bypass to locked port tests Hans Schultz
2022-05-26 14:27 ` Ido Schimmel
2022-05-27 9:07 ` Hans Schultz
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='CAKUejP6NG_X-Bh_xeA2y4Wru2=pxgHaRMdsvMu8NATNxPVeQ7A@mail.gmail.com' \
--to=schultz.hans@gmail.com \
--cc=andrew@lunn.ch \
--cc=bridge@lists.linux-foundation.org \
--cc=daniel@iogearbox.net \
--cc=davem@davemloft.net \
--cc=edumazet@google.com \
--cc=f.fainelli@gmail.com \
--cc=idosch@nvidia.com \
--cc=ivecera@redhat.com \
--cc=jiri@resnulli.us \
--cc=kuba@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-kselftest@vger.kernel.org \
--cc=netdev@vger.kernel.org \
--cc=olteanv@gmail.com \
--cc=pabeni@redhat.com \
--cc=razor@blackwall.org \
--cc=roopa@nvidia.com \
--cc=schultz.hans+netdev@gmail.com \
--cc=shuah@kernel.org \
--cc=vivien.didelot@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
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;
as well as URLs for NNTP newsgroup(s).