Re: [PATCH mptcp-next 2/2] mptcp: implement .read_sock

[Matthieu Baerts <matttbe@kernel.org>]

Hi Geliang,

On 22/05/2025 11:21, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> nvme_tcp_try_recv() needs to call .read_sock interface of struct
> proto_ops, but it is not implemented in MPTCP.

Thank you for looking at that!

> This patch implements it with reference to __mptcp_recvmsg_mskq().

Can you share more details about the implementation please?

I didn't check in details what this .read_sock interface is supposed to
do, but it might be good to add some explanations about that, mentioning
the differences with __mptcp_recvmsg_mskq() and why they are there, why
you cannot share code (which is certainly fine, but better with a few
words about that) etc. Plus also the differences with __tcp_read_sock()
I suppose.

Without that, I'm sorry, but it is difficult to review the code, at
least for me who doesn't know much about this interface, what it is
supposed to do nor how it is used :)

Also, would it be possible to add a new MPTCP specific selftest?

> 
> Reviewed-by: Hannes Reinecke <hare@kernel.org>
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
>  net/mptcp/protocol.c | 54 ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 54 insertions(+)
> 
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index a9094cd8bcd0..e5c69b13dbf7 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -3956,6 +3956,58 @@ static __poll_t mptcp_poll(struct file *file, struct socket *sock,
>  	return mask;
>  }
>  

If it is inspired by something else, please add a comment about that
here so if something change there, we might think about changing things
here as well.

> +static int mptcp_read_sock(struct sock *sk, read_descriptor_t *desc,
> +			   sk_read_actor_t recv_actor)
> +{
> +	struct mptcp_sock *msk = mptcp_sk(sk);
> +	struct scm_timestamping_internal tss;
> +	struct sk_buff *skb, *tmp;
> +	size_t len = INT_MAX;
> +	int copied = 0;
> +
> +	skb_queue_walk_safe(&sk->sk_receive_queue, skb, tmp) {
> +		u32 offset = MPTCP_SKB_CB(skb)->offset;
> +		u32 data_len = skb->len - offset;
> +		u32 count = min_t(size_t, len - copied, data_len);
> +		int used;
> +
> +		used = recv_actor(desc, skb, offset, count);

(to avoid confusions with __mptcp_recvmsg_mskq(), maybe clearer to use
'count' instead of 'used' here? I mean: below, 'used' is used here,
while 'count' is used in __mptcp_recvmsg_mskq(), that's a bit confusing)

> +		if (used <= 0) {
> +			if (!copied)
> +				copied = used;
> +			break;
> +		}
> +
> +		if (MPTCP_SKB_CB(skb)->has_rxtstamp)
> +			tcp_update_recv_tstamps(skb, &tss);
> +
> +		copied += used;
> +
> +		if (used < data_len) {
> +			MPTCP_SKB_CB(skb)->offset += used;
> +			MPTCP_SKB_CB(skb)->map_seq += used;
> +			msk->bytes_consumed += used;
> +			break;
> +		}
> +
> +		skb->destructor = NULL;

That looks strange, please keep the comment that was there before

> +		atomic_sub(skb->truesize, &sk->sk_rmem_alloc);
> +		sk_mem_uncharge(sk, skb->truesize);
> +		sk_eat_skb(sk, skb);
> +		msk->bytes_consumed += used;
> +
> +		if (copied >= len)
> +			break;
> +	}
> +
> +	mptcp_rcv_space_adjust(msk, copied);
> +
> +	if (copied > 0)
> +		mptcp_cleanup_rbuf(msk, copied);
> +
> +	return copied;

On TCP side, they first check the sk_state, and they also look for the
end of the end of a connection. Is it not needed here?

> +}
> +
>  static const struct proto_ops mptcp_stream_ops = {
>  	.family		   = PF_INET,
>  	.owner		   = THIS_MODULE,
> @@ -3976,6 +4028,7 @@ static const struct proto_ops mptcp_stream_ops = {
>  	.recvmsg	   = inet_recvmsg,
>  	.mmap		   = sock_no_mmap,
>  	.set_rcvlowat	   = mptcp_set_rcvlowat,
> +	.read_sock	   = mptcp_read_sock,
>  };
>  
>  static struct inet_protosw mptcp_protosw = {
> @@ -4080,6 +4133,7 @@ static const struct proto_ops mptcp_v6_stream_ops = {
>  	.compat_ioctl	   = inet6_compat_ioctl,
>  #endif
>  	.set_rcvlowat	   = mptcp_set_rcvlowat,
> +	.read_sock	   = mptcp_read_sock,
>  };
>  
>  static struct proto mptcp_v6_prot;

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.