Hi Paolo,
Thanks for this patch. I've left some minor cleanup comments below.
On Tue, 2025-09-16 at 18:27 +0200, Paolo Abeni wrote:
> Factor out all the skb initialization step in a new helper and
> use it. Note that this change moves the MPTCP CB initialization
> earlier: we can do such step as soon as the skb leaves the
> subflow socket receive queues.
>
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> ---
> net/mptcp/protocol.c | 46 ++++++++++++++++++++++++------------------
> --
> 1 file changed, 25 insertions(+), 21 deletions(-)
>
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 671c51cb9539c..879157a1f4fb1 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -323,27 +323,11 @@ static void mptcp_data_queue_ofo(struct
> mptcp_sock *msk, struct sk_buff *skb)
> mptcp_rcvbuf_grow(sk);
> }
>
> -static bool __mptcp_move_skb(struct mptcp_sock *msk, struct sock
> *ssk,
> - struct sk_buff *skb, unsigned int
> offset,
> - size_t copy_len)
> +static void mptcp_init_skb(struct sock *ssk,
> + struct mptcp_subflow_context *subflow,
subflow and ssk parameters are redundant, we can just use ssk.
> + struct sk_buff *skb, int offset, int
> copy_len)
> {
> - struct mptcp_subflow_context *subflow =
> mptcp_subflow_ctx(ssk);
> - struct sock *sk = (struct sock *)msk;
> - struct sk_buff *tail;
> - bool has_rxtstamp;
> -
> - __skb_unlink(skb, &ssk->sk_receive_queue);
> -
> - skb_ext_reset(skb);
> - skb_orphan(skb);
> -
> - /* try to fetch required memory from subflow */
> - if (!sk_rmem_schedule(sk, skb, skb->truesize)) {
> - MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_RCVPRUNED);
> - goto drop;
> - }
> -
> - has_rxtstamp = TCP_SKB_CB(skb)->has_rxtstamp;
> + bool has_rxtstamp = TCP_SKB_CB(skb)->has_rxtstamp;
>
> /* the skb map_seq accounts for the skb offset:
> * mptcp_subflow_get_mapped_dsn() is based on the current
> tp->copied_seq
> @@ -355,6 +339,24 @@ static bool __mptcp_move_skb(struct mptcp_sock
> *msk, struct sock *ssk,
> MPTCP_SKB_CB(skb)->has_rxtstamp = has_rxtstamp;
> MPTCP_SKB_CB(skb)->cant_coalesce = 0;
>
> + __skb_unlink(skb, &ssk->sk_receive_queue);
> +
> + skb_ext_reset(skb);
skb_orphan(skb) call was moved from here to after mptcp_init_skb(), but
was then completely removed in patch 11. I'm a bit confused by this. I
think it might be better to retain skb_orphan(skb) call here. Or
perhaps the addition of skb_dst_drop(skb) has made skb_orphan(skb)
unnecessary. I'd like to consult on this point.
> + skb_dst_drop(skb);
> +}
> +
> +static bool __mptcp_move_skb(struct mptcp_sock *msk, struct sk_buff
> *skb)
The parameter 'msk' was changed to 'struct sock *sk' in patch 9. We can
directly adopt 'struct sock *sk' here as well.
Thanks,
-Geliang
> +{
> + u64 copy_len = MPTCP_SKB_CB(skb)->end_seq -
> MPTCP_SKB_CB(skb)->map_seq;
> + struct sock *sk = (struct sock *)msk;
> + struct sk_buff *tail;
> +
> + /* try to fetch required memory from subflow */
> + if (!sk_rmem_schedule(sk, skb, skb->truesize)) {
> + MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_RCVPRUNED);
> + goto drop;
> + }
> +
> if (MPTCP_SKB_CB(skb)->map_seq == msk->ack_seq) {
> /* in sequence */
> msk->bytes_received += copy_len;
> @@ -662,7 +664,9 @@ static bool __mptcp_move_skbs_from_subflow(struct
> mptcp_sock *msk,
> if (offset < skb->len) {
> size_t len = skb->len - offset;
>
> - ret = __mptcp_move_skb(msk, ssk, skb,
> offset, len) || ret;
> + mptcp_init_skb(ssk, subflow, skb, offset,
> len);
> + skb_orphan(skb);
> + ret = __mptcp_move_skb(msk, skb) || ret;
> seq += len;
>
> if (unlikely(map_remaining < len)) {