Re: [PATCH net-next,v4] net/tls: Calculate nsg for zerocopy path without skb_cow_data.

[Doron Roberts-Kedes <doronrk@fb.com>]

On Sat, Aug 11, 2018 at 11:54:53AM -0700, David Miller wrote:
> From: Doron Roberts-Kedes <doronrk@fb.com>
> Date: Thu, 9 Aug 2018 15:43:44 -0700
> 
> The reason is that we usually never need to "map" an SKB on receive,
> and on transmit the SKB geometry is normalized by the destination
> device features which means no frag_list.
> 
> And the other existing receive path doing SW crypto, IPSEC, always
> COWs the data so get the number of SG array entries needed from
> skb_cow_data().

Makes sense, thanks! 

> Frag lists on RX should be pretty rare.  They occur when GRO
> segmentation encouters poorly arranged incoming SKBs.  For example
> this happens if the incoming frames use lots of tiny SKB page frags,
> and therefore prevent accumulation into the page frag array of the
> head GRO skb.
> 
> So yes it can happen, and we have to account for it.
> 
> So I guess you really do need to accomodate this situation.  Why
> don't you try to rearrange your new function with some likely()
> and unlikely() tests so that the straight code path optimizes for
> the non-frag-list case?

So I did some poking around and found that basically 100% of skb's
recieved by ktls have a frag_list because of how strparser is
implemented. skb's from TCP that do not a have a frag_list are
accumulated by strparser using frag_list of a new skb. skb's from TCP
that do have a frag_list can become part of skb's with nested frag_lists
(skb's with non-NULL frag_list that are themselves part of a frag_list).
Unfortunatley, frag_list seems to be the common case, so is probably not a
good candidate for an unlikely() test. 

Regarding nested frag_list's more generally, is a good rule of thumb
that multiple layers of frag_list will not occur except for
post-processing such as in strparser? When should skb-handling code be
prepared for nested frag_lists? 

Given that frag_lists are not unlikely in this case, I believe the only
remaining feedback on the original patch was the recursive
implementation. If you'd like, I can re-submit with an iterative
implementation, but I noticed that goes against the existing recursive
pattern in functions like skb_release_data -> kfree_skb_list -> kfree_skb 
-> __kfree_skb -> skb_release_all -> skb_release_data, as well as
skb_to_sgvec. Let me know whether an iterative implementation is
preferred here, or whether I can simply rebase and resubmit a patch
similar to the original (modulo some variable renaming improvements).