[MPTCP] Re: [PATCH net-next 6/6] mptcp: use mptcp backlog.

[MPTCP] Re: [PATCH net-next 6/6] mptcp: use mptcp backlog.

[Davide Caratti]

[-- Attachment #1: Type: text/plain, Size: 1839 bytes --]

hello,

On Tue, 2020-11-10 at 12:25 +0100, Paolo Abeni wrote:
> We have some tasks triggered by the subflow receive path
> which require to access the msk socket status, specifically:
> mptcp_clean_una() and mptcp_push_pending()
> 
> We have almost everything in place to use the msk backlog
> and a standard !owned by user schema for that.
> 
> Introduce a dummy per msk sk_buff to trigger into the backlog
> as needed.
> 
> Since the worker is no more used to clean the acked data,
> for fallback sockets we need to explicitly call flush them.
> 
> As added bonus we can simplify a lot mptcp_poll() and
> move the timer update under the data lock
> 
> Signed-off-by: Paolo Abeni <pabeni(a)redhat.com>
> --
> rfc v3-> v2:
>  - dfrag_release is always invoked under sock spin lock,
>    must use mem_release
>  - move the mem_reclaim in mptcp_clean_una
>  - cleanup poll, leveraging the previous patch
> ---
>  net/mptcp/options.c  |  18 +++--
>  net/mptcp/protocol.c | 183 +++++++++++++++++++++++++++----------------
>  net/mptcp/protocol.h |   8 ++
>  3 files changed, 134 insertions(+), 75 deletions(-)
> 
> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> index 3986454a0340..48dd6622a04c 100644
> --- a/net/mptcp/options.c
> +++ b/net/mptcp/options.c
> @@ -189,7 +189,7 @@ static void mptcp_parse_option(const struct sk_buff *skb,
>  				ptr += 4;
>  			}
> 
> -			pr_debug("data_ack=%llu", mp_opt->data_ack);
> +			pr_debug("64=%d data_ack=%llx", mp_opt->ack64, mp_opt->data_ack);

why are you changing (here and in hunks below) the format specifier of
DACKs from decimal to hex format? tcpdump and wireshark print it as
decimal, and we use to compare it with TCP sequence numbers, that are in
decimal format too: I'd prefer to keep it %llu.

WDYT?
-- 
davide

[MPTCP] Re: [PATCH net-next 6/6] mptcp: use mptcp backlog.

[Paolo Abeni]

[-- Attachment #1: Type: text/plain, Size: 1990 bytes --]

On Wed, 2020-11-11 at 10:02 +0100, Davide Caratti wrote:
> hello,
> 
> On Tue, 2020-11-10 at 12:25 +0100, Paolo Abeni wrote:
> > We have some tasks triggered by the subflow receive path
> > which require to access the msk socket status, specifically:
> > mptcp_clean_una() and mptcp_push_pending()
> > 
> > We have almost everything in place to use the msk backlog
> > and a standard !owned by user schema for that.
> > 
> > Introduce a dummy per msk sk_buff to trigger into the backlog
> > as needed.
> > 
> > Since the worker is no more used to clean the acked data,
> > for fallback sockets we need to explicitly call flush them.
> > 
> > As added bonus we can simplify a lot mptcp_poll() and
> > move the timer update under the data lock
> > 
> > Signed-off-by: Paolo Abeni <pabeni(a)redhat.com>
> > --
> > rfc v3-> v2:
> >  - dfrag_release is always invoked under sock spin lock,
> >    must use mem_release
> >  - move the mem_reclaim in mptcp_clean_una
> >  - cleanup poll, leveraging the previous patch
> > ---
> >  net/mptcp/options.c  |  18 +++--
> >  net/mptcp/protocol.c | 183 +++++++++++++++++++++++++++----------------
> >  net/mptcp/protocol.h |   8 ++
> >  3 files changed, 134 insertions(+), 75 deletions(-)
> > 
> > diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> > index 3986454a0340..48dd6622a04c 100644
> > --- a/net/mptcp/options.c
> > +++ b/net/mptcp/options.c
> > @@ -189,7 +189,7 @@ static void mptcp_parse_option(const struct sk_buff *skb,
> >  				ptr += 4;
> >  			}
> > 
> > -			pr_debug("data_ack=%llu", mp_opt->data_ack);
> > +			pr_debug("64=%d data_ack=%llx", mp_opt->ack64, mp_opt->data_ack);
> 
> why are you changing (here and in hunks below) the format specifier of
> DACKs from decimal to hex format? 

whoops, this is an unneeded/unindended change that slipped in - it's
there as debug hint, fits better my personal preference/setup.

Will be dropped in later iteration.

Cheers,

Paolo