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 3BD33A0547; Mon, 21 Jun 2021 13:26:30 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id B1D3241158; Mon, 21 Jun 2021 13:26:29 +0200 (CEST) Received: from mga06.intel.com (mga06.intel.com [134.134.136.31]) by mails.dpdk.org (Postfix) with ESMTP id 6881E40040 for ; Mon, 21 Jun 2021 13:26:28 +0200 (CEST) IronPort-SDR: tywAljQMC0S99DVX3UCpRTOqWRVJ2O+X+FStabo6ABqQd7/TkOAJEIqa6yMQ8SPp7vB8E9fU5R kM+ICIeOGwMw== X-IronPort-AV: E=McAfee;i="6200,9189,10021"; a="267963661" X-IronPort-AV: E=Sophos;i="5.83,289,1616482800"; d="scan'208";a="267963661" Received: from fmsmga008.fm.intel.com ([10.253.24.58]) by orsmga104.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 21 Jun 2021 04:26:26 -0700 IronPort-SDR: 0avEuYRTYr5R/75MZNaRqjcwgmBvQYuLbCmmzcN7MgM7eY9J4k63Bu14nzNcQ85Q6tazrHVOTW IjiL4zFWjSlw== X-IronPort-AV: E=Sophos;i="5.83,289,1616482800"; d="scan'208";a="453849974" Received: from fyigit-mobl1.ger.corp.intel.com (HELO [10.213.238.17]) ([10.213.238.17]) by fmsmga008-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 21 Jun 2021 04:26:25 -0700 To: wangyunjian , "dev@dpdk.org" Cc: "liucheng (J)" , dingxiaoxiong 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: Mon, 21 Jun 2021 12:26:22 +0100 MIME-Version: 1.0 In-Reply-To: <50aabf5f062c4858806bf6429a1a8c24@huawei.com> 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/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. Can you please add fixes line too? > 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)) { >>> >