diff mbox series

[net,v4,2/2] mptcp: count CLOSE-WAIT sockets for MPTCP_MIB_CURRESTAB

Message ID 20240531091753.75930-3-kerneljasonxing@gmail.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series tcp/mptcp: count CLOSE-WAIT for CurrEstab | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag present in non-next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 905 this patch: 905
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers success CCed 8 of 8 maintainers
netdev/build_clang success Errors and warnings before: 906 this patch: 906
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success Fixes tag looks correct
netdev/build_allmodconfig_warn success Errors and warnings before: 909 this patch: 909
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 12 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
netdev/contest success net-next-2024-06-03--15-00 (tests: 1041)

Commit Message

Jason Xing May 31, 2024, 9:17 a.m. UTC 
From: Jason Xing <kernelxing@tencent.com>

Like previous patch does in TCP, we need to adhere to RFC 1213:

  "tcpCurrEstab OBJECT-TYPE
   ...
   The number of TCP connections for which the current state
   is either ESTABLISHED or CLOSE- WAIT."

So let's consider CLOSE-WAIT sockets.

The logic of counting
When we increment the counter?
a) Only if we change the state to ESTABLISHED.

When we decrement the counter?
a) if the socket leaves ESTABLISHED and will never go into CLOSE-WAIT,
say, on the client side, changing from ESTABLISHED to FIN-WAIT-1.
b) if the socket leaves CLOSE-WAIT, say, on the server side, changing
from CLOSE-WAIT to LAST-ACK.

Fixes: d9cd27b8cd19 ("mptcp: add CurrEstab MIB counter support")
Signed-off-by: Jason Xing <kernelxing@tencent.com>
---
 net/mptcp/protocol.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Comments

Matthieu Baerts (NGI0) June 3, 2024, 12:47 p.m. UTC | #1 
Hi Jason,

On 31/05/2024 11:17, Jason Xing wrote:
> From: Jason Xing <kernelxing@tencent.com>
> 
> Like previous patch does in TCP, we need to adhere to RFC 1213:
> 
>   "tcpCurrEstab OBJECT-TYPE
>    ...
>    The number of TCP connections for which the current state
>    is either ESTABLISHED or CLOSE- WAIT."
> 
> So let's consider CLOSE-WAIT sockets.
> 
> The logic of counting
> When we increment the counter?
> a) Only if we change the state to ESTABLISHED.
> 
> When we decrement the counter?
> a) if the socket leaves ESTABLISHED and will never go into CLOSE-WAIT,
> say, on the client side, changing from ESTABLISHED to FIN-WAIT-1.
> b) if the socket leaves CLOSE-WAIT, say, on the server side, changing
> from CLOSE-WAIT to LAST-ACK.

Thank you for this modification, and for having updated the Fixes tag.

> Fixes: d9cd27b8cd19 ("mptcp: add CurrEstab MIB counter support")
> Signed-off-by: Jason Xing <kernelxing@tencent.com>
> ---
>  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 7d44196ec5b6..6d59c1c4baba 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -2916,9 +2916,10 @@ void mptcp_set_state(struct sock *sk, int state)
>  		if (oldstate != TCP_ESTABLISHED)
>  			MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_CURRESTAB);
>  		break;
> -
> +	case TCP_CLOSE_WAIT:
> +		break;

The modification is correct: currently, and compared to TCP, the MPTCP
"accepted" socket will not go through the TCP_SYN_RECV state because it
will be created later on.

Still, I wonder if it would not be clearer to explicitly mention this
here, and (or) in the commit message, to be able to understand why the
logic is different here, compared to TCP. I don't think the SYN_RECV
state will be used in the future with MPTCP sockets, but just in case,
it might help to mention TCP_SYN_RECV state here. Could add a small
comment here above please?

>  	default:
> -		if (oldstate == TCP_ESTABLISHED)
> +		if (oldstate == TCP_ESTABLISHED || oldstate == TCP_CLOSE_WAIT)
>  			MPTCP_DEC_STATS(sock_net(sk), MPTCP_MIB_CURRESTAB);
>  	}
>  

Cheers,
Matt
Jason Xing June 3, 2024, 1:26 p.m. UTC | #2 
Hello Matthieu,

On Mon, Jun 3, 2024 at 8:47 PM Matthieu Baerts <matttbe@kernel.org> wrote:
>
> Hi Jason,
>
> On 31/05/2024 11:17, Jason Xing wrote:
> > From: Jason Xing <kernelxing@tencent.com>
> >
> > Like previous patch does in TCP, we need to adhere to RFC 1213:
> >
> >   "tcpCurrEstab OBJECT-TYPE
> >    ...
> >    The number of TCP connections for which the current state
> >    is either ESTABLISHED or CLOSE- WAIT."
> >
> > So let's consider CLOSE-WAIT sockets.
> >
> > The logic of counting
> > When we increment the counter?
> > a) Only if we change the state to ESTABLISHED.
> >
> > When we decrement the counter?
> > a) if the socket leaves ESTABLISHED and will never go into CLOSE-WAIT,
> > say, on the client side, changing from ESTABLISHED to FIN-WAIT-1.
> > b) if the socket leaves CLOSE-WAIT, say, on the server side, changing
> > from CLOSE-WAIT to LAST-ACK.
>
> Thank you for this modification, and for having updated the Fixes tag.
>
> > Fixes: d9cd27b8cd19 ("mptcp: add CurrEstab MIB counter support")
> > Signed-off-by: Jason Xing <kernelxing@tencent.com>
> > ---
> >  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 7d44196ec5b6..6d59c1c4baba 100644
> > --- a/net/mptcp/protocol.c
> > +++ b/net/mptcp/protocol.c
> > @@ -2916,9 +2916,10 @@ void mptcp_set_state(struct sock *sk, int state)
> >               if (oldstate != TCP_ESTABLISHED)
> >                       MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_CURRESTAB);
> >               break;
> > -
> > +     case TCP_CLOSE_WAIT:
> > +             break;
>
> The modification is correct: currently, and compared to TCP, the MPTCP
> "accepted" socket will not go through the TCP_SYN_RECV state because it
> will be created later on.
>
> Still, I wonder if it would not be clearer to explicitly mention this
> here, and (or) in the commit message, to be able to understand why the
> logic is different here, compared to TCP. I don't think the SYN_RECV
> state will be used in the future with MPTCP sockets, but just in case,
> it might help to mention TCP_SYN_RECV state here. Could add a small
> comment here above please?

Sure, but what comments do you suggest?
For example, the comment above the case statement is:
"Unlike TCP, MPTCP would not have TCP_SYN_RECV state, so we can skip
it directly"
?

Thanks,
Jason

>
> >       default:
> > -             if (oldstate == TCP_ESTABLISHED)
> > +             if (oldstate == TCP_ESTABLISHED || oldstate == TCP_CLOSE_WAIT)
> >                       MPTCP_DEC_STATS(sock_net(sk), MPTCP_MIB_CURRESTAB);
> >       }
> >
>
> Cheers,
> Matt
> --
> Sponsored by the NGI0 Core fund.
>
Matthieu Baerts (NGI0) June 3, 2024, 1:57 p.m. UTC | #3 
Hi Jason,

On 03/06/2024 15:26, Jason Xing wrote:
> Hello Matthieu,
> 
> On Mon, Jun 3, 2024 at 8:47 PM Matthieu Baerts <matttbe@kernel.org> wrote:
>>
>> Hi Jason,
>>
>> On 31/05/2024 11:17, Jason Xing wrote:
>>> From: Jason Xing <kernelxing@tencent.com>
>>>
>>> Like previous patch does in TCP, we need to adhere to RFC 1213:
>>>
>>>   "tcpCurrEstab OBJECT-TYPE
>>>    ...
>>>    The number of TCP connections for which the current state
>>>    is either ESTABLISHED or CLOSE- WAIT."
>>>
>>> So let's consider CLOSE-WAIT sockets.
>>>
>>> The logic of counting
>>> When we increment the counter?
>>> a) Only if we change the state to ESTABLISHED.
>>>
>>> When we decrement the counter?
>>> a) if the socket leaves ESTABLISHED and will never go into CLOSE-WAIT,
>>> say, on the client side, changing from ESTABLISHED to FIN-WAIT-1.
>>> b) if the socket leaves CLOSE-WAIT, say, on the server side, changing
>>> from CLOSE-WAIT to LAST-ACK.
>>
>> Thank you for this modification, and for having updated the Fixes tag.
>>
>>> Fixes: d9cd27b8cd19 ("mptcp: add CurrEstab MIB counter support")
>>> Signed-off-by: Jason Xing <kernelxing@tencent.com>
>>> ---
>>>  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 7d44196ec5b6..6d59c1c4baba 100644
>>> --- a/net/mptcp/protocol.c
>>> +++ b/net/mptcp/protocol.c
>>> @@ -2916,9 +2916,10 @@ void mptcp_set_state(struct sock *sk, int state)
>>>               if (oldstate != TCP_ESTABLISHED)
>>>                       MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_CURRESTAB);
>>>               break;
>>> -
>>> +     case TCP_CLOSE_WAIT:
>>> +             break;
>>
>> The modification is correct: currently, and compared to TCP, the MPTCP
>> "accepted" socket will not go through the TCP_SYN_RECV state because it
>> will be created later on.
>>
>> Still, I wonder if it would not be clearer to explicitly mention this
>> here, and (or) in the commit message, to be able to understand why the
>> logic is different here, compared to TCP. I don't think the SYN_RECV
>> state will be used in the future with MPTCP sockets, but just in case,
>> it might help to mention TCP_SYN_RECV state here. Could add a small
>> comment here above please?
> 
> Sure, but what comments do you suggest?
> For example, the comment above the case statement is:
> "Unlike TCP, MPTCP would not have TCP_SYN_RECV state, so we can skip
> it directly"
> ?
Yes, thank you, it looks good to me. But while at it, you can also add
the reason:

  case TCP_CLOSE_WAIT:
          /* Unlike TCP, MPTCP sk would not have the TCP_SYN_RECV state:
           * MPTCP "accepted" sockets will be created later on. So no
           * transition from TCP_SYN_RECV to TCP_CLOSE_WAIT.
           */

WDYT?

Cheers,
Matt
Jason Xing June 3, 2024, 3:04 p.m. UTC | #4 
Hi Matthieu,

On Mon, Jun 3, 2024 at 9:57 PM Matthieu Baerts <matttbe@kernel.org> wrote:
>
> Hi Jason,
>
> On 03/06/2024 15:26, Jason Xing wrote:
> > Hello Matthieu,
> >
> > On Mon, Jun 3, 2024 at 8:47 PM Matthieu Baerts <matttbe@kernel.org> wrote:
> >>
> >> Hi Jason,
> >>
> >> On 31/05/2024 11:17, Jason Xing wrote:
> >>> From: Jason Xing <kernelxing@tencent.com>
> >>>
> >>> Like previous patch does in TCP, we need to adhere to RFC 1213:
> >>>
> >>>   "tcpCurrEstab OBJECT-TYPE
> >>>    ...
> >>>    The number of TCP connections for which the current state
> >>>    is either ESTABLISHED or CLOSE- WAIT."
> >>>
> >>> So let's consider CLOSE-WAIT sockets.
> >>>
> >>> The logic of counting
> >>> When we increment the counter?
> >>> a) Only if we change the state to ESTABLISHED.
> >>>
> >>> When we decrement the counter?
> >>> a) if the socket leaves ESTABLISHED and will never go into CLOSE-WAIT,
> >>> say, on the client side, changing from ESTABLISHED to FIN-WAIT-1.
> >>> b) if the socket leaves CLOSE-WAIT, say, on the server side, changing
> >>> from CLOSE-WAIT to LAST-ACK.
> >>
> >> Thank you for this modification, and for having updated the Fixes tag.
> >>
> >>> Fixes: d9cd27b8cd19 ("mptcp: add CurrEstab MIB counter support")
> >>> Signed-off-by: Jason Xing <kernelxing@tencent.com>
> >>> ---
> >>>  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 7d44196ec5b6..6d59c1c4baba 100644
> >>> --- a/net/mptcp/protocol.c
> >>> +++ b/net/mptcp/protocol.c
> >>> @@ -2916,9 +2916,10 @@ void mptcp_set_state(struct sock *sk, int state)
> >>>               if (oldstate != TCP_ESTABLISHED)
> >>>                       MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_CURRESTAB);
> >>>               break;
> >>> -
> >>> +     case TCP_CLOSE_WAIT:
> >>> +             break;
> >>
> >> The modification is correct: currently, and compared to TCP, the MPTCP
> >> "accepted" socket will not go through the TCP_SYN_RECV state because it
> >> will be created later on.
> >>
> >> Still, I wonder if it would not be clearer to explicitly mention this
> >> here, and (or) in the commit message, to be able to understand why the
> >> logic is different here, compared to TCP. I don't think the SYN_RECV
> >> state will be used in the future with MPTCP sockets, but just in case,
> >> it might help to mention TCP_SYN_RECV state here. Could add a small
> >> comment here above please?
> >
> > Sure, but what comments do you suggest?
> > For example, the comment above the case statement is:
> > "Unlike TCP, MPTCP would not have TCP_SYN_RECV state, so we can skip
> > it directly"
> > ?
> Yes, thank you, it looks good to me. But while at it, you can also add
> the reason:
>
>   case TCP_CLOSE_WAIT:
>           /* Unlike TCP, MPTCP sk would not have the TCP_SYN_RECV state:
>            * MPTCP "accepted" sockets will be created later on. So no
>            * transition from TCP_SYN_RECV to TCP_CLOSE_WAIT.
>            */
>
> WDYT?

So great. Thank you. I will update it soon.

Thanks,
Jason

>
> Cheers,
> Matt
> --
> Sponsored by the NGI0 Core fund.
>
diff mbox series

Patch

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 7d44196ec5b6..6d59c1c4baba 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -2916,9 +2916,10 @@  void mptcp_set_state(struct sock *sk, int state)
 		if (oldstate != TCP_ESTABLISHED)
 			MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_CURRESTAB);
 		break;
-
+	case TCP_CLOSE_WAIT:
+		break;
 	default:
-		if (oldstate == TCP_ESTABLISHED)
+		if (oldstate == TCP_ESTABLISHED || oldstate == TCP_CLOSE_WAIT)
 			MPTCP_DEC_STATS(sock_net(sk), MPTCP_MIB_CURRESTAB);
 	}