On Sat, Aug 11, 2018 at 11:54:53AM -0700, David Miller wrote: > From: Doron Roberts-Kedes <doro...@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).
