On 2023-08-31 08:58:51 +0200, Eric Dumazet wrote:
On Thu, Aug 31, 2023 at 1:28 AM Mohamed Khalfella mkhalfella@purestorage.com wrote:
do { struct sk_buff *nskb; skb_frag_t *nskb_frag;@@ -4465,6 +4471,10 @@ struct sk_buff *skb_segment(struct sk_buff *head_skb, (skb_headlen(list_skb) == len || sg)) { BUG_ON(skb_headlen(list_skb) > len);
nskb = skb_clone(list_skb, GFP_ATOMIC);if (unlikely(!nskb))goto err;This patch is quite complex to review, so I am asking if this part was really needed ?
Unfortunately the patch is complex because I try to avoid calling skb_orphan_frags() in the middle of processing these frags. Otherwise it would be much harder to implement because as reallocated frags do not map 1:1 with existing frags as Willem mentioned.
<1> : You moved here <2> and <3>
<2> was moved here because skb_clone() calls skb_orphan_frags(). By moving this up we do not need to call skb_orphan_frags() for list_skb and we can start to use nr_frags and frags without worrying their value is going to change.
<3> was moved here because <2> was moved here. Fail fast if we can not clone list_skb.
If this is not strictly needed, please keep the code as is to ease code review...
i = 0; nfrags = skb_shinfo(list_skb)->nr_frags; frag = skb_shinfo(list_skb)->frags;@@ -4483,12 +4493,8 @@ struct sk_buff *skb_segment(struct sk_buff *head_skb, frag++; }
nskb = skb_clone(list_skb, GFP_ATOMIC);<2>
list_skb = list_skb->next;
if (unlikely(!nskb))goto err;<3>
if (unlikely(pskb_trim(nskb, len))) { kfree_skb(nskb); goto err;@@ -4564,12 +4570,16 @@ struct sk_buff *skb_segment(struct sk_buff *head_skb, skb_shinfo(nskb)->flags |= skb_shinfo(head_skb)->flags & SKBFL_SHARED_FRAG;
if (skb_orphan_frags(frag_skb, GFP_ATOMIC) ||skb_zerocopy_clone(nskb, frag_skb, GFP_ATOMIC))
if (skb_zerocopy_clone(nskb, list_skb, GFP_ATOMIC))Why using list_skb here instead of frag_skb ? Again, I have to look at the whole thing to understand why you did this.
Oops, this is a mistake. It should be frag_skb. Will fix it run the test one more time and post v3.