Such field is there to avoid acquiring the data lock in a few spots,
but if adds complexity to the already non trivial locking schema.

All the relevant call sites (mptcp-level reinjection, set socket optins),
are slow-path, drop such field in favor of 'cb_flags', adding the relevant
locking.

Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 net/mptcp/protocol.c | 12 ++++++------
 net/mptcp/protocol.h |  1 -
 2 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index e570c7e2b18c..5129e1235cc5 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -1505,8 +1505,11 @@ static void mptcp_update_post_push(struct mptcp_sock *msk,
 
 void mptcp_check_and_set_pending(struct sock *sk)
 {
-	if (mptcp_send_head(sk))
-		mptcp_sk(sk)->push_pending |= BIT(MPTCP_PUSH_PENDING);
+	if (mptcp_send_head(sk)) {
+		mptcp_data_lock(sk);
+		mptcp_sk(sk)->cb_flags |= BIT(MPTCP_PUSH_PENDING);
+		mptcp_data_unlock(sk);
+	}
 }
 
 static int __subflow_push_pending(struct sock *sk, struct sock *ssk,
@@ -3145,7 +3148,6 @@ static int mptcp_disconnect(struct sock *sk, int flags)
 	mptcp_destroy_common(msk, MPTCP_CF_FASTCLOSE);
 	WRITE_ONCE(msk->flags, 0);
 	msk->cb_flags = 0;
-	msk->push_pending = 0;
 	msk->recovery = false;
 	msk->can_ack = false;
 	msk->fully_established = false;
@@ -3333,8 +3335,7 @@ static void mptcp_release_cb(struct sock *sk)
 	struct mptcp_sock *msk = mptcp_sk(sk);
 
 	for (;;) {
-		unsigned long flags = (msk->cb_flags & MPTCP_FLAGS_PROCESS_CTX_NEED) |
-				      msk->push_pending;
+		unsigned long flags = (msk->cb_flags & MPTCP_FLAGS_PROCESS_CTX_NEED);
 		struct list_head join_list;
 
 		if (!flags)
@@ -3350,7 +3351,6 @@ static void mptcp_release_cb(struct sock *sk)
 		 *    datapath acquires the msk socket spinlock while helding
 		 *    the subflow socket lock
 		 */
-		msk->push_pending = 0;
 		msk->cb_flags &= ~flags;
 		spin_unlock_bh(&sk->sk_lock.slock);
 
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index f7b9c1b995df..ebb32b7563be 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -286,7 +286,6 @@ struct mptcp_sock {
 	int		rmem_released;
 	unsigned long	flags;
 	unsigned long	cb_flags;
-	unsigned long	push_pending;
 	bool		recovery;		/* closing subflow write queue reinjected */
 	bool		can_ack;
 	bool		fully_established;
-- 
2.43.0

Hi Paolo,

On 15/01/2024 16:16, Paolo Abeni wrote:
> Such field is there to avoid acquiring the data lock in a few spots,
> but if adds complexity to the already non trivial locking schema.
> 
> All the relevant call sites (mptcp-level reinjection, set socket optins),
> are slow-path, drop such field in favor of 'cb_flags', adding the relevant
> locking.

In your cover-letter, you mentioned:

> Patch 1 has no fixes, but still is logically tied to the other patches.

I think it makes sense to have this patch in -net, especially because it
is logically tied to the others. Would it be OK for you if I add:

  Fixes: e9d09baca676 ("mptcp: avoid atomic bit manipulation when possible")

Or do you prefer without?

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

On Thu, 2024-02-08 at 12:36 +0100, Matthieu Baerts wrote:
> Hi Paolo,
> 
> On 15/01/2024 16:16, Paolo Abeni wrote:
> > Such field is there to avoid acquiring the data lock in a few spots,
> > but if adds complexity to the already non trivial locking schema.
> > 
> > All the relevant call sites (mptcp-level reinjection, set socket optins),
> > are slow-path, drop such field in favor of 'cb_flags', adding the relevant
> > locking.
> 
> In your cover-letter, you mentioned:
> 
> > Patch 1 has no fixes, but still is logically tied to the other patches.
> 
> I think it makes sense to have this patch in -net, especially because it
> is logically tied to the others. Would it be OK for you if I add:
> 
>   Fixes: e9d09baca676 ("mptcp: avoid atomic bit manipulation when possible")

The obove, or add a sentence to the commit message alike:

"""
This will also simplify the next patch
"""
> 
> Or do you prefer without?
> 
> Cheers,
> Matt

Hi Paolo,

On 08/02/2024 16:12, Paolo Abeni wrote:
> On Thu, 2024-02-08 at 12:36 +0100, Matthieu Baerts wrote:
>> Hi Paolo,
>>
>> On 15/01/2024 16:16, Paolo Abeni wrote:
>>> Such field is there to avoid acquiring the data lock in a few spots,
>>> but if adds complexity to the already non trivial locking schema.
>>>
>>> All the relevant call sites (mptcp-level reinjection, set socket optins),
>>> are slow-path, drop such field in favor of 'cb_flags', adding the relevant
>>> locking.
>>
>> In your cover-letter, you mentioned:
>>
>>> Patch 1 has no fixes, but still is logically tied to the other patches.
>>
>> I think it makes sense to have this patch in -net, especially because it
>> is logically tied to the others. Would it be OK for you if I add:
>>
>>   Fixes: e9d09baca676 ("mptcp: avoid atomic bit manipulation when possible")
> 
> The obove, or add a sentence to the commit message alike:
> 
> """
> This will also simplify the next patch
> """

Good idea! I added both:

> This patch could be seen as an improvement, instead of a fix. But it
> simplifies the next patch. The 'Fixes' tag has been added to help having
> this series backported to stable.

I hope that's OK! :)

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

On Thu, 2024-02-08 at 19:05 +0100, Matthieu Baerts wrote:
> On 08/02/2024 16:12, Paolo Abeni wrote:
> 
> > This patch could be seen as an improvement, instead of a fix. But it
> > simplifies the next patch. The 'Fixes' tag has been added to help having
> > this series backported to stable.
> 
> I hope that's OK! :)

fine by me, thanks!

/P