Hi Geliang,
On 14/09/2023 16:50, Geliang Tang wrote:
> If the initial subflow has been removed, we cannot know without checking
> other counters, e.g. ss -ti <filter> | grep -c tcp-ulp-mptcp or
> getsockopt(SOL_MPTCP, MPTCP_FULL_INFO, ...) (or others except MPTCP_INFO
> of course) and then check mptcp_subflow_data->num_subflows to get the
> total amount of subflows.
>
> This patch adds a new flag NO_INITIAL_SUBFLOW in mptcpi_flags to know if
> the initial subflow has been removed.
>
> Add a new helper __mptcp_check_initial_subflow() to check whether the
> initial subflow is closing or closed.
>
> Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/428
> Signed-off-by: Geliang Tang <geliang.tang@suse.com>
> ---
> include/uapi/linux/mptcp.h | 1 +
> net/mptcp/pm_netlink.c | 3 ++-
> net/mptcp/protocol.h | 5 +++++
> net/mptcp/sockopt.c | 2 ++
> 4 files changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/include/uapi/linux/mptcp.h b/include/uapi/linux/mptcp.h
> index ee9c49f949a2..c92421866aaf 100644
> --- a/include/uapi/linux/mptcp.h
> +++ b/include/uapi/linux/mptcp.h
> @@ -107,6 +107,7 @@ enum {
>
> #define MPTCP_INFO_FLAG_FALLBACK _BITUL(0)
> #define MPTCP_INFO_FLAG_REMOTE_KEY_RECEIVED _BITUL(1)
> +#define MPTCP_INFO_FLAG_NO_INITIAL_SUBFLOW _BITUL(2)
With only one flag, the userspace cannot know if the kernel supports
this feature or not: if you are on an older kernel, this flag will not
be set if the initial subflow has been removed. It means that the
userspace cannot be sure about the total number of subflows. We might
need to set another flag is the initial subflow is there.
But then, instead of adding 2 flags, I wonder if it would not be better
to add a new counter (e.g. 'mptcpi_total_subflows'): we would avoid
confusions with 'mptcpi_subflows' which doesn't take into account the
initial subflow.
>
> struct mptcp_info {
> __u8 mptcpi_subflows;
> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> index 9661f3812682..4b196235c058 100644
> --- a/net/mptcp/pm_netlink.c
> +++ b/net/mptcp/pm_netlink.c
> @@ -1462,7 +1462,8 @@ static int mptcp_nl_remove_id_zero_address(struct net *net,
> struct sock *sk = (struct sock *)msk;
> struct mptcp_addr_info msk_local;
>
> - if (list_empty(&msk->conn_list) || mptcp_pm_is_userspace(msk))
> + if (list_empty(&msk->conn_list) || mptcp_pm_is_userspace(msk) ||
> + !__mptcp_check_initial_subflow(msk))
Again, sorry it is a quick review but why do you need to do this
modification here?
> goto next;
>
> mptcp_local_address((struct sock_common *)msk, &msk_local);
> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> index 3c938e3560e4..96e3846b50fe 100644
> --- a/net/mptcp/protocol.h
> +++ b/net/mptcp/protocol.h
> @@ -1031,6 +1031,11 @@ static inline void __mptcp_do_fallback(struct mptcp_sock *msk)
> set_bit(MPTCP_FALLBACK_DONE, &msk->flags);
> }
>
> +static inline bool __mptcp_check_initial_subflow(const struct mptcp_sock *msk)
Maybe "__mptcp_has_initial_subflow()"?
> +{
> + return msk->first && inet_sk_state_load(msk->first) != TCP_CLOSE;
No need to do a READ_ONCE() when called from mptcp_diag_fill_info()?
Did you check if it is normal to have 'msk->first' still set after
having removed it? Why do we keep it? (I don't remember)
Cheers,
Matt
> +}
> +
> static inline void mptcp_do_fallback(struct sock *ssk)
> {
> struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(ssk);
> diff --git a/net/mptcp/sockopt.c b/net/mptcp/sockopt.c
> index 8260202c0066..a2e1c3d159d6 100644
> --- a/net/mptcp/sockopt.c
> +++ b/net/mptcp/sockopt.c
> @@ -919,6 +919,8 @@ void mptcp_diag_fill_info(struct mptcp_sock *msk, struct mptcp_info *info)
> flags |= MPTCP_INFO_FLAG_FALLBACK;
> if (READ_ONCE(msk->can_ack))
> flags |= MPTCP_INFO_FLAG_REMOTE_KEY_RECEIVED;
> + if (!__mptcp_check_initial_subflow(msk))
> + flags |= MPTCP_INFO_FLAG_NO_INITIAL_SUBFLOW;
> info->mptcpi_flags = flags;
> mptcp_data_lock(sk);
> info->mptcpi_snd_una = msk->snd_una;
--
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net