From: Geliang Tang <tanggeliang@kylinos.cn>

It's better to get msk->snd_nxt with READ_ONCE() in get_subflow() of
the burst scheduler.

Similar to mptcp_wnd_end() helper, this patch defines a new helper
mptcp_snd_nxt(), and use it in mptcp_subflow_get_send().

It will be used in BPF burst scheduler too, so export it in protocol.h.

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 net/mptcp/protocol.c | 7 ++++++-
 net/mptcp/protocol.h | 1 +
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index ac94225489f8..6f662e7d6ff9 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -55,6 +55,11 @@ u64 mptcp_wnd_end(const struct mptcp_sock *msk)
 	return READ_ONCE(msk->wnd_end);
 }
 
+u64 mptcp_snd_nxt(const struct mptcp_sock *msk)
+{
+	return READ_ONCE(msk->snd_nxt);
+}
+
 static const struct proto_ops *mptcp_fallback_tcp_ops(const struct sock *sk)
 {
 #if IS_ENABLED(CONFIG_MPTCP_IPV6)
@@ -1465,7 +1470,7 @@ struct sock *mptcp_subflow_get_send(struct mptcp_sock *msk)
 	if (!ssk || !sk_stream_memory_free(ssk))
 		return NULL;
 
-	burst = min_t(int, MPTCP_SEND_BURST_SIZE, mptcp_wnd_end(msk) - msk->snd_nxt);
+	burst = min_t(int, MPTCP_SEND_BURST_SIZE, mptcp_wnd_end(msk) - mptcp_snd_nxt(msk));
 	wmem = READ_ONCE(ssk->sk_wmem_queued);
 	if (!burst)
 		return ssk;
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 19d60b6d5b45..26b010c88773 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -706,6 +706,7 @@ void mptcp_subflow_reset(struct sock *ssk);
 void mptcp_subflow_queue_clean(struct sock *sk, struct sock *ssk);
 void mptcp_sock_graft(struct sock *sk, struct socket *parent);
 u64 mptcp_wnd_end(const struct mptcp_sock *msk);
+u64 mptcp_snd_nxt(const struct mptcp_sock *msk);
 void mptcp_set_timeout(struct sock *sk);
 bool bpf_mptcp_subflow_queues_empty(struct sock *sk);
 struct mptcp_subflow_context *
-- 
2.43.0

Hi Geliang,

Thank you for this patch!

On 10/07/2024 15:46, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> It's better to get msk->snd_nxt with READ_ONCE() in get_subflow() of
> the burst scheduler.

Please always explain the reason → "It's better": OK, but why is it
better? :)

Can we currently have a data race with the in-kernel scheduler? Is this
field being read and written locklessly?

If yes, this should be explained in the commit message, target -net, and
have a proper 'Fixes' tag.

> Similar to mptcp_wnd_end() helper, this patch defines a new helper
> mptcp_snd_nxt(), and use it in mptcp_subflow_get_send().
> 
> It will be used in BPF burst scheduler too, so export it in protocol.h.

(if this patch is for -net, you can remove it).

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

Hi Matt,

Thanks for your review.

On Thu, 2024-07-11 at 18:13 +0200, Matthieu Baerts wrote:
> Hi Geliang,
> 
> Thank you for this patch!
> 
> On 10/07/2024 15:46, Geliang Tang wrote:
> > From: Geliang Tang <tanggeliang@kylinos.cn>
> > 
> > It's better to get msk->snd_nxt with READ_ONCE() in get_subflow()
> > of
> > the burst scheduler.
> 
> Please always explain the reason → "It's better": OK, but why is it
> better? :)
> 
> Can we currently have a data race with the in-kernel scheduler? Is
> this
> field being read and written locklessly?

No. I haven't actually encountered this data race, so this patch can
only be considered as a cleanup, not a fixes. This patch was written
when I debugged a data race issue a long time ago. It has been in my
tree for a long time. I just discovered it recently.

Alternatively, we can simply drop this set.

Thanks,
-Geliang

> 
> If yes, this should be explained in the commit message, target -net,
> and
> have a proper 'Fixes' tag.
> 
> > Similar to mptcp_wnd_end() helper, this patch defines a new helper
> > mptcp_snd_nxt(), and use it in mptcp_subflow_get_send().
> > 
> > It will be used in BPF burst scheduler too, so export it in
> > protocol.h.
> 
> (if this patch is for -net, you can remove it).
> 
> Cheers,
> Matt