From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id C15C6A0C41; Tue, 22 Jun 2021 09:44:09 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 3F1224003F; Tue, 22 Jun 2021 09:44:09 +0200 (CEST) Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) by mails.dpdk.org (Postfix) with ESMTP id 75D5D4003C for ; Tue, 22 Jun 2021 09:44:07 +0200 (CEST) IronPort-SDR: 9iRVFZAmTHmODwZivyZQuiZolH+IVfDylmisYztWSbrZ38ttFEQxE+c1cGQLigDLjQwfTPM7kS yV8jyhzdAxBw== X-IronPort-AV: E=McAfee;i="6200,9189,10022"; a="207044965" X-IronPort-AV: E=Sophos;i="5.83,291,1616482800"; d="scan'208";a="207044965" Received: from orsmga001.jf.intel.com ([10.7.209.18]) by orsmga103.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 22 Jun 2021 00:44:01 -0700 IronPort-SDR: 6wWmhwaQPzmTeBZ2bwFzrKqELOv7lacczW/s8++iG1DFJd+pIneFRP20ojyontrXC4UKDPRBYu cg7hin0vcjlQ== X-IronPort-AV: E=Sophos;i="5.83,291,1616482800"; d="scan'208";a="486811170" Received: from pmcgoldr-mobl2.ger.corp.intel.com (HELO [10.213.201.251]) ([10.213.201.251]) by orsmga001-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 22 Jun 2021 00:43:56 -0700 To: wangyunjian , "dev@dpdk.org" Cc: "liucheng (J)" , dingxiaoxiong , Honnappa Nagarahalli , "Ruifeng Wang (Arm Technology China)" References: <4ebfe0d38b335a437edc9c58368153d005f562ce.1622460655.git.wangyunjian@huawei.com> <0d996824-6015-18d6-c730-1821a34aae0c@intel.com> <50aabf5f062c4858806bf6429a1a8c24@huawei.com> From: Ferruh Yigit X-User: ferruhy Message-ID: Date: Tue, 22 Jun 2021 08:43:52 +0100 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH] kni: fix wrong mbuf alloc count in kni_allocate_mbufs X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On 6/22/2021 8:32 AM, wangyunjian wrote: >> -----Original Message----- >> From: Ferruh Yigit [mailto:ferruh.yigit@intel.com] >> Sent: Monday, June 21, 2021 7:26 PM >> To: wangyunjian ; dev@dpdk.org >> Cc: liucheng (J) ; dingxiaoxiong >> >> Subject: Re: [dpdk-dev] [PATCH] kni: fix wrong mbuf alloc count in >> kni_allocate_mbufs >> >> On 6/21/2021 4:27 AM, wangyunjian wrote: >>>> -----Original Message----- >>>> From: Ferruh Yigit [mailto:ferruh.yigit@intel.com] >>>> Sent: Friday, June 18, 2021 9:37 PM >>>> To: wangyunjian ; dev@dpdk.org >>>> Cc: liucheng (J) ; dingxiaoxiong >>>> >>>> Subject: Re: [dpdk-dev] [PATCH] kni: fix wrong mbuf alloc count in >>>> kni_allocate_mbufs >>>> >>>> On 5/31/2021 1:09 PM, wangyunjian wrote: >>>>> From: Yunjian Wang >>>>> >>>>> In kni_allocate_mbufs(), we alloc mbuf for alloc_q as this code. >>>>> allocq_free = (kni->alloc_q->read - kni->alloc_q->write - 1) \ >>>>> & (MAX_MBUF_BURST_NUM - 1); >>>>> The value of allocq_free maybe zero (e.g 32 & (32 - 1) = 0), and it >>>>> will not fill the alloc_q. When the alloc_q's free count is zero, it >>>>> will drop the packet in kernel kni. >>>>> >>>> >>>> nack >>>> >>>> Both 'read' & 'write' pointers can be max 'len-1', so 'read - write - >>>> 1' can't be 'len'. >>>> For above example first part can't be '32'. >>>> >>>> But if you are observing a problem, can you please describe it a >>>> little more, it may be because of something else. >>> >>> The ring size is 1024. After init, write = read = 0. Then we fill kni->alloc_q to >> full. At this time, write = 1023, read = 0. >>> Then the kernel send 32 packets to userspace. At this time, write = 1023, >> read = 32. >>> And then the userspace recieve this 32 packets. Then fill the kni->alloc_q, (32 >> - 1023 - 1)&31 = 0, fill nothing. >>> ... >>> Then the kernel send 32 packets to userspace. At this time, write = 1023, >> read = 992. >>> And then the userspace recieve this 32 packets. Then fill the kni->alloc_q, >> (992 - 1023 - 1)&31 = 0, fill nothing. >>> Then the kernel send 32 packets to userspace. The kni->alloc_q only has 31 >> mbufs and will drop one packet. >>> >>> Absolutely, this is a special scene. Normally, it will fill some mbufs everytime, >> but may not enough for the kernel to use. >>> In this patch, we always keep the kni->alloc_q to full for the kernel to use. >>> >> >> I see now, yes it is technically possible to have above scenario and it can cause >> glitch in the datapath. >> >> Below fix looks good, +1 to use 'kni_fifo_free_count()' instead of calculation >> within the function which may be wrong for the 'RTE_USE_C11_MEM_MODEL' >> case. > > I compiled them on the ARM and x86 platforms with the 'RTE_USE_C11_MEM_MODEL' > case, and no error is reported. > May not be build error, but in 'RTE_USE_C11_MEM_MODEL' case 'read'/'write' are not volatile and need to read them via C11 atomic instructions. 'allocq_free' calculation in the 'kni_allocate_mbufs()' doesn't do that, that is why better to replace calculation with 'kni_fifo_free_count()'. >> >> Can you please add fixes line too? > > OK, will include it in next version. > Thanks. > Thanks > >> >>> Thanks >>> >>>> >>>>> In this patch, we set the allocq_free as the min between >>>>> MAX_MBUF_BURST_NUM and the free count of the alloc_q. >>>>> >>>>> Signed-off-by: Cheng Liu >>>>> Signed-off-by: Yunjian Wang >>>>> --- >>>>> lib/kni/rte_kni.c | 5 +++-- >>>>> 1 file changed, 3 insertions(+), 2 deletions(-) >>>>> >>>>> diff --git a/lib/kni/rte_kni.c b/lib/kni/rte_kni.c index >>>>> 9dae6a8d7c..20d8f20cef 100644 >>>>> --- a/lib/kni/rte_kni.c >>>>> +++ b/lib/kni/rte_kni.c >>>>> @@ -677,8 +677,9 @@ kni_allocate_mbufs(struct rte_kni *kni) >>>>> return; >>>>> } >>>>> >>>>> - allocq_free = (kni->alloc_q->read - kni->alloc_q->write - 1) >>>>> - & (MAX_MBUF_BURST_NUM - 1); >>>>> + allocq_free = kni_fifo_free_count(kni->alloc_q); >>>>> + allocq_free = (allocq_free > MAX_MBUF_BURST_NUM) ? >>>>> + MAX_MBUF_BURST_NUM : allocq_free; >>>>> for (i = 0; i < allocq_free; i++) { >>>>> pkts[i] = rte_pktmbuf_alloc(kni->pktmbuf_pool); >>>>> if (unlikely(pkts[i] == NULL)) { >>>>> >>> >