| Message ID | 20241021-net-mptcp-sched-lock-v1-2-637759cf061c@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mptcp: sched: fix some lock issues | expand |
On Mon, Oct 21, 2024 at 12:25:27PM +0200, Matthieu Baerts (NGI0) wrote: > mptcp_get_available_schedulers() needs to iterate over the schedulers' > list only to read the names: it doesn't modify anything there. > > In this case, it is enough to hold the RCU read lock, no need to combine > this with the associated spin lock. > > Fixes: 73c900aa3660 ("mptcp: add net.mptcp.available_schedulers") > Cc: stable@vger.kernel.org > Suggested-by: Paolo Abeni <pabeni@redhat.com> > Reviewed-by: Geliang Tang <geliang@kernel.org> > Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org> I do wonder if it would be more appropriate to route this via net-next (without a fixes tag) rather than via net. But either way this looks good to me. Reviewed-by: Simon Horman <horms@kernel.org> ...
Hi Simon, Thank you for the reviews! On 23/10/2024 14:21, Simon Horman wrote: > On Mon, Oct 21, 2024 at 12:25:27PM +0200, Matthieu Baerts (NGI0) wrote: >> mptcp_get_available_schedulers() needs to iterate over the schedulers' >> list only to read the names: it doesn't modify anything there. >> >> In this case, it is enough to hold the RCU read lock, no need to combine >> this with the associated spin lock. >> >> Fixes: 73c900aa3660 ("mptcp: add net.mptcp.available_schedulers") >> Cc: stable@vger.kernel.org >> Suggested-by: Paolo Abeni <pabeni@redhat.com> >> Reviewed-by: Geliang Tang <geliang@kernel.org> >> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org> > > I do wonder if it would be more appropriate to route this via net-next > (without a fixes tag) rather than via net. But either way this looks good > to me. Good point. On one hand, I marked it as a fix, because when working on the patch 1/3, we noticed these spin_(un)lock() were not supposed to be there in the first place. On the other hand, even it's fixing a small performance issue, it is not fixing a regression. I think it is easier to route this via -net, but I'm fine if it is applied in net-next. Cheers, Matt
On Wed, Oct 23, 2024 at 04:13:36PM +0200, Matthieu Baerts wrote: > Hi Simon, > > Thank you for the reviews! > > On 23/10/2024 14:21, Simon Horman wrote: > > On Mon, Oct 21, 2024 at 12:25:27PM +0200, Matthieu Baerts (NGI0) wrote: > >> mptcp_get_available_schedulers() needs to iterate over the schedulers' > >> list only to read the names: it doesn't modify anything there. > >> > >> In this case, it is enough to hold the RCU read lock, no need to combine > >> this with the associated spin lock. > >> > >> Fixes: 73c900aa3660 ("mptcp: add net.mptcp.available_schedulers") > >> Cc: stable@vger.kernel.org > >> Suggested-by: Paolo Abeni <pabeni@redhat.com> > >> Reviewed-by: Geliang Tang <geliang@kernel.org> > >> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org> > > > > I do wonder if it would be more appropriate to route this via net-next > > (without a fixes tag) rather than via net. But either way this looks good > > to me. > Good point. On one hand, I marked it as a fix, because when working on > the patch 1/3, we noticed these spin_(un)lock() were not supposed to be > there in the first place. On the other hand, even it's fixing a small > performance issue, it is not fixing a regression. > > I think it is easier to route this via -net, but I'm fine if it is > applied in net-next. Understood. FTR, I don't feel strongly about this either way.
On Wed, 23 Oct 2024 16:13:36 +0200 Matthieu Baerts wrote: > On 23/10/2024 14:21, Simon Horman wrote: > > On Mon, Oct 21, 2024 at 12:25:27PM +0200, Matthieu Baerts (NGI0) wrote: > >> mptcp_get_available_schedulers() needs to iterate over the schedulers' > >> list only to read the names: it doesn't modify anything there. > >> > >> In this case, it is enough to hold the RCU read lock, no need to combine > >> this with the associated spin lock. > >> > >> Fixes: 73c900aa3660 ("mptcp: add net.mptcp.available_schedulers") > >> Cc: stable@vger.kernel.org > >> Suggested-by: Paolo Abeni <pabeni@redhat.com> > >> Reviewed-by: Geliang Tang <geliang@kernel.org> > >> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org> > > > > I do wonder if it would be more appropriate to route this via net-next > > (without a fixes tag) rather than via net. But either way this looks good > > to me. > Good point. On one hand, I marked it as a fix, because when working on > the patch 1/3, we noticed these spin_(un)lock() were not supposed to be > there in the first place. On the other hand, even it's fixing a small > performance issue, it is not fixing a regression. > > I think it is easier to route this via -net, but I'm fine if it is > applied in net-next. I agree with Simon's initial response. Let's not blur the lines. Please re-queue for net-next, I'll apply the rest. BTW thanks a lot for proactively fixing the CONFIG_PROVE_RCU_LIST splats!
diff --git a/net/mptcp/sched.c b/net/mptcp/sched.c index 78ed508ebc1b8dd9f0e020cca1bdd86f24f0afeb..df7dbcfa3b71370cc4d7e4e4f16cc1e41a50dddf 100644 --- a/net/mptcp/sched.c +++ b/net/mptcp/sched.c @@ -60,7 +60,6 @@ void mptcp_get_available_schedulers(char *buf, size_t maxlen) size_t offs = 0; rcu_read_lock(); - spin_lock(&mptcp_sched_list_lock); list_for_each_entry_rcu(sched, &mptcp_sched_list, list) { offs += snprintf(buf + offs, maxlen - offs, "%s%s", @@ -69,7 +68,6 @@ void mptcp_get_available_schedulers(char *buf, size_t maxlen) if (WARN_ON_ONCE(offs >= maxlen)) break; } - spin_unlock(&mptcp_sched_list_lock); rcu_read_unlock(); }
mptcp_get_available_schedulers() needs to iterate over the schedulers' list only to read the names: it doesn't modify anything there. In this case, it is enough to hold the RCU read lock, no need to combine this with the associated spin lock. Fixes: 73c900aa3660 ("mptcp: add net.mptcp.available_schedulers") Cc: stable@vger.kernel.org Suggested-by: Paolo Abeni <pabeni@redhat.com> Reviewed-by: Geliang Tang <geliang@kernel.org> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org> --- net/mptcp/sched.c | 2 -- 1 file changed, 2 deletions(-)