Sorry, I should not add "Here cause next BUG_ON always false." It cause misunderstanding, I just comment on BUG_ON in else branch.
> -----Original Message----- > From: Yonghong Song [mailto:y...@fb.com] > Sent: Tuesday, March 20, 2018 1:54 PM > To: Yuan, Linyu (NSB - CN/Shanghai); eduma...@google.com; a...@fb.com; > dan...@iogearbox.net; dipt...@fb.com; netdev@vger.kernel.org > Cc: kernel-t...@fb.com > Subject: Re: [PATCH net-next 1/2] net: permit skb_segment on head_frag > frag_list skb > > > > On 3/19/18 10:30 PM, Yuan, Linyu (NSB - CN/Shanghai) wrote: > > > > > >> -----Original Message----- > >> From: netdev-ow...@vger.kernel.org > [mailto:netdev-ow...@vger.kernel.org] > >> On Behalf Of Yonghong Song > >> Sent: Tuesday, March 20, 2018 1:16 PM > >> To: eduma...@google.com; a...@fb.com; dan...@iogearbox.net; > >> dipt...@fb.com; netdev@vger.kernel.org > >> Cc: kernel-t...@fb.com > >> Subject: [PATCH net-next 1/2] net: permit skb_segment on head_frag > frag_list > >> skb > >> > >> > >> while (pos < offset + len) { > >> if (i >= nfrags) { > >> - BUG_ON(skb_headlen(list_skb)); > >> + if (skb_headlen(list_skb) && check_list_skb == > >> list_skb) { > > Here cause next BUG_ON always false. > > The idea is since in this branch, we did not do list_skb = > list_skb->next. So we update check_list_skb. Next time, when the > control reaches here, list_skb may still be the same, but check_list_skb > is not, so we proceed to process list_skb->frags in the else branch. > > In the else branch, we have > list_skb = list_skb->next; > check_list_skb = list_skb; > > So when the current frags are processed and ready for the list_skb. > list_skb will be equal to check_list_skb and it will be processed again. > > It is a little bit convoluted. Please let me know you have better idea. > > >> + } else { > >> + BUG_ON(skb_headlen(list_skb) && > >> check_list_skb == > >> list_skb); > > Just according code logic, no need BUG_ON, right? > > Oh, yes, we do not need this. Will remove in the next version. > > >> > >> - i = 0; > >> - nfrags = skb_shinfo(list_skb)->nr_frags; > >> - frag = skb_shinfo(list_skb)->frags; > >> - frag_skb = list_skb; > >> + i = 0; > >> + nfrags = skb_shinfo(list_skb)->nr_frags; > >> + frag = skb_shinfo(list_skb)->frags; > >> + frag_skb = list_skb; > >> > >> - BUG_ON(!nfrags); > >> + BUG_ON(!nfrags); > >> > >> - if (skb_orphan_frags(frag_skb, GFP_ATOMIC) || > >> - skb_zerocopy_clone(nskb, frag_skb, > >> - GFP_ATOMIC)) > >> - goto err; > >> + if (skb_orphan_frags(frag_skb, > >> GFP_ATOMIC) || > >> + skb_zerocopy_clone(nskb, frag_skb, > >> GFP_ATOMIC)) > >> + goto err; > >> > >> - list_skb = list_skb->next; > >> + list_skb = list_skb->next; > >> + check_list_skb = list_skb; > >> + } > >> } > >> > >> if (unlikely(skb_shinfo(nskb)->nr_frags >= > >> -- > >> 2.9.5 > >
