* [dpdk-dev] [PATCH] kni: fix wrong mbuf alloc count in kni_allocate_mbufs @ 2021-05-31 12:09 wangyunjian 2021-06-18 13:37 ` Ferruh Yigit 2021-06-22 10:57 ` [dpdk-dev] [PATCH v2] " wangyunjian 0 siblings, 2 replies; 16+ messages in thread From: wangyunjian @ 2021-05-31 12:09 UTC (permalink / raw) To: dev; +Cc: ferruh.yigit, liucheng11, dingxiaoxiong, Yunjian Wang 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 (e.g 32 & (32 - 1) = 0), and it will not fill the alloc_q. When the alloc_q's free count is zero, it will drop the packet in kernel kni. In this patch, we set the allocq_free as the min between MAX_MBUF_BURST_NUM and the free count of the alloc_q. Signed-off-by: Cheng Liu <liucheng11@huawei.com> Signed-off-by: Yunjian Wang <wangyunjian@huawei.com> --- 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..20d8f20cef 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [dpdk-dev] [PATCH] kni: fix wrong mbuf alloc count in kni_allocate_mbufs 2021-05-31 12:09 [dpdk-dev] [PATCH] kni: fix wrong mbuf alloc count in kni_allocate_mbufs wangyunjian @ 2021-06-18 13:37 ` Ferruh Yigit 2021-06-21 3:27 ` wangyunjian 2021-06-22 10:57 ` [dpdk-dev] [PATCH v2] " wangyunjian 1 sibling, 1 reply; 16+ messages in thread From: Ferruh Yigit @ 2021-06-18 13:37 UTC (permalink / raw) To: wangyunjian, dev; +Cc: liucheng11, dingxiaoxiong On 5/31/2021 1:09 PM, wangyunjian 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 (e.g 32 & (32 - 1) = 0), and > it will not fill the alloc_q. When the alloc_q's free count is > zero, it will drop the packet in kernel kni. > nack Both 'read' & 'write' pointers can be max 'len-1', so 'read - write - 1' can't be 'len'. For above example first part can't be '32'. But if you are observing a problem, can you please describe it a little more, it may be because of something else. > In this patch, we set the allocq_free as the min between > MAX_MBUF_BURST_NUM and the free count of the alloc_q. > > Signed-off-by: Cheng Liu <liucheng11@huawei.com> > Signed-off-by: Yunjian Wang <wangyunjian@huawei.com> > --- > 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..20d8f20cef 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)) { > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [dpdk-dev] [PATCH] kni: fix wrong mbuf alloc count in kni_allocate_mbufs 2021-06-18 13:37 ` Ferruh Yigit @ 2021-06-21 3:27 ` wangyunjian 2021-06-21 11:26 ` Ferruh Yigit 0 siblings, 1 reply; 16+ messages in thread From: wangyunjian @ 2021-06-21 3:27 UTC (permalink / raw) To: Ferruh Yigit, dev; +Cc: liucheng (J), dingxiaoxiong > -----Original Message----- > From: Ferruh Yigit [mailto:ferruh.yigit@intel.com] > Sent: Friday, June 18, 2021 9:37 PM > To: wangyunjian <wangyunjian@huawei.com>; dev@dpdk.org > Cc: liucheng (J) <liucheng11@huawei.com>; dingxiaoxiong > <dingxiaoxiong@huawei.com> > Subject: Re: [dpdk-dev] [PATCH] kni: fix wrong mbuf alloc count in > kni_allocate_mbufs > > On 5/31/2021 1:09 PM, wangyunjian 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 (e.g 32 & (32 - 1) = 0), and it > > will not fill the alloc_q. When the alloc_q's free count is zero, it > > will drop the packet in kernel kni. > > > > nack > > Both 'read' & 'write' pointers can be max 'len-1', so 'read - write - 1' can't be > 'len'. > For above example first part can't be '32'. > > But if you are observing a problem, can you please describe it a little more, it > may be because of something else. 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 recieve 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 recieve 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. Thanks > > > In this patch, we set the allocq_free as the min between > > MAX_MBUF_BURST_NUM and the free count of the alloc_q. > > > > Signed-off-by: Cheng Liu <liucheng11@huawei.com> > > Signed-off-by: Yunjian Wang <wangyunjian@huawei.com> > > --- > > 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..20d8f20cef 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)) { > > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [dpdk-dev] [PATCH] kni: fix wrong mbuf alloc count in kni_allocate_mbufs 2021-06-21 3:27 ` wangyunjian @ 2021-06-21 11:26 ` Ferruh Yigit 2021-06-22 7:32 ` wangyunjian 0 siblings, 1 reply; 16+ messages in thread From: Ferruh Yigit @ 2021-06-21 11:26 UTC (permalink / raw) To: wangyunjian, dev; +Cc: liucheng (J), dingxiaoxiong On 6/21/2021 4:27 AM, wangyunjian wrote: >> -----Original Message----- >> From: Ferruh Yigit [mailto:ferruh.yigit@intel.com] >> Sent: Friday, June 18, 2021 9:37 PM >> To: wangyunjian <wangyunjian@huawei.com>; dev@dpdk.org >> Cc: liucheng (J) <liucheng11@huawei.com>; dingxiaoxiong >> <dingxiaoxiong@huawei.com> >> Subject: Re: [dpdk-dev] [PATCH] kni: fix wrong mbuf alloc count in >> kni_allocate_mbufs >> >> On 5/31/2021 1:09 PM, wangyunjian 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 (e.g 32 & (32 - 1) = 0), and it >>> will not fill the alloc_q. When the alloc_q's free count is zero, it >>> will drop the packet in kernel kni. >>> >> >> nack >> >> Both 'read' & 'write' pointers can be max 'len-1', so 'read - write - 1' can't be >> 'len'. >> For above example first part can't be '32'. >> >> But if you are observing a problem, can you please describe it a little more, it >> may be because of something else. > > 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 recieve 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 recieve 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. > I see now, yes it is technically possible to have above scenario and it can cause glitch in the datapath. Below fix looks good, +1 to use 'kni_fifo_free_count()' instead of calculation within the function which may be wrong for the 'RTE_USE_C11_MEM_MODEL' case. Can you please add fixes line too? > Thanks > >> >>> In this patch, we set the allocq_free as the min between >>> MAX_MBUF_BURST_NUM and the free count of the alloc_q. >>> >>> Signed-off-by: Cheng Liu <liucheng11@huawei.com> >>> Signed-off-by: Yunjian Wang <wangyunjian@huawei.com> >>> --- >>> 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..20d8f20cef 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)) { >>> > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [dpdk-dev] [PATCH] kni: fix wrong mbuf alloc count in kni_allocate_mbufs 2021-06-21 11:26 ` Ferruh Yigit @ 2021-06-22 7:32 ` wangyunjian 2021-06-22 7:43 ` Ferruh Yigit 0 siblings, 1 reply; 16+ messages in thread From: wangyunjian @ 2021-06-22 7:32 UTC (permalink / raw) To: Ferruh Yigit, dev; +Cc: liucheng (J), dingxiaoxiong > -----Original Message----- > From: Ferruh Yigit [mailto:ferruh.yigit@intel.com] > Sent: Monday, June 21, 2021 7:26 PM > To: wangyunjian <wangyunjian@huawei.com>; dev@dpdk.org > Cc: liucheng (J) <liucheng11@huawei.com>; dingxiaoxiong > <dingxiaoxiong@huawei.com> > Subject: Re: [dpdk-dev] [PATCH] kni: fix wrong mbuf alloc count in > kni_allocate_mbufs > > On 6/21/2021 4:27 AM, wangyunjian wrote: > >> -----Original Message----- > >> From: Ferruh Yigit [mailto:ferruh.yigit@intel.com] > >> Sent: Friday, June 18, 2021 9:37 PM > >> To: wangyunjian <wangyunjian@huawei.com>; dev@dpdk.org > >> Cc: liucheng (J) <liucheng11@huawei.com>; dingxiaoxiong > >> <dingxiaoxiong@huawei.com> > >> Subject: Re: [dpdk-dev] [PATCH] kni: fix wrong mbuf alloc count in > >> kni_allocate_mbufs > >> > >> On 5/31/2021 1:09 PM, wangyunjian 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 (e.g 32 & (32 - 1) = 0), and it > >>> will not fill the alloc_q. When the alloc_q's free count is zero, it > >>> will drop the packet in kernel kni. > >>> > >> > >> nack > >> > >> Both 'read' & 'write' pointers can be max 'len-1', so 'read - write - > >> 1' can't be 'len'. > >> For above example first part can't be '32'. > >> > >> But if you are observing a problem, can you please describe it a > >> little more, it may be because of something else. > > > > 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 recieve 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 recieve 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. > > > > I see now, yes it is technically possible to have above scenario and it can cause > glitch in the datapath. > > Below fix looks good, +1 to use 'kni_fifo_free_count()' instead of calculation > within the function which may be wrong for the 'RTE_USE_C11_MEM_MODEL' > case. I compiled them on the ARM and x86 platforms with the 'RTE_USE_C11_MEM_MODEL' case, and no error is reported. > > Can you please add fixes line too? OK, will include it in next version. Thanks > > > Thanks > > > >> > >>> In this patch, we set the allocq_free as the min between > >>> MAX_MBUF_BURST_NUM and the free count of the alloc_q. > >>> > >>> Signed-off-by: Cheng Liu <liucheng11@huawei.com> > >>> Signed-off-by: Yunjian Wang <wangyunjian@huawei.com> > >>> --- > >>> 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..20d8f20cef 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)) { > >>> > > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [dpdk-dev] [PATCH] kni: fix wrong mbuf alloc count in kni_allocate_mbufs 2021-06-22 7:32 ` wangyunjian @ 2021-06-22 7:43 ` Ferruh Yigit 0 siblings, 0 replies; 16+ messages in thread From: Ferruh Yigit @ 2021-06-22 7:43 UTC (permalink / raw) To: wangyunjian, dev Cc: liucheng (J), dingxiaoxiong, Honnappa Nagarahalli, Ruifeng Wang (Arm Technology China) On 6/22/2021 8:32 AM, wangyunjian wrote: >> -----Original Message----- >> From: Ferruh Yigit [mailto:ferruh.yigit@intel.com] >> Sent: Monday, June 21, 2021 7:26 PM >> To: wangyunjian <wangyunjian@huawei.com>; dev@dpdk.org >> Cc: liucheng (J) <liucheng11@huawei.com>; dingxiaoxiong >> <dingxiaoxiong@huawei.com> >> Subject: Re: [dpdk-dev] [PATCH] kni: fix wrong mbuf alloc count in >> kni_allocate_mbufs >> >> On 6/21/2021 4:27 AM, wangyunjian wrote: >>>> -----Original Message----- >>>> From: Ferruh Yigit [mailto:ferruh.yigit@intel.com] >>>> Sent: Friday, June 18, 2021 9:37 PM >>>> To: wangyunjian <wangyunjian@huawei.com>; dev@dpdk.org >>>> Cc: liucheng (J) <liucheng11@huawei.com>; dingxiaoxiong >>>> <dingxiaoxiong@huawei.com> >>>> Subject: Re: [dpdk-dev] [PATCH] kni: fix wrong mbuf alloc count in >>>> kni_allocate_mbufs >>>> >>>> On 5/31/2021 1:09 PM, wangyunjian 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 (e.g 32 & (32 - 1) = 0), and it >>>>> will not fill the alloc_q. When the alloc_q's free count is zero, it >>>>> will drop the packet in kernel kni. >>>>> >>>> >>>> nack >>>> >>>> Both 'read' & 'write' pointers can be max 'len-1', so 'read - write - >>>> 1' can't be 'len'. >>>> For above example first part can't be '32'. >>>> >>>> But if you are observing a problem, can you please describe it a >>>> little more, it may be because of something else. >>> >>> 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 recieve 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 recieve 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. >>> >> >> I see now, yes it is technically possible to have above scenario and it can cause >> glitch in the datapath. >> >> Below fix looks good, +1 to use 'kni_fifo_free_count()' instead of calculation >> within the function which may be wrong for the 'RTE_USE_C11_MEM_MODEL' >> case. > > I compiled them on the ARM and x86 platforms with the 'RTE_USE_C11_MEM_MODEL' > case, and no error is reported. > May not be build error, but in 'RTE_USE_C11_MEM_MODEL' case 'read'/'write' are not volatile and need to read them via C11 atomic instructions. 'allocq_free' calculation in the 'kni_allocate_mbufs()' doesn't do that, that is why better to replace calculation with 'kni_fifo_free_count()'. >> >> Can you please add fixes line too? > > OK, will include it in next version. > Thanks. > Thanks > >> >>> Thanks >>> >>>> >>>>> In this patch, we set the allocq_free as the min between >>>>> MAX_MBUF_BURST_NUM and the free count of the alloc_q. >>>>> >>>>> Signed-off-by: Cheng Liu <liucheng11@huawei.com> >>>>> Signed-off-by: Yunjian Wang <wangyunjian@huawei.com> >>>>> --- >>>>> 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..20d8f20cef 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)) { >>>>> >>> > ^ permalink raw reply [flat|nested] 16+ messages in thread
* [dpdk-dev] [PATCH v2] kni: fix wrong mbuf alloc count in kni_allocate_mbufs 2021-05-31 12:09 [dpdk-dev] [PATCH] kni: fix wrong mbuf alloc count in kni_allocate_mbufs wangyunjian 2021-06-18 13:37 ` Ferruh Yigit @ 2021-06-22 10:57 ` wangyunjian 2021-06-22 12:27 ` Ferruh Yigit 2021-06-22 12:44 ` [dpdk-dev] [PATCH v3] kni: fix mbuf allocation for alloc FIFO wangyunjian 1 sibling, 2 replies; 16+ messages in thread From: wangyunjian @ 2021-06-22 10:57 UTC (permalink / raw) To: dev Cc: ferruh.yigit, thomas, gowrishankar.m, dingxiaoxiong, Yunjian Wang, stable, Cheng Liu 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> --- 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [dpdk-dev] [PATCH v2] kni: fix wrong mbuf alloc count in kni_allocate_mbufs 2021-06-22 10:57 ` [dpdk-dev] [PATCH v2] " wangyunjian @ 2021-06-22 12:27 ` Ferruh Yigit 2021-06-22 12:32 ` wangyunjian 2021-06-22 12:44 ` [dpdk-dev] [PATCH v3] kni: fix mbuf allocation for alloc FIFO wangyunjian 1 sibling, 1 reply; 16+ messages in thread From: Ferruh Yigit @ 2021-06-22 12:27 UTC (permalink / raw) To: wangyunjian, dev; +Cc: thomas, gowrishankar.m, dingxiaoxiong, stable, Cheng Liu On 6/22/2021 11:57 AM, wangyunjian 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> What do you think to change patch title to something like: kni: fix mbuf allocation for alloc FIFO ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [dpdk-dev] [PATCH v2] kni: fix wrong mbuf alloc count in kni_allocate_mbufs 2021-06-22 12:27 ` Ferruh Yigit @ 2021-06-22 12:32 ` wangyunjian 0 siblings, 0 replies; 16+ messages in thread From: wangyunjian @ 2021-06-22 12:32 UTC (permalink / raw) To: Ferruh Yigit, dev Cc: thomas, gowrishankar.m, dingxiaoxiong, stable, liucheng (J) > -----Original Message----- > From: Ferruh Yigit [mailto:ferruh.yigit@intel.com] > Sent: Tuesday, June 22, 2021 8:28 PM > To: wangyunjian <wangyunjian@huawei.com>; dev@dpdk.org > Cc: thomas@monjalon.net; gowrishankar.m@linux.vnet.ibm.com; > dingxiaoxiong <dingxiaoxiong@huawei.com>; stable@dpdk.org; liucheng (J) > <liucheng11@huawei.com> > Subject: Re: [PATCH v2] kni: fix wrong mbuf alloc count in kni_allocate_mbufs > > On 6/22/2021 11:57 AM, wangyunjian 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> > > What do you think to change patch title to something like: > kni: fix mbuf allocation for alloc FIFO OK, I will change patch title later. Thanks ^ permalink raw reply [flat|nested] 16+ messages in thread
* [dpdk-dev] [PATCH v3] kni: fix mbuf allocation for alloc FIFO 2021-06-22 10:57 ` [dpdk-dev] [PATCH v2] " wangyunjian 2021-06-22 12:27 ` Ferruh Yigit @ 2021-06-22 12:44 ` wangyunjian 2021-06-22 20:46 ` [dpdk-dev] [dpdk-stable] " Thomas Monjalon 2021-06-24 1:55 ` Ajit Khaparde 1 sibling, 2 replies; 16+ messages in thread From: wangyunjian @ 2021-06-22 12:44 UTC (permalink / raw) To: dev Cc: ferruh.yigit, thomas, gowrishankar.m, dingxiaoxiong, Yunjian Wang, stable, Cheng Liu 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); + 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [dpdk-dev] [dpdk-stable] [PATCH v3] kni: fix mbuf allocation for alloc FIFO 2021-06-22 12:44 ` [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-24 1:55 ` Ajit Khaparde 1 sibling, 1 reply; 16+ messages in thread From: Thomas Monjalon @ 2021-06-22 20:46 UTC (permalink / raw) To: Yunjian Wang, Cheng Liu Cc: dev, stable, ferruh.yigit, gowrishankar.m, dingxiaoxiong, wangyunjian 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? ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [dpdk-dev] [dpdk-stable] [PATCH v3] kni: fix mbuf allocation for alloc FIFO 2021-06-22 20:46 ` [dpdk-dev] [dpdk-stable] " Thomas Monjalon @ 2021-06-23 12:16 ` wangyunjian 2021-06-23 14:11 ` Ferruh Yigit 0 siblings, 1 reply; 16+ messages in thread From: wangyunjian @ 2021-06-23 12:16 UTC (permalink / raw) To: Thomas Monjalon Cc: dev, stable, ferruh.yigit, gowrishankar.m, dingxiaoxiong, liucheng (J) > -----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"? ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [dpdk-dev] [dpdk-stable] [PATCH v3] kni: fix mbuf allocation for alloc FIFO 2021-06-23 12:16 ` wangyunjian @ 2021-06-23 14:11 ` Ferruh Yigit 2021-06-23 14:41 ` Thomas Monjalon 0 siblings, 1 reply; 16+ messages in thread From: Ferruh Yigit @ 2021-06-23 14:11 UTC (permalink / raw) To: wangyunjian, Thomas Monjalon Cc: dev, stable, gowrishankar.m, dingxiaoxiong, liucheng (J) 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? ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [dpdk-dev] [dpdk-stable] [PATCH v3] kni: fix mbuf allocation for alloc FIFO 2021-06-23 14:11 ` Ferruh Yigit @ 2021-06-23 14:41 ` Thomas Monjalon 0 siblings, 0 replies; 16+ messages in thread From: Thomas Monjalon @ 2021-06-23 14:41 UTC (permalink / raw) To: wangyunjian, Ferruh Yigit Cc: dev, stable, gowrishankar.m, dingxiaoxiong, liucheng (J) 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. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [dpdk-dev] [dpdk-stable] [PATCH v3] kni: fix mbuf allocation for alloc FIFO 2021-06-22 12:44 ` [dpdk-dev] [PATCH v3] kni: fix mbuf allocation for alloc FIFO wangyunjian 2021-06-22 20:46 ` [dpdk-dev] [dpdk-stable] " Thomas Monjalon @ 2021-06-24 1:55 ` Ajit Khaparde 2021-06-24 7:43 ` Thomas Monjalon 1 sibling, 1 reply; 16+ messages in thread From: Ajit Khaparde @ 2021-06-24 1:55 UTC (permalink / raw) To: wangyunjian Cc: dpdk-dev, Ferruh Yigit, Thomas Monjalon, gowrishankar.m, dingxiaoxiong, dpdk stable, Cheng Liu [-- Attachment #1: Type: text/plain, Size: 2447 bytes --] 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 > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [dpdk-dev] [dpdk-stable] [PATCH v3] kni: fix mbuf allocation for alloc FIFO 2021-06-24 1:55 ` Ajit Khaparde @ 2021-06-24 7:43 ` Thomas Monjalon 0 siblings, 0 replies; 16+ messages in thread From: Thomas Monjalon @ 2021-06-24 7:43 UTC (permalink / raw) To: wangyunjian Cc: stable, dpdk-dev, Ferruh Yigit, gowrishankar.m, dingxiaoxiong, dpdk stable, Cheng Liu, Ajit Khaparde 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2021-06-24 7:43 UTC | newest] Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-05-31 12:09 [dpdk-dev] [PATCH] kni: fix wrong mbuf alloc count in kni_allocate_mbufs wangyunjian 2021-06-18 13:37 ` Ferruh Yigit 2021-06-21 3:27 ` wangyunjian 2021-06-21 11:26 ` Ferruh Yigit 2021-06-22 7:32 ` wangyunjian 2021-06-22 7:43 ` Ferruh Yigit 2021-06-22 10:57 ` [dpdk-dev] [PATCH v2] " wangyunjian 2021-06-22 12:27 ` Ferruh Yigit 2021-06-22 12:32 ` wangyunjian 2021-06-22 12:44 ` [dpdk-dev] [PATCH v3] kni: fix mbuf allocation for alloc FIFO wangyunjian 2021-06-22 20:46 ` [dpdk-dev] [dpdk-stable] " Thomas Monjalon 2021-06-23 12:16 ` wangyunjian 2021-06-23 14:11 ` Ferruh Yigit 2021-06-23 14:41 ` Thomas Monjalon 2021-06-24 1:55 ` Ajit Khaparde 2021-06-24 7:43 ` Thomas Monjalon
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).