From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx0b-00082601.pphosted.com ([67.231.153.30]:60090 "EHLO mx0a-00082601.pphosted.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751320AbeCTFyj (ORCPT ); Tue, 20 Mar 2018 01:54:39 -0400 Subject: Re: [PATCH net-next 1/2] net: permit skb_segment on head_frag frag_list skb To: "Yuan, Linyu (NSB - CN/Shanghai)" , "edumazet@google.com" , "ast@fb.com" , "daniel@iogearbox.net" , "diptanu@fb.com" , "netdev@vger.kernel.org" CC: "kernel-team@fb.com" References: <20180320051618.2691692-1-yhs@fb.com> <20180320051618.2691692-2-yhs@fb.com> From: Yonghong Song Message-ID: Date: Mon, 19 Mar 2018 22:54:03 -0700 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8"; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: netdev-owner@vger.kernel.org List-ID: On 3/19/18 10:30 PM, Yuan, Linyu (NSB - CN/Shanghai) wrote: > > >> -----Original Message----- >> From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org] >> On Behalf Of Yonghong Song >> Sent: Tuesday, March 20, 2018 1:16 PM >> To: edumazet@google.com; ast@fb.com; daniel@iogearbox.net; >> diptanu@fb.com; netdev@vger.kernel.org >> Cc: kernel-team@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 >