| Message ID | 20200902143750.28478-1-fw@strlen.de |
|---|---|
| State | Accepted, archived |
| Delegated to: | Matthieu Baerts |
| Headers | show |
| Series | [mptcp-next] mptcp: use _fast lock version in __mptcp_move_skbs | expand |
On Wed, 2020-09-02 at 16:37 +0200, Florian Westphal wrote: > The function is short and won't sleep, so this can use the _fast version. > > Signed-off-by: Florian Westphal <fw@strlen.de> > --- > net/mptcp/protocol.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c > index 3b621a99c8c5..0c2b506d17de 100644 > --- a/net/mptcp/protocol.c > +++ b/net/mptcp/protocol.c > @@ -1472,13 +1472,14 @@ static bool __mptcp_move_skbs(struct mptcp_sock *msk) > __mptcp_flush_join_list(msk); > do { > struct sock *ssk = mptcp_subflow_recv_lookup(msk); > + bool slowpath; > > if (!ssk) > break; > > - lock_sock(ssk); > + slowpath = lock_sock_fast(ssk); > done = __mptcp_move_skbs_from_subflow(msk, ssk, &moved); > - release_sock(ssk); > + unlock_sock_fast(ssk, slowpath); > } while (!done); > > if (mptcp_ofo_queue(msk) || moved > 0) { Good catch! the patch LGTM, thanks! Do you think it could useful do the same also in mptcp_sendmsg() and mptcp_worker()? (since mptcp_sendmsg_frag() does not sleep, should be possible...) Cheers, Paolo
Paolo Abeni <pabeni@redhat.com> wrote: > On Wed, 2020-09-02 at 16:37 +0200, Florian Westphal wrote: > > The function is short and won't sleep, so this can use the _fast version. > > > > Signed-off-by: Florian Westphal <fw@strlen.de> > > --- > > net/mptcp/protocol.c | 5 +++-- > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c > > index 3b621a99c8c5..0c2b506d17de 100644 > > --- a/net/mptcp/protocol.c > > +++ b/net/mptcp/protocol.c > > @@ -1472,13 +1472,14 @@ static bool __mptcp_move_skbs(struct mptcp_sock *msk) > > __mptcp_flush_join_list(msk); > > do { > > struct sock *ssk = mptcp_subflow_recv_lookup(msk); > > + bool slowpath; > > > > if (!ssk) > > break; > > > > - lock_sock(ssk); > > + slowpath = lock_sock_fast(ssk); > > done = __mptcp_move_skbs_from_subflow(msk, ssk, &moved); > > - release_sock(ssk); > > + unlock_sock_fast(ssk, slowpath); > > } while (!done); > > > > if (mptcp_ofo_queue(msk) || moved > 0) { > > Good catch! the patch LGTM, thanks! > > Do you think it could useful do the same also in mptcp_sendmsg() > and mptcp_worker()? (since mptcp_sendmsg_frag() does not sleep, should > be possible...) sendmsg path might sleep (it copies data from userspace). mptcp_worker might work, but I think we would need to make sure mptcp_ext_cache_refill() uses atomic allocations for this to work. Not sure this is a good idea. For work queue case I think its better to try to make it a true slowpath that isn't invoked frequently.
On Thu, 2020-09-03 at 00:01 +0200, Florian Westphal wrote: > Paolo Abeni <pabeni@redhat.com> wrote: > > On Wed, 2020-09-02 at 16:37 +0200, Florian Westphal wrote: > > > The function is short and won't sleep, so this can use the _fast version. > > > > > > Signed-off-by: Florian Westphal <fw@strlen.de> > > > --- > > > net/mptcp/protocol.c | 5 +++-- > > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > > > diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c > > > index 3b621a99c8c5..0c2b506d17de 100644 > > > --- a/net/mptcp/protocol.c > > > +++ b/net/mptcp/protocol.c > > > @@ -1472,13 +1472,14 @@ static bool __mptcp_move_skbs(struct mptcp_sock *msk) > > > __mptcp_flush_join_list(msk); > > > do { > > > struct sock *ssk = mptcp_subflow_recv_lookup(msk); > > > + bool slowpath; > > > > > > if (!ssk) > > > break; > > > > > > - lock_sock(ssk); > > > + slowpath = lock_sock_fast(ssk); > > > done = __mptcp_move_skbs_from_subflow(msk, ssk, &moved); > > > - release_sock(ssk); > > > + unlock_sock_fast(ssk, slowpath); > > > } while (!done); > > > > > > if (mptcp_ofo_queue(msk) || moved > 0) { > > > > Good catch! the patch LGTM, thanks! > > > > Do you think it could useful do the same also in mptcp_sendmsg() > > and mptcp_worker()? (since mptcp_sendmsg_frag() does not sleep, should > > be possible...) > > sendmsg path might sleep (it copies data from userspace). Whoops I missed that. > mptcp_worker might work, but I think we would need to make sure > mptcp_ext_cache_refill() uses atomic allocations for this to work. > > Not sure this is a good idea. For work queue case I think its better > to try to make it a true slowpath that isn't invoked frequently. Agreed, we really don't want to over-optimize the worker I'm fine with this, thanks! Paolo
Hi Florian, Paolo, On 03/09/2020 11:58, Paolo Abeni wrote: > On Thu, 2020-09-03 at 00:01 +0200, Florian Westphal wrote: >> Paolo Abeni <pabeni@redhat.com> wrote: >>> On Wed, 2020-09-02 at 16:37 +0200, Florian Westphal wrote: >>>> The function is short and won't sleep, so this can use the _fast version. >>>> >>>> Signed-off-by: Florian Westphal <fw@strlen.de> (...) >>> Good catch! the patch LGTM, thanks! >>> >>> Do you think it could useful do the same also in mptcp_sendmsg() >>> and mptcp_worker()? (since mptcp_sendmsg_frag() does not sleep, should >>> be possible...) >> >> sendmsg path might sleep (it copies data from userspace). > > Whoops I missed that. > >> mptcp_worker might work, but I think we would need to make sure >> mptcp_ext_cache_refill() uses atomic allocations for this to work. >> >> Not sure this is a good idea. For work queue case I think its better >> to try to make it a true slowpath that isn't invoked frequently. > > Agreed, we really don't want to over-optimize the worker > > I'm fine with this, thanks! Thank you for the patch and the review! Just added at the end of the series! Tests + export are in progress. Cheers, Matt
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c index 3b621a99c8c5..0c2b506d17de 100644 --- a/net/mptcp/protocol.c +++ b/net/mptcp/protocol.c @@ -1472,13 +1472,14 @@ static bool __mptcp_move_skbs(struct mptcp_sock *msk) __mptcp_flush_join_list(msk); do { struct sock *ssk = mptcp_subflow_recv_lookup(msk); + bool slowpath; if (!ssk) break; - lock_sock(ssk); + slowpath = lock_sock_fast(ssk); done = __mptcp_move_skbs_from_subflow(msk, ssk, &moved); - release_sock(ssk); + unlock_sock_fast(ssk, slowpath); } while (!done); if (mptcp_ofo_queue(msk) || moved > 0) {
The function is short and won't sleep, so this can use the _fast version. Signed-off-by: Florian Westphal <fw@strlen.de> --- net/mptcp/protocol.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)