* [MPTCP] Re: [PATCH mptcp-next] mptcp: use _fast lock version in __mptcp_move_skbs
@ 2020-09-02 22:01 Florian Westphal
0 siblings, 0 replies; 4+ messages in thread
From: Florian Westphal @ 2020-09-02 22:01 UTC (permalink / raw)
To: mptcp
[-- Attachment #1: Type: text/plain, Size: 1600 bytes --]
Paolo Abeni <pabeni(a)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(a)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.
^ permalink raw reply [flat|nested] 4+ messages in thread* [MPTCP] Re: [PATCH mptcp-next] mptcp: use _fast lock version in __mptcp_move_skbs
@ 2020-09-04 16:40 Matthieu Baerts
0 siblings, 0 replies; 4+ messages in thread
From: Matthieu Baerts @ 2020-09-04 16:40 UTC (permalink / raw)
To: mptcp
[-- Attachment #1: Type: text/plain, Size: 1284 bytes --]
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(a)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(a)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
--
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net
^ permalink raw reply [flat|nested] 4+ messages in thread
* [MPTCP] Re: [PATCH mptcp-next] mptcp: use _fast lock version in __mptcp_move_skbs
@ 2020-09-03 9:58 Paolo Abeni
0 siblings, 0 replies; 4+ messages in thread
From: Paolo Abeni @ 2020-09-03 9:58 UTC (permalink / raw)
To: mptcp
[-- Attachment #1: Type: text/plain, Size: 1873 bytes --]
On Thu, 2020-09-03 at 00:01 +0200, Florian Westphal wrote:
> Paolo Abeni <pabeni(a)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(a)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
^ permalink raw reply [flat|nested] 4+ messages in thread* [MPTCP] Re: [PATCH mptcp-next] mptcp: use _fast lock version in __mptcp_move_skbs
@ 2020-09-02 16:16 Paolo Abeni
0 siblings, 0 replies; 4+ messages in thread
From: Paolo Abeni @ 2020-09-02 16:16 UTC (permalink / raw)
To: mptcp
[-- Attachment #1: Type: text/plain, Size: 1174 bytes --]
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(a)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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-09-04 16:40 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-09-02 22:01 [MPTCP] Re: [PATCH mptcp-next] mptcp: use _fast lock version in __mptcp_move_skbs Florian Westphal
-- strict thread matches above, loose matches on Subject: below --
2020-09-04 16:40 Matthieu Baerts
2020-09-03 9:58 Paolo Abeni
2020-09-02 16:16 Paolo Abeni
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.