From: Vlad Buslov <vladbu@nvidia•com>
To: Vladimir Oltean <olteanv@gmail•com>
Cc: Vladimir Oltean <vladimir.oltean@nxp•com>,
<netdev@vger•kernel.org>, Jakub Kicinski <kuba@kernel•org>,
"David S. Miller" <davem@davemloft•net>,
Roopa Prabhu <roopa@nvidia•com>,
Nikolay Aleksandrov <nikolay@nvidia•com>,
Andrew Lunn <andrew@lunn•ch>,
Florian Fainelli <f.fainelli@gmail•com>,
"Vivien Didelot" <vivien.didelot@gmail•com>,
Vadym Kochan <vkochan@marvell•com>,
"Taras Chornyi" <tchornyi@marvell•com>,
Jiri Pirko <jiri@nvidia•com>, Ido Schimmel <idosch@nvidia•com>,
<UNGLinuxDriver@microchip•com>,
Grygorii Strashko <grygorii.strashko@ti•com>,
Marek Behun <kabel@blackhole•sk>,
DENG Qingfang <dqfext@gmail•com>,
Kurt Kanzenbach <kurt@linutronix•de>,
Hauke Mehrtens <hauke@hauke-m•de>,
Woojung Huh <woojung.huh@microchip•com>,
Sean Wang <sean.wang@mediatek•com>,
Landen Chao <Landen.Chao@mediatek•com>,
"Claudiu Manoil" <claudiu.manoil@nxp•com>,
Alexandre Belloni <alexandre.belloni@bootlin•com>,
George McCollister <george.mccollister@gmail•com>,
Ioana Ciornei <ioana.ciornei@nxp•com>,
"Saeed Mahameed" <saeedm@nvidia•com>,
Leon Romanovsky <leon@kernel•org>,
Lars Povlsen <lars.povlsen@microchip•com>,
Steen Hegelund <Steen.Hegelund@microchip•com>,
Julian Wiedmann <jwi@linux•ibm.com>,
Karsten Graul <kgraul@linux•ibm.com>,
Heiko Carstens <hca@linux•ibm.com>,
Vasily Gorbik <gor@linux•ibm.com>,
Christian Borntraeger <borntraeger@de•ibm.com>,
Ivan Vecera <ivecera@redhat•com>, Jianbo Liu <jianbol@nvidia•com>,
Mark Bloch <mbloch@nvidia•com>, Roi Dayan <roid@nvidia•com>,
Tobias Waldekranz <tobias@waldekranz•com>,
Vignesh Raghavendra <vigneshr@ti•com>,
"Jesse Brandeburg" <jesse.brandeburg@intel•com>
Subject: Re: [PATCH v2 net-next 1/5] net: switchdev: move SWITCHDEV_FDB_{ADD,DEL}_TO_DEVICE to the blocking notifier chain
Date: Fri, 20 Aug 2021 10:36:41 +0300 [thread overview]
Message-ID: <ygnh35r4pnxy.fsf@nvidia.com> (raw)
In-Reply-To: <20210819231849.us3hxtszkwbo2nik@skbuf>
On Fri 20 Aug 2021 at 02:18, Vladimir Oltean <olteanv@gmail•com> wrote:
> Hi Vlad,
>
> On Thu, Aug 19, 2021 at 09:15:17PM +0300, Vlad Buslov wrote:
>> On Thu 19 Aug 2021 at 19:07, Vladimir Oltean <vladimir.oltean@nxp•com> wrote:
>> > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/rep/bridge.c b/drivers/net/ethernet/mellanox/mlx5/core/en/rep/bridge.c
>> > index 0c38c2e319be..ea7c3f07f6fe 100644
>> > --- a/drivers/net/ethernet/mellanox/mlx5/core/en/rep/bridge.c
>> > +++ b/drivers/net/ethernet/mellanox/mlx5/core/en/rep/bridge.c
>> > @@ -276,6 +276,55 @@ mlx5_esw_bridge_port_obj_attr_set(struct net_device *dev,
>> > return err;
>> > }
>> >
>> > +static struct mlx5_bridge_switchdev_fdb_work *
>> > +mlx5_esw_bridge_init_switchdev_fdb_work(struct net_device *dev, bool add,
>> > + struct switchdev_notifier_fdb_info *fdb_info,
>> > + struct mlx5_esw_bridge_offloads *br_offloads);
>> > +
>> > +static int
>> > +mlx5_esw_bridge_fdb_event(struct net_device *dev, unsigned long event,
>> > + struct switchdev_notifier_info *info,
>> > + struct mlx5_esw_bridge_offloads *br_offloads)
>> > +{
>> > + struct switchdev_notifier_fdb_info *fdb_info;
>> > + struct mlx5_bridge_switchdev_fdb_work *work;
>> > + struct mlx5_eswitch *esw = br_offloads->esw;
>> > + u16 vport_num, esw_owner_vhca_id;
>> > + struct net_device *upper, *rep;
>> > +
>> > + upper = netdev_master_upper_dev_get_rcu(dev);
>> > + if (!upper)
>> > + return 0;
>> > + if (!netif_is_bridge_master(upper))
>> > + return 0;
>> > +
>> > + rep = mlx5_esw_bridge_rep_vport_num_vhca_id_get(dev, esw,
>> > + &vport_num,
>> > + &esw_owner_vhca_id);
>> > + if (!rep)
>> > + return 0;
>> > +
>> > + /* only handle the event on peers */
>> > + if (mlx5_esw_bridge_is_local(dev, rep, esw))
>> > + return 0;
>>
>> This check is only needed for SWITCHDEV_FDB_DEL_TO_BRIDGE case. Here it
>> breaks the offload.
>
> Very good point, thanks for looking. I copied the entire atomic notifier
> handler and deleted the code which wasn't needed, but I actually took a
> break while converting mlx5, and so I forgot to delete this part when I
> came back.
>
>> > +
>> > + fdb_info = container_of(info, struct switchdev_notifier_fdb_info, info);
>> > +
>> > + work = mlx5_esw_bridge_init_switchdev_fdb_work(dev,
>> > + event == SWITCHDEV_FDB_ADD_TO_DEVICE,
>> > + fdb_info,
>>
>> Here FDB info can already be deallocated[1] since this is now executing
>> asynchronously and races with fdb_rcu_free() that is scheduled to be
>> called after rcu grace period by fdb_delete().
>
> I am incredibly lucky that you caught this, apparently I needed to add
> an msleep(1000) to see it as well.
>
> It is not the struct switchdev_notifier_fdb_info *fdb_info that gets
> freed under RCU. It is fdb_info->addr (the MAC address), since
> switchdev_deferred_enqueue only performs a shallow copy. I will address
> that in v3.
>
>> > @@ -415,9 +470,7 @@ static int mlx5_esw_bridge_switchdev_event(struct notifier_block *nb,
>> > /* only handle the event on peers */
>> > if (mlx5_esw_bridge_is_local(dev, rep, esw))
>> > break;
>>
>> I really like the idea of completely remove the driver wq from FDB
>> handling code, but I'm not yet too familiar with bridge internals to
>> easily determine whether same approach can be applied to
>> SWITCHDEV_FDB_{ADD|DEL}_TO_BRIDGE event after this series is accepted.
>> It seems that all current users already generate these events from
>> blocking context, so would it be a trivial change for me to do in your
>> opinion? That would allow me to get rid of mlx5_esw_bridge_offloads->wq
>> in our driver.
>
> If all callers really are in blocking context (and they do appear to be)
> you can even forgo the switchdev_deferred_enqueue that switchdev_fdb_add_to_device
> does, and just call_switchdev_blocking_notifiers() directly. Then you
> move the bridge handler from br_switchdev_event() to br_switchdev_blocking_event().
> It should be even simpler than this conversion.
Thanks for your advice! I'll start looking into it as soon as this
series is accepted.
next prev parent reply other threads:[~2021-08-20 7:37 UTC|newest]
Thread overview: 34+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-08-19 16:07 [PATCH v2 net-next 0/5] Make SWITCHDEV_FDB_{ADD,DEL}_TO_DEVICE blocking Vladimir Oltean
2021-08-19 16:07 ` [PATCH v2 net-next 1/5] net: switchdev: move SWITCHDEV_FDB_{ADD,DEL}_TO_DEVICE to the blocking notifier chain Vladimir Oltean
2021-08-19 18:15 ` Vlad Buslov
2021-08-19 23:18 ` Vladimir Oltean
2021-08-20 7:36 ` Vlad Buslov [this message]
2021-08-19 16:07 ` [PATCH v2 net-next 2/5] net: bridge: switchdev: make br_fdb_replay offer sleepable context to consumers Vladimir Oltean
2021-08-19 16:07 ` [PATCH v2 net-next 3/5] net: switchdev: drop the atomic notifier block from switchdev_bridge_port_{,un}offload Vladimir Oltean
2021-08-19 16:07 ` [PATCH v2 net-next 4/5] net: switchdev: don't assume RCU context in switchdev_handle_fdb_{add,del}_to_device Vladimir Oltean
2021-08-19 16:07 ` [PATCH v2 net-next 5/5] net: dsa: handle SWITCHDEV_FDB_{ADD,DEL}_TO_DEVICE synchronously Vladimir Oltean
2021-08-20 9:16 ` [PATCH v2 net-next 0/5] Make SWITCHDEV_FDB_{ADD,DEL}_TO_DEVICE blocking Ido Schimmel
2021-08-20 9:37 ` Vladimir Oltean
2021-08-20 16:09 ` Ido Schimmel
2021-08-20 17:06 ` Vladimir Oltean
2021-08-20 23:36 ` Nikolay Aleksandrov
2021-08-21 0:22 ` Vladimir Oltean
2021-08-22 6:48 ` Ido Schimmel
2021-08-22 9:12 ` Nikolay Aleksandrov
2021-08-22 13:31 ` Vladimir Oltean
2021-08-22 17:06 ` Ido Schimmel
2021-08-22 17:44 ` Vladimir Oltean
2021-08-23 10:47 ` Ido Schimmel
2021-08-23 11:00 ` Vladimir Oltean
2021-08-23 12:16 ` Ido Schimmel
2021-08-23 14:29 ` Vladimir Oltean
2021-08-23 15:18 ` Ido Schimmel
2021-08-23 15:42 ` Nikolay Aleksandrov
2021-08-23 15:42 ` Vladimir Oltean
2021-08-23 16:02 ` Ido Schimmel
2021-08-23 16:11 ` Vladimir Oltean
2021-08-23 16:23 ` Vladimir Oltean
2021-08-20 10:49 ` Vladimir Oltean
2021-08-20 16:11 ` Ido Schimmel
2021-08-21 19:09 ` Vladimir Oltean
2021-08-22 7:19 ` Ido Schimmel
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=ygnh35r4pnxy.fsf@nvidia.com \
--to=vladbu@nvidia$(echo .)com \
--cc=Landen.Chao@mediatek$(echo .)com \
--cc=Steen.Hegelund@microchip$(echo .)com \
--cc=UNGLinuxDriver@microchip$(echo .)com \
--cc=alexandre.belloni@bootlin$(echo .)com \
--cc=andrew@lunn$(echo .)ch \
--cc=borntraeger@de$(echo .)ibm.com \
--cc=claudiu.manoil@nxp$(echo .)com \
--cc=davem@davemloft$(echo .)net \
--cc=dqfext@gmail$(echo .)com \
--cc=f.fainelli@gmail$(echo .)com \
--cc=george.mccollister@gmail$(echo .)com \
--cc=gor@linux$(echo .)ibm.com \
--cc=grygorii.strashko@ti$(echo .)com \
--cc=hauke@hauke-m$(echo .)de \
--cc=hca@linux$(echo .)ibm.com \
--cc=idosch@nvidia$(echo .)com \
--cc=ioana.ciornei@nxp$(echo .)com \
--cc=ivecera@redhat$(echo .)com \
--cc=jesse.brandeburg@intel$(echo .)com \
--cc=jianbol@nvidia$(echo .)com \
--cc=jiri@nvidia$(echo .)com \
--cc=jwi@linux$(echo .)ibm.com \
--cc=kabel@blackhole$(echo .)sk \
--cc=kgraul@linux$(echo .)ibm.com \
--cc=kuba@kernel$(echo .)org \
--cc=kurt@linutronix$(echo .)de \
--cc=lars.povlsen@microchip$(echo .)com \
--cc=leon@kernel$(echo .)org \
--cc=mbloch@nvidia$(echo .)com \
--cc=netdev@vger$(echo .)kernel.org \
--cc=nikolay@nvidia$(echo .)com \
--cc=olteanv@gmail$(echo .)com \
--cc=roid@nvidia$(echo .)com \
--cc=roopa@nvidia$(echo .)com \
--cc=saeedm@nvidia$(echo .)com \
--cc=sean.wang@mediatek$(echo .)com \
--cc=tchornyi@marvell$(echo .)com \
--cc=tobias@waldekranz$(echo .)com \
--cc=vigneshr@ti$(echo .)com \
--cc=vivien.didelot@gmail$(echo .)com \
--cc=vkochan@marvell$(echo .)com \
--cc=vladimir.oltean@nxp$(echo .)com \
--cc=woojung.huh@microchip$(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