Re: [PATCH v2 mptcp-next] mptcp: drop tx skb cache

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

On Wed, 26 May 2021, Paolo Abeni wrote:

> On Fri, 2021-05-21 at 15:57 -0700, Mat Martineau wrote:
>> On Fri, 21 May 2021, Paolo Abeni wrote:
>>
>>> The mentioned cache was introduced to reduce the number of skb
>>> allocation in atomic context, but the required complexity is
>>> excessive.
>>>
>>> This change remove the mentioned cache.
>>>
>>> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
>>> ---
>>> v1 -> v2:
>>> - drop unused field skb_tx_cache
>>
>> There's also a size_goal_cache field in struct mptcp_sock that's now
>> unused.
>
> yep, will do in v3 (sorry, I lost track of this review)
>
>> I like the simplification! Do the self tests trigger enough memory
>> pressure to test allocation in that scenario?
>
> Will ensure that before posting v3
>
>> The commit that introduced the skb_tx_cache did so to allow removal of the
>> skb_ext cache. It looks like both caches are now unnecessary because the
>> skb and skb_ext allocations are handled together in the
>> __mptcp_{do_,}alloc_tx_skb() functions - so an allocation failure can be
>> handled without the complexity of the caches.
>
> The cache goal was avoid/reduce skb allocation in atomic context. That
> is still true (e.g. with skb_tx_cache we have less skb allocation in
> atomic context, expecially when the transfer is link/pacing/peer
> limited). Plain TCP can allocare skbs in atomic context, but that does
> not happen often. MPTCP will do that almost for each skb,
> when link/pacing/peer limited.
>
> I think we can ignore the above, as e.g. high performance NIC device
> driver always allocate skb in atomic context.
>

I think you have a better sense of the tradeoffs with the allocation 
context than I do - from what you describe it doesn't seem like the extra 
complexity to avoid atomic allocation has a significant payoff.

>>  Does that match your
>> understanding? If so, it would be helpful information for the commit
>> message.
>
> The main trick, imho is that we still use the sk->sk_tx_skb_cache to
> prevent the TCP stack from allocating extension-less skb. That trick
> was introduced with the mptcp tx_cache.
>
> Not sure if the above clarify anything ?!?

Yes - it's helpful background, thanks!

--
Mat Martineau
Intel