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