diff mbox series

[v3] kni: fix mbuf allocation for alloc FIFO

Message ID 1624365869-31872-1-git-send-email-wangyunjian@huawei.com (mailing list archive)
State Accepted, archived
Headers
Series [v3] kni: fix mbuf allocation for alloc FIFO |

Checks

Context Check Description
ci/checkpatch warning coding style issues
ci/github-robot success github build: passed
ci/Intel-compilation success Compilation OK
ci/iol-abi-testing success Testing PASS
ci/iol-testing success Testing PASS
ci/intel-Testing success Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-mellanox-Functional fail Functional Testing issues

Commit Message

Yunjian Wang June 22, 2021, 12:44 p.m. UTC 
  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(-)

Comments

Thomas Monjalon June 22, 2021, 8:46 p.m. UTC | #1 
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?

> +	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?
Yunjian Wang June 23, 2021, 12:16 p.m. UTC | #2 
> -----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
 */

> 
> > +	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"?
Ferruh Yigit June 23, 2021, 2:11 p.m. UTC | #3 
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?

>>
>>> +	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?
Thomas Monjalon June 23, 2021, 2:41 p.m. UTC | #4 
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.
Ajit Khaparde June 24, 2021, 1:55 a.m. UTC | #5 
On Tue, Jun 22, 2021 at 5:44 AM wangyunjian <wangyunjian@huawei.com> wrote:
>
> 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>
Acked-by: Ajit Khaparde <ajit.khaparde@broadcom.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);
> +       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)) {
> --
> 2.23.0
>
Thomas Monjalon June 24, 2021, 7:43 a.m. UTC | #6 
24/06/2021 03:55, Ajit Khaparde:
> On Tue, Jun 22, 2021 at 5:44 AM wangyunjian <wangyunjian@huawei.com> wrote:
> >
> > 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>
> Acked-by: Ajit Khaparde <ajit.khaparde@broadcom.com>

Applied, thanks
diff mbox series

Patch

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);
+	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)) {