Re: [PATCH mptcp-next v4 03/11] mptcp: move burst check out of subflow_get_send

[Mat Martineau <mathew.j.martineau@linux.intel.com>]

On Tue, 4 Oct 2022, Mat Martineau wrote:

> On Sun, 2 Oct 2022, Geliang Tang wrote:
>
>> This patch moves the burst check conditions out of the function
>> mptcp_subflow_get_send(), check them in __mptcp_push_pending() and
>> __mptcp_subflow_push_pending() in the inner "for each pending dfrag" loop.
>> 
>> Signed-off-by: Geliang Tang <geliang.tang@suse.com>
>
> This patch changes behavior in a way that would break MPTCP sends if someone 
> was bisecting git commits. I recommend squashing this with the next patch and 
> making sure the code tests ok.
>
>> ---
>> net/mptcp/protocol.c | 27 +++++++++++++++------------
>> 1 file changed, 15 insertions(+), 12 deletions(-)
>> 
>> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
>> index bbc43212a20f..785c52b738cf 100644
>> --- a/net/mptcp/protocol.c
>> +++ b/net/mptcp/protocol.c
>> @@ -1417,14 +1417,6 @@ struct sock *mptcp_subflow_get_send(struct 
>> mptcp_sock *msk)
>> 	u64 linger_time;
>> 	long tout = 0;
>> 
>> -	/* re-use last subflow, if the burst allow that */
>> -	if (msk->last_snd && msk->snd_burst > 0 &&
>> -	    sk_stream_memory_free(msk->last_snd) &&
>> -	    mptcp_subflow_active(mptcp_subflow_ctx(msk->last_snd))) {
>> -		mptcp_set_timeout(sk);
>> -		return msk->last_snd;
>
> Once msk->last_snd is removed here, that member of msk is no longer used and 
> it can be removed from struct mptcp_sock. I think I mentioned this in a 
> previous review - if I'm mistaken please correct me here. Otherwise I will 
> keep making the same suggestion :)
>
>

Also, if msk->last_snd is removed, MPTCP_RESET_SCHEDULER can be removed 
too.

- Mat
>
>
>> -	}
>> -
>> 	/* pick the subflow with the lower wmem/wspace ratio */
>> 	for (i = 0; i < SSK_MODE_MAX; ++i) {
>> 		send_info[i].ssk = NULL;
>> @@ -1477,16 +1469,13 @@ struct sock *mptcp_subflow_get_send(struct 
>> mptcp_sock *msk)
>>
>> 	burst = min_t(int, MPTCP_SEND_BURST_SIZE, mptcp_wnd_end(msk) - 
>> msk->snd_nxt);
>> 	wmem = READ_ONCE(ssk->sk_wmem_queued);
>> -	if (!burst) {
>> -		msk->last_snd = NULL;
>> +	if (!burst)
>> 		return ssk;
>> -	}
>>
>> 	subflow = mptcp_subflow_ctx(ssk);
>> 	subflow->avg_pacing_rate = div_u64((u64)subflow->avg_pacing_rate * 
>> wmem +
>> 					   READ_ONCE(ssk->sk_pacing_rate) * 
>> burst,
>> 					   burst + wmem);
>> -	msk->last_snd = ssk;
>> 	msk->snd_burst = burst;
>> 	return ssk;
>> }
>> @@ -1579,6 +1568,13 @@ void __mptcp_push_pending(struct sock *sk, unsigned 
>> int flags)
>> 			mptcp_update_post_push(msk, dfrag, ret);
>> 		}
>> 		WRITE_ONCE(msk->first_pending, mptcp_send_next(sk));
>> +
>> +		if (msk->snd_burst <= 0 ||
>> +		    !sk_stream_memory_free(ssk) ||
>> +		    !mptcp_subflow_active(mptcp_subflow_ctx(ssk))) {
>> +			goto out;
>> +		}
>> +		mptcp_set_timeout(sk);
>> 	}
>>
>> 	/* at this point we held the socket lock for the last subflow we used 
>> */
>> @@ -1637,6 +1633,13 @@ static void __mptcp_subflow_push_pending(struct sock 
>> *sk, struct sock *ssk,
>> 			mptcp_update_post_push(msk, dfrag, ret);
>> 		}
>> 		WRITE_ONCE(msk->first_pending, mptcp_send_next(sk));
>> +
>> +		if (msk->snd_burst <= 0 ||
>> +		    !sk_stream_memory_free(ssk) ||
>> +		    !mptcp_subflow_active(mptcp_subflow_ctx(ssk))) {
>> +			goto out;
>> +		}
>> +		mptcp_set_timeout(sk);
>> 	}
>> 
>> out:
>> -- 
>> 2.35.3
>> 
>> 
>> 
>
> --
> Mat Martineau
> Intel
>
>

--
Mat Martineau
Intel