From mboxrd@z Thu Jan 1 00:00:00 1970 From: Doron Roberts-Kedes Subject: Re: [PATCH net-next,v4] net/tls: Calculate nsg for zerocopy path without skb_cow_data. Date: Mon, 20 Aug 2018 17:27:23 -0700 Message-ID: <20180821002723.GA79644@doronrk-mbp> References: <20180807180939.3872231-1-doronrk@fb.com> <20180808.121430.2168057232319674612.davem@davemloft.net> <20180809224344.GA48089@doronrk-mbp.dhcp.thefacebook.com> <20180811.115453.2016294695634012705.davem@davemloft.net> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Cc: , , , , To: David Miller Return-path: Received: from mx0b-00082601.pphosted.com ([67.231.153.30]:33992 "EHLO mx0b-00082601.pphosted.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726585AbeHUDpd (ORCPT ); Mon, 20 Aug 2018 23:45:33 -0400 Content-Disposition: inline In-Reply-To: <20180811.115453.2016294695634012705.davem@davemloft.net> Sender: netdev-owner@vger.kernel.org List-ID: On Sat, Aug 11, 2018 at 11:54:53AM -0700, David Miller wrote: > From: Doron Roberts-Kedes > 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).