On 08/01/18 01:49 PM, Vakul Garg wrote: > > I don't think this patch is safe as-is. sgin_arr is a stack array of size > > MAX_SKB_FRAGS (+ overhead), while my read of skb_cow_data is that it > > walks the whole chain of skbs from skb->next, and can return any number of > > segments. Therefore we need to heap allocate. I think I copied the IPSEC > > code here. > > > > For perf though, we could use the stack array if skb_cow_data returns <= > > MAX_SKB_FRAGS. > > skb_cow_data() is being called only when caller passes sgout=NULL (i.e. > non-zero copy case). In case of zero-copy, we do not call skb_cow_data() > and just assume that MAX_SKB_FRAGS+2 sized scatterlist array sgin_arr[] > is sufficient. This assumption could be wrong. So skb_cow_data() should be > called unconditionally to determine number of scatterlist entries required > for skb.
I agree it is best to unify them. I was originally worried about perf with the extra allocation (which you proposed fixing by merging with the crypto allocation, which would be great), and the perf of skb_cow_data(). Zerocopy doesn't require skb_cow_data(), but we do have to walk the skbs to calculate nsg correctly. However skb_cow_data perf might be fine after your fix "strparser: Call skb_unclone conditionally".
