Re: [dpdk-dev] [dpdk-stable] [PATCH v3] kni: fix mbuf allocation for alloc FIFO

[Thomas Monjalon <thomas@monjalon.net>]

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.