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 F2E04A0C41; Wed, 23 Jun 2021 16:41:32 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 8D0664003F; Wed, 23 Jun 2021 16:41:32 +0200 (CEST) Received: from wout2-smtp.messagingengine.com (wout2-smtp.messagingengine.com [64.147.123.25]) by mails.dpdk.org (Postfix) with ESMTP id 3E33D4003E; Wed, 23 Jun 2021 16:41:31 +0200 (CEST) Received: from compute5.internal (compute5.nyi.internal [10.202.2.45]) by mailout.west.internal (Postfix) with ESMTP id B0F8710CF; Wed, 23 Jun 2021 10:41:29 -0400 (EDT) Received: from mailfrontend1 ([10.202.2.162]) by compute5.internal (MEProxy); Wed, 23 Jun 2021 10:41:30 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=monjalon.net; h= from:to:cc:subject:date:message-id:in-reply-to:references :mime-version:content-transfer-encoding:content-type; s=fm1; bh= NYOBA33re0XnxUh09r8roEhUcNAZq9rPGAIxalB+ZL8=; b=6FyL/ONDy3tMD+6t qKKf96rYZDmaB8Zx/8whfMgH3jXPc8W+fxiUXNk7v6ymqwqU8GDQahZYmRGnBqDX c9zQlFnQrOlIj68qc+fr9DjAbDju3NLLDiBUOSGJ010CWYOdSRGr5bKOlHfa/LSm oI7cnvY8VaBEhauN+ZeIgWQNLfoTvMiT4n8E9Aa4aioyMom7J+SYKpxbj4/v50LH QFnz3PpeL9Zp2rqihPNQDUsGZGmg34S2a4B+AVCcS0Jm8HZyKHGwTH2BsLLnwsDN /zr8pb4jS6/hIyi2AmXGPHBa6RjugBs6gAcb7RWx5K8r6xBP/N2Y4Iq6CILsD2c0 PgcrBA== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:content-transfer-encoding:content-type :date:from:in-reply-to:message-id:mime-version:references :subject:to:x-me-proxy:x-me-proxy:x-me-sender:x-me-sender :x-sasl-enc; s=fm3; bh=NYOBA33re0XnxUh09r8roEhUcNAZq9rPGAIxalB+Z L8=; b=H95F2Zjqh/2xZdMxJcYngJtVPrPWKXAcgYeFQw7TVg8gTurVuP/7N6x7P bEfvPhXbWOy5YpSpbp6x/7b3DzuKv/PG/KcZIwLgMO//agQa1MitA7lD8qfiF3Nv jh2Pd1biwYRPBSqZQSPkBM6mIbxJ+ouTLV8oxtLcALjIuBWLOPPa0GsYkmPMKAoR RBu8iWjFUy8X59QRBlWzjItzwD4xt/fzBxjpz4/AglWzE6xfnj2BI/GHty8FmvBY O4yy0SReXOt8psC2t/nDDoV7yt33lP94ZxBFjJTLBcyrP2ndp3CJCF15DUyifoQC 8TbpVi1WP4HqZnl02Qbcw4OqWzQkQ== X-ME-Sender: X-ME-Received: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgeduledrfeegfedgjeelucetufdoteggodetrfdotf fvucfrrhhofhhilhgvmecuhfgrshhtofgrihhlpdfqfgfvpdfurfetoffkrfgpnffqhgen uceurghilhhouhhtmecufedttdenucesvcftvggtihhpihgvnhhtshculddquddttddmne cujfgurhephffvufffkfgjfhgggfgtsehtufertddttddvnecuhfhrohhmpefvhhhomhgr shcuofhonhhjrghlohhnuceothhhohhmrghssehmohhnjhgrlhhonhdrnhgvtheqnecugg ftrfgrthhtvghrnhepudeggfdvfeduffdtfeeglefghfeukefgfffhueejtdetuedtjeeu ieeivdffgeehnecuvehluhhsthgvrhfuihiivgeptdenucfrrghrrghmpehmrghilhhfrh homhepthhhohhmrghssehmohhnjhgrlhhonhdrnhgvth X-ME-Proxy: Received: by mail.messagingengine.com (Postfix) with ESMTPA; Wed, 23 Jun 2021 10:41:27 -0400 (EDT) From: Thomas Monjalon To: wangyunjian , Ferruh Yigit Cc: "dev@dpdk.org" , "stable@dpdk.org" , "gowrishankar.m@linux.vnet.ibm.com" , dingxiaoxiong , "liucheng (J)" Date: Wed, 23 Jun 2021 16:41:25 +0200 Message-ID: <5642725.EOCvtSPk5b@thomas> In-Reply-To: <8b7084b2-12fc-acd6-9cf8-1bc238b77d7d@intel.com> References: <4aebf99afe5bae2b25f2e5445a32243ffd6f7e97.1624359204.git.wangyunjian@huawei.com> <8b7084b2-12fc-acd6-9cf8-1bc238b77d7d@intel.com> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" Subject: Re: [dpdk-dev] [dpdk-stable] [PATCH v3] kni: fix mbuf allocation for alloc FIFO 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" 23/06/2021 16:11, Ferruh Yigit: > On 6/23/2021 1:16 PM, wangyunjian wrote: > > > > > >> -----Original Message----- > >> From: Thomas Monjalon [mailto:thomas@monjalon.net] > >> Sent: Wednesday, June 23, 2021 4:46 AM > >> To: wangyunjian ; liucheng (J) > >> > >> Cc: dev@dpdk.org; stable@dpdk.org; ferruh.yigit@intel.com; > >> gowrishankar.m@linux.vnet.ibm.com; dingxiaoxiong > >> ; wangyunjian > >> Subject: Re: [dpdk-stable] [dpdk-dev] [PATCH v3] kni: fix mbuf allocation for alloc > >> FIFO > >> > >> 22/06/2021 14:44, wangyunjian: > >>> 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, for example : > >>> 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 receive 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 receive 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. > >>> > >>> Fixes: 49da4e82cf94 ("kni: allocate no more mbuf than empty slots in > >>> queue") > >>> Cc: stable@dpdk.org > >>> > >>> Signed-off-by: Cheng Liu > >>> Signed-off-by: Yunjian Wang > >>> Acked-by: Ferruh Yigit > >>> --- > >>> v3: > >>> update patch title > >>> v2: > >>> add fixes tag and update commit log > >>> --- > >>> 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..eb24b0d0ae 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); > >> > >> Can we insert a comment here to explain the logic? > > > > OK, how about like this? > > > > /* Because 'read/write' maybe not volatile, so use kni_fifo_free_count() > > * to get the num of available elements in the fifo > > */ > > > > A comment like above may make sense in the commit log to explain the reason of > the change, but for developer reading the new code it doesn't give any useful > information, it even may be confusing. > > @Thomas, > Code gets the numbers of the free slots in the FIFO and fills it up to MAX_NUM > unless it gets full first. Can you please clarify which logic to comment more? Maybe no comment is needed indeed. > >> > >>> + 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)) { > >> > >> About the title, I don't understand the part "for alloc FIFO", given all mbufs are > >> in a FIFO queue in KNI, right? > > > > The title is "kni: fix mbuf allocation for FIFO queue"? > > > > There are multiple FIFOs in the KNI, one of their name is 'alloc_q', which is > for providing mbufs to the kernel side to use. So userspace allocates mbufs and > puts them to 'alloc_q' to be used by kernel side. > Mainly the "kni: fix mbuf allocation" is enough to describe the fix, but it > sounds too generic, "for alloc FIFO" gives more context to clarify which mbuf > allocation we are referring too. > It is also possible to say as below without refering to name of the FIFO: > "kni: fix mbuf allocation for kernel side use" > Is this any better? Yes it looks less confusing, thanks.