patches for DPDK stable branches
 help / color / mirror / Atom feed
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>
Subject: Re: [dpdk-stable] [dpdk-dev] [PATCH v3] kni: fix mbuf allocation for alloc FIFO
Date: Wed, 23 Jun 2021 16:41:25 +0200	[thread overview]
Message-ID: <5642725.EOCvtSPk5b@thomas> (raw)
In-Reply-To: <8b7084b2-12fc-acd6-9cf8-1bc238b77d7d@intel.com>

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.



  reply	other threads:[~2021-06-23 14:41 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <4ebfe0d38b335a437edc9c58368153d005f562ce.1622460655.git.wangyunjian@huawei.com>
2021-06-22 10:57 ` [dpdk-stable] [PATCH v2] kni: fix wrong mbuf alloc count in kni_allocate_mbufs wangyunjian
2021-06-22 12:27   ` Ferruh Yigit
2021-06-22 12:32     ` wangyunjian
2021-06-22 12:44   ` [dpdk-stable] [dpdk-dev] [PATCH v3] kni: fix mbuf allocation for alloc FIFO wangyunjian
2021-06-22 20:46     ` Thomas Monjalon
2021-06-23 12:16       ` wangyunjian
2021-06-23 14:11         ` Ferruh Yigit
2021-06-23 14:41           ` Thomas Monjalon [this message]
2021-06-24  1:55     ` Ajit Khaparde
2021-06-24  7:43       ` Thomas Monjalon

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=5642725.EOCvtSPk5b@thomas \
    --to=thomas@monjalon.net \
    --cc=dev@dpdk.org \
    --cc=dingxiaoxiong@huawei.com \
    --cc=ferruh.yigit@intel.com \
    --cc=gowrishankar.m@linux.vnet.ibm.com \
    --cc=liucheng11@huawei.com \
    --cc=stable@dpdk.org \
    --cc=wangyunjian@huawei.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).