Re: [PATCH net-next,v4] net/tls: Calculate nsg for zerocopy path without skb_cow_data.
From: Doron Roberts-Kedes Date: Mon, 20 Aug 2018 17:27:23 -0700 > 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). Ok, I guess staying with the recursive implementation is fine. It's a real shame that frag lists are so common in this code path, especially nested ones :-/ In the long term, perhaps we can do something about that. In the short term, I guess this means your original change is OK. Please resubmit when the net-next tree opens back up, thanks.
Re: [PATCH net-next,v4] net/tls: Calculate nsg for zerocopy path without skb_cow_data.
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).
Re: [PATCH net-next,v4] net/tls: Calculate nsg for zerocopy path without skb_cow_data.
From: Doron Roberts-Kedes Date: Thu, 9 Aug 2018 15:43:44 -0700 > Taking a step back, is there an existing solution for what this function > is trying to do? I was surprised to find that there did not seem to > exist a function for determining the number of scatterlist elements > required to map an skb without COW. 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(). 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? Thanks!
Re: [PATCH net-next,v4] net/tls: Calculate nsg for zerocopy path without skb_cow_data.
On Wed, Aug 08, 2018 at 12:14:30PM -0700, David Miller wrote:
> From: Doron Roberts-Kedes
> Date: Tue, 7 Aug 2018 11:09:39 -0700
>
> > +static int __skb_nsg(struct sk_buff *skb, int offset, int len,
> > +unsigned int recursion_level)
> > +{
> > + int start = skb_headlen(skb);
> > + int i, copy = start - offset;
> > + struct sk_buff *frag_iter;
> > + int elt = 0;
> > +
> > + if (unlikely(recursion_level >= 24))
> > + return -EMSGSIZE;
>
> This recursion is kinda crazy.
>
> Even skb_cow_data() doesn't recurse like this (of course because it copies
> into linear buffers).
>
> There has to be a way to simplify this. Fragment lists are such a rarely
> used SKB geometry, and few if any devices support it for transmission
> (so the fraglist will get undone at transmit time anyways).
>
Interesting. Just wanted to clarify whether the issue is the use of
recursion or the fact that the function is handling the frag_list at
all. This is the rx path, so my understanding was that we need to handle
the frag_list. Please let me know if I'm misunderstanding your point
about the rare use of fragment lists.
If the issue is the recursion, I can rewrite the function to not use
recursion, but skb_to_sgvec uses a similar pattern and is invoked
immediately afterwards.
Taking a step back, is there an existing solution for what this function
is trying to do? I was surprised to find that there did not seem to
exist a function for determining the number of scatterlist elements
required to map an skb without COW.
Re: [PATCH net-next,v4] net/tls: Calculate nsg for zerocopy path without skb_cow_data.
From: Doron Roberts-Kedes
Date: Tue, 7 Aug 2018 11:09:39 -0700
> +static int __skb_nsg(struct sk_buff *skb, int offset, int len,
> + unsigned int recursion_level)
> +{
> + int start = skb_headlen(skb);
> + int i, copy = start - offset;
> + struct sk_buff *frag_iter;
> + int elt = 0;
> +
> + if (unlikely(recursion_level >= 24))
> + return -EMSGSIZE;
This recursion is kinda crazy.
Even skb_cow_data() doesn't recurse like this (of course because it copies
into linear buffers).
There has to be a way to simplify this. Fragment lists are such a rarely
used SKB geometry, and few if any devices support it for transmission
(so the fraglist will get undone at transmit time anyways).
> + // We need one extra for ctx->rx_aad_ciphertext
Please do not use C++ style comments in code.
Thanks.
