From: Ivan Vecera <ivecera@redhat.com>
To: Jakub Kicinski <kuba@kernel.org>
Cc: Cong Wang <xiyou.wangcong@gmail.com>,
Linux Kernel Network Developers <netdev@vger.kernel.org>,
saeed@kernel.org, Jiri Pirko <jiri@resnulli.us>
Subject: Re: [PATCH net] team: postpone features update to avoid deadlock
Date: Fri, 22 Jan 2021 09:30:27 +0100 [thread overview]
Message-ID: <20210122093027.33b2e8e7@ceranb> (raw)
In-Reply-To: <20210121183452.47f0cffa@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
On Thu, 21 Jan 2021 18:34:52 -0800
Jakub Kicinski <kuba@kernel.org> wrote:
> On Thu, 21 Jan 2021 11:29:37 +0100 Ivan Vecera wrote:
> > On Wed, 20 Jan 2021 15:18:20 -0800
> > Cong Wang <xiyou.wangcong@gmail.com> wrote:
> > > On Wed, Jan 20, 2021 at 4:56 AM Ivan Vecera <ivecera@redhat.com> wrote:
> > > > Team driver protects port list traversal by its team->lock mutex
> > > > in functions like team_change_mtu(), team_set_rx_mode(),
>
> The set_rx_mode part can't be true, set_rx_mode can't sleep and
> team->lock is a mutex.
>
> > > > To fix the problem __team_compute_features() needs to be postponed
> > > > for these cases.
> > >
> > > Is there any user-visible effect after deferring this feature change?
> >
> > An user should not notice this change.
>
> I think Cong is right, can you expand a little on your assertion?
> User should be able to assume that the moment syscall returns the
> features had settled.
>
> What does team->mutex actually protect in team_compute_features()?
> All callers seem to hold RTNL at a quick glance. This is a bit of
> a long shot but isn't it just tryin to protect the iteration over
> ports which could be under RCU?
In fact the mutex could be removed at all because all port-list
writers are running under rtnl_lock, some readers like team_change_mtu()
or team_device_event() [notifier] as well and hot path readers are
protected by RCU.
I have discussed this with Jiri but he don't want to introduce any dependency
on RTNL to team as it was designed as RTNL-independent from beginning.
Anyway your idea to run team_compute_features under RCU could be fine
as subsequent __team_compute_features() cannot sleep...
Do you mean something like this?
diff --git a/drivers/net/team/team.c b/drivers/net/team/team.c
index c19dac21c468..dd7917cab2b1 100644
--- a/drivers/net/team/team.c
+++ b/drivers/net/team/team.c
@@ -992,7 +992,8 @@ static void __team_compute_features(struct team *team)
unsigned int dst_release_flag = IFF_XMIT_DST_RELEASE |
IFF_XMIT_DST_RELEASE_PERM;
- list_for_each_entry(port, &team->port_list, list) {
+ rcu_read_lock();
+ list_for_each_entry_rcu(port, &team->port_list, list) {
vlan_features = netdev_increment_features(vlan_features,
port->dev->vlan_features,
TEAM_VLAN_FEATURES);
@@ -1006,6 +1007,7 @@ static void __team_compute_features(struct team *team)
if (port->dev->hard_header_len > max_hard_header_len)
max_hard_header_len = port->dev->hard_header_len;
}
+ rcu_read_unlock();
team->dev->vlan_features = vlan_features;
team->dev->hw_enc_features = enc_features | NETIF_F_GSO_ENCAP_ALL |
@@ -1020,9 +1022,7 @@ static void __team_compute_features(struct team *team)
static void team_compute_features(struct team *team)
{
- mutex_lock(&team->lock);
__team_compute_features(team);
- mutex_unlock(&team->lock);
netdev_change_features(team->dev);
}
Thanks for comments,
Ivan
next prev parent reply other threads:[~2021-01-22 8:39 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-01-20 12:23 [PATCH net] team: postpone features update to avoid deadlock Ivan Vecera
2021-01-20 13:40 ` Jiri Pirko
2021-01-20 23:18 ` Cong Wang
2021-01-21 10:29 ` Ivan Vecera
2021-01-22 2:34 ` Jakub Kicinski
2021-01-22 8:30 ` Ivan Vecera [this message]
2021-01-22 18:03 ` Jakub Kicinski
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=20210122093027.33b2e8e7@ceranb \
--to=ivecera@redhat.com \
--cc=jiri@resnulli.us \
--cc=kuba@kernel.org \
--cc=netdev@vger.kernel.org \
--cc=saeed@kernel.org \
--cc=xiyou.wangcong@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).