From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <dev-bounces@dpdk.org>
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: <xms:GEjTYEIX3Ot5DFftlQ9khMIQx9TgDfBwa7n56oicjN3FNQf8F4Tvvg>
<xme:GEjTYEJPaR53n-qGLHFIo37GtM98cBsebq1j7zv5jRZvQTSZRGCCX5b8wTpbT9QQn
2QuJxVKvtGVUK79jQ>
X-ME-Received: <xmr:GEjTYEtjRNTERJiFQT-h8Ta7eEh4XXyG92f6a-afn_i-yaeQ-E6HWXxjD217n_AQTFU-4-loUfKBhTIwtGq0-hhgEQ>
X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgeduledrfeegfedgjeelucetufdoteggodetrfdotf
fvucfrrhhofhhilhgvmecuhfgrshhtofgrihhlpdfqfgfvpdfurfetoffkrfgpnffqhgen
uceurghilhhouhhtmecufedttdenucesvcftvggtihhpihgvnhhtshculddquddttddmne
cujfgurhephffvufffkfgjfhgggfgtsehtufertddttddvnecuhfhrohhmpefvhhhomhgr
shcuofhonhhjrghlohhnuceothhhohhmrghssehmohhnjhgrlhhonhdrnhgvtheqnecugg
ftrfgrthhtvghrnhepudeggfdvfeduffdtfeeglefghfeukefgfffhueejtdetuedtjeeu
ieeivdffgeehnecuvehluhhsthgvrhfuihiivgeptdenucfrrghrrghmpehmrghilhhfrh
homhepthhhohhmrghssehmohhnjhgrlhhonhdrnhgvth
X-ME-Proxy: <xmx:GEjTYBYVW6r1kBcOq3S63F36No8fYfJCLd842cxDXexx3tzob4aAsQ>
<xmx:GEjTYLaCRR4OkMRrUtPgkFcIdai3WSeMY2iAxLRzYRZX2Fz-c-WyHA>
<xmx:GEjTYNDHtivxPcns1C__r-uHk3u25cyFOHeY5j3thagGYxM99f2VcQ>
<xmx:GUjTYGxa-AKVy7iRz16Hti7JT2xDOIjfCmQ6UETwnhLFdUk6RZMMQQ>
Received: by mail.messagingengine.com (Postfix) with ESMTPA; Wed,
23 Jun 2021 10:41:27 -0400 (EDT)
From: Thomas Monjalon <thomas@monjalon.net>
To: wangyunjian <wangyunjian@huawei.com>, Ferruh Yigit <ferruh.yigit@intel.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>, "stable@dpdk.org" <stable@dpdk.org>,
"gowrishankar.m@linux.vnet.ibm.com" <gowrishankar.m@linux.vnet.ibm.com>,
dingxiaoxiong <dingxiaoxiong@huawei.com>,
"liucheng (J)" <liucheng11@huawei.com>
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>
<aa1afe7f17e74f0da8bca632c8ad6226@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 <dev.dpdk.org>
List-Unsubscribe: <https://mails.dpdk.org/options/dev>,
<mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://mails.dpdk.org/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://mails.dpdk.org/listinfo/dev>,
<mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-bounces@dpdk.org
Sender: "dev" <dev-bounces@dpdk.org>
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 <wangyunjian@huawei.com>; liucheng (J)
> >> <liucheng11@huawei.com>
> >> Cc: dev@dpdk.org; stable@dpdk.org; ferruh.yigit@intel.com;
> >> gowrishankar.m@linux.vnet.ibm.com; dingxiaoxiong
> >> <dingxiaoxiong@huawei.com>; wangyunjian <wangyunjian@huawei.com>
> >> Subject: Re: [dpdk-stable] [dpdk-dev] [PATCH v3] kni: fix mbuf allocation for alloc
> >> FIFO
> >>
> >> 22/06/2021 14:44, wangyunjian:
> >>> From: Yunjian Wang <wangyunjian@huawei.com>
> >>>
> >>> 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 <liucheng11@huawei.com>
> >>> Signed-off-by: Yunjian Wang <wangyunjian@huawei.com>
> >>> Acked-by: Ferruh Yigit <ferruh.yigit@intel.com>
> >>> ---
> >>> 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.