* [dpdk-dev] [PATCH] kni: optimize the kni release speed
@ 2018-02-06 10:33 zhouyangchao
2018-03-26 16:44 ` Ferruh Yigit
0 siblings, 1 reply; 4+ messages in thread
From: zhouyangchao @ 2018-02-06 10:33 UTC (permalink / raw)
To: ferruh.yigit; +Cc: dev
Physical addresses in the fifo named alloc_q need to be traversed to
release in user space. The physical address to the virtual address
conversion in kernel space is much better.
Signed-off-by: Yangchao Zhou <zhouyates@gmail.com>
---
lib/librte_eal/linuxapp/kni/kni_dev.h | 1 +
lib/librte_eal/linuxapp/kni/kni_misc.c | 1 +
lib/librte_eal/linuxapp/kni/kni_net.c | 15 +++++++++++++++
lib/librte_kni/rte_kni.c | 26 +-------------------------
4 files changed, 18 insertions(+), 25 deletions(-)
diff --git a/lib/librte_eal/linuxapp/kni/kni_dev.h b/lib/librte_eal/linuxapp/kni/kni_dev.h
index c9393d8..7cd9bf8 100644
--- a/lib/librte_eal/linuxapp/kni/kni_dev.h
+++ b/lib/librte_eal/linuxapp/kni/kni_dev.h
@@ -92,6 +92,7 @@ struct kni_dev {
void *alloc_va[MBUF_BURST_SZ];
};
+void kni_net_fifo_pa2va(struct kni_dev *kni);
void kni_net_rx(struct kni_dev *kni);
void kni_net_init(struct net_device *dev);
void kni_net_config_lo_mode(char *lo_str);
diff --git a/lib/librte_eal/linuxapp/kni/kni_misc.c b/lib/librte_eal/linuxapp/kni/kni_misc.c
index 01574ec..668488b 100644
--- a/lib/librte_eal/linuxapp/kni/kni_misc.c
+++ b/lib/librte_eal/linuxapp/kni/kni_misc.c
@@ -507,6 +507,7 @@ kni_ioctl_release(struct net *net, uint32_t ioctl_num,
dev->pthread = NULL;
}
+ kni_net_fifo_pa2va(dev);
kni_dev_remove(dev);
list_del(&dev->list);
ret = 0;
diff --git a/lib/librte_eal/linuxapp/kni/kni_net.c b/lib/librte_eal/linuxapp/kni/kni_net.c
index 9f9b798..662a527 100644
--- a/lib/librte_eal/linuxapp/kni/kni_net.c
+++ b/lib/librte_eal/linuxapp/kni/kni_net.c
@@ -73,6 +73,21 @@ va2pa(void *va, struct rte_kni_mbuf *m)
return pa;
}
+/* convert physical addresses to virtual addresses in fifo for kni release */
+void
+kni_net_fifo_pa2va(struct kni_dev *kni)
+{
+ void *fifo = kni->alloc_q;
+ int i, count = kni_fifo_count(fifo);
+ void *pa = NULL, *kva, *va;
+ for (i = 0; i < count; ++i) {
+ (void)kni_fifo_get(fifo, &pa, 1);
+ kva = pa2kva(pa);
+ va = pa2va(pa, kva);
+ (void)kni_fifo_put(fifo, &va, 1);
+ }
+}
+
/*
* It can be called to process the request.
*/
diff --git a/lib/librte_kni/rte_kni.c b/lib/librte_kni/rte_kni.c
index 2867411..f8398a9 100644
--- a/lib/librte_kni/rte_kni.c
+++ b/lib/librte_kni/rte_kni.c
@@ -435,30 +435,6 @@ va2pa(struct rte_mbuf *m)
(unsigned long)m->buf_iova));
}
-static void
-obj_free(struct rte_mempool *mp __rte_unused, void *opaque, void *obj,
- unsigned obj_idx __rte_unused)
-{
- struct rte_mbuf *m = obj;
- void *mbuf_phys = opaque;
-
- if (va2pa(m) == mbuf_phys)
- rte_pktmbuf_free(m);
-}
-
-static void
-kni_free_fifo_phy(struct rte_mempool *mp, struct rte_kni_fifo *fifo)
-{
- void *mbuf_phys;
- int ret;
-
- do {
- ret = kni_fifo_get(fifo, &mbuf_phys, 1);
- if (ret)
- rte_mempool_obj_iter(mp, obj_free, mbuf_phys);
- } while (ret);
-}
-
int
rte_kni_release(struct rte_kni *kni)
{
@@ -484,7 +460,7 @@ rte_kni_release(struct rte_kni *kni)
if (kni_fifo_count(kni->rx_q))
RTE_LOG(ERR, KNI, "Fail to free all Rx-q items\n");
- kni_free_fifo_phy(kni->pktmbuf_pool, kni->alloc_q);
+ kni_free_fifo(kni->alloc_q);
kni_free_fifo(kni->tx_q);
kni_free_fifo(kni->free_q);
--
2.9.0.windows.1
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [dpdk-dev] [PATCH] kni: optimize the kni release speed
2018-02-06 10:33 [dpdk-dev] [PATCH] kni: optimize the kni release speed zhouyangchao
@ 2018-03-26 16:44 ` Ferruh Yigit
2018-04-03 13:30 ` zhouyangchao
0 siblings, 1 reply; 4+ messages in thread
From: Ferruh Yigit @ 2018-03-26 16:44 UTC (permalink / raw)
To: zhouyangchao; +Cc: dev
On 2/6/2018 10:33 AM, zhouyangchao wrote:
> Physical addresses in the fifo named alloc_q need to be traversed to
> release in user space. The physical address to the virtual address
> conversion in kernel space is much better.
Yes current approach should be slower but this is not in data path, this is when
a kni interface released, I expect no recognizable difference.
>
> Signed-off-by: Yangchao Zhou <zhouyates@gmail.com>
> ---
> lib/librte_eal/linuxapp/kni/kni_dev.h | 1 +
> lib/librte_eal/linuxapp/kni/kni_misc.c | 1 +
> lib/librte_eal/linuxapp/kni/kni_net.c | 15 +++++++++++++++
> lib/librte_kni/rte_kni.c | 26 +-------------------------
> 4 files changed, 18 insertions(+), 25 deletions(-)
>
> diff --git a/lib/librte_eal/linuxapp/kni/kni_dev.h b/lib/librte_eal/linuxapp/kni/kni_dev.h
> index c9393d8..7cd9bf8 100644
> --- a/lib/librte_eal/linuxapp/kni/kni_dev.h
> +++ b/lib/librte_eal/linuxapp/kni/kni_dev.h
> @@ -92,6 +92,7 @@ struct kni_dev {
> void *alloc_va[MBUF_BURST_SZ];
> };
>
> +void kni_net_fifo_pa2va(struct kni_dev *kni);
> void kni_net_rx(struct kni_dev *kni);
> void kni_net_init(struct net_device *dev);
> void kni_net_config_lo_mode(char *lo_str);
> diff --git a/lib/librte_eal/linuxapp/kni/kni_misc.c b/lib/librte_eal/linuxapp/kni/kni_misc.c
> index 01574ec..668488b 100644
> --- a/lib/librte_eal/linuxapp/kni/kni_misc.c
> +++ b/lib/librte_eal/linuxapp/kni/kni_misc.c
> @@ -507,6 +507,7 @@ kni_ioctl_release(struct net *net, uint32_t ioctl_num,
> dev->pthread = NULL;
> }
>
> + kni_net_fifo_pa2va(dev);
> kni_dev_remove(dev);
> list_del(&dev->list);
> ret = 0;
> diff --git a/lib/librte_eal/linuxapp/kni/kni_net.c b/lib/librte_eal/linuxapp/kni/kni_net.c
> index 9f9b798..662a527 100644
> --- a/lib/librte_eal/linuxapp/kni/kni_net.c
> +++ b/lib/librte_eal/linuxapp/kni/kni_net.c
> @@ -73,6 +73,21 @@ va2pa(void *va, struct rte_kni_mbuf *m)
> return pa;
> }
>
> +/* convert physical addresses to virtual addresses in fifo for kni release */
> +void
> +kni_net_fifo_pa2va(struct kni_dev *kni)
> +{
> + void *fifo = kni->alloc_q;
> + int i, count = kni_fifo_count(fifo);
> + void *pa = NULL, *kva, *va;
> + for (i = 0; i < count; ++i) {
> + (void)kni_fifo_get(fifo, &pa, 1);
> + kva = pa2kva(pa);
> + va = pa2va(pa, kva);
> + (void)kni_fifo_put(fifo, &va, 1);
kni fifo are single producer, single consumer. For alloc_q kernel side is
consumer, I aware at this stage applications should stop the traffic, but still
I am not comfortable mixing producer/consumer roles here.
Also alloc_q should have physical addresses this logic stores virtual addresses
in it and not sure about this either to mix addressing logic in the queue.
Instead of this conversion, what about moving from alloc_q to free_q? free_q
already has virtual addresses and freed by userspace, so this will be safe.
I suggest keeping alloc_q free logic in the userspace in any case, if alloc_q is
free it won't cost anyway.
And while checking for this I may found something else. We have same problem
with rx_q, it has physical addresses which makes hard to free in userspace. The
existing intention is to give some time to kernel to consume the rx_q so that it
won't be an issue for userspace. But that logic can be wrong.
During the time userspace waits the netdev may be already destroyed and there is
nothing to receive the packet, perhaps we should move wait above the ioctl.
Since you are already checking these parts perhaps you would like to comment :)
> + }
> +}
> +
> /*
> * It can be called to process the request.
> */
> diff --git a/lib/librte_kni/rte_kni.c b/lib/librte_kni/rte_kni.c
> index 2867411..f8398a9 100644
> --- a/lib/librte_kni/rte_kni.c
> +++ b/lib/librte_kni/rte_kni.c
> @@ -435,30 +435,6 @@ va2pa(struct rte_mbuf *m)
> (unsigned long)m->buf_iova));
> }
>
> -static void
> -obj_free(struct rte_mempool *mp __rte_unused, void *opaque, void *obj,
> - unsigned obj_idx __rte_unused)
> -{
> - struct rte_mbuf *m = obj;
> - void *mbuf_phys = opaque;
> -
> - if (va2pa(m) == mbuf_phys)
> - rte_pktmbuf_free(m);
> -}
> -
> -static void
> -kni_free_fifo_phy(struct rte_mempool *mp, struct rte_kni_fifo *fifo)
> -{
> - void *mbuf_phys;
> - int ret;
> -
> - do {
> - ret = kni_fifo_get(fifo, &mbuf_phys, 1);
> - if (ret)
> - rte_mempool_obj_iter(mp, obj_free, mbuf_phys);
> - } while (ret);
> -}
> -
> int
> rte_kni_release(struct rte_kni *kni)
> {
> @@ -484,7 +460,7 @@ rte_kni_release(struct rte_kni *kni)
> if (kni_fifo_count(kni->rx_q))
> RTE_LOG(ERR, KNI, "Fail to free all Rx-q items\n");
>
> - kni_free_fifo_phy(kni->pktmbuf_pool, kni->alloc_q);
> + kni_free_fifo(kni->alloc_q);
> kni_free_fifo(kni->tx_q);
> kni_free_fifo(kni->free_q);
>
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [dpdk-dev] [PATCH] kni: optimize the kni release speed
2018-03-26 16:44 ` Ferruh Yigit
@ 2018-04-03 13:30 ` zhouyangchao
0 siblings, 0 replies; 4+ messages in thread
From: zhouyangchao @ 2018-04-03 13:30 UTC (permalink / raw)
To: Ferruh Yigit; +Cc: dev
On Tue, Mar 27, 2018 at 12:44 AM Ferruh Yigit <ferruh.yigit@intel.com>
wrote:
> On 2/6/2018 10:33 AM, zhouyangchao wrote:
> > Physical addresses in the fifo named alloc_q need to be traversed to
> > release in user space. The physical address to the virtual address
> > conversion in kernel space is much better.
>
> Yes current approach should be slower but this is not in data path, this
> is when
> a kni interface released, I expect no recognizable difference.
>
>
I changed the number of pre-allocation mbufs to 512 to improve performance.
It cost nearly 3 seconds to release mbufs with a mbuf pool that has more
than 131072 mbufs. I need to lock the forwarding threads for some reason.
> >
> > Signed-off-by: Yangchao Zhou <zhouyates@gmail.com>
> > ---
> > lib/librte_eal/linuxapp/kni/kni_dev.h | 1 +
> > lib/librte_eal/linuxapp/kni/kni_misc.c | 1 +
> > lib/librte_eal/linuxapp/kni/kni_net.c | 15 +++++++++++++++
> > lib/librte_kni/rte_kni.c | 26 +-------------------------
> > 4 files changed, 18 insertions(+), 25 deletions(-)
> >
> > diff --git a/lib/librte_eal/linuxapp/kni/kni_dev.h
> b/lib/librte_eal/linuxapp/kni/kni_dev.h
> > index c9393d8..7cd9bf8 100644
> > --- a/lib/librte_eal/linuxapp/kni/kni_dev.h
> > +++ b/lib/librte_eal/linuxapp/kni/kni_dev.h
> > @@ -92,6 +92,7 @@ struct kni_dev {
> > void *alloc_va[MBUF_BURST_SZ];
> > };
> >
> > +void kni_net_fifo_pa2va(struct kni_dev *kni);
> > void kni_net_rx(struct kni_dev *kni);
> > void kni_net_init(struct net_device *dev);
> > void kni_net_config_lo_mode(char *lo_str);
> > diff --git a/lib/librte_eal/linuxapp/kni/kni_misc.c
> b/lib/librte_eal/linuxapp/kni/kni_misc.c
> > index 01574ec..668488b 100644
> > --- a/lib/librte_eal/linuxapp/kni/kni_misc.c
> > +++ b/lib/librte_eal/linuxapp/kni/kni_misc.c
> > @@ -507,6 +507,7 @@ kni_ioctl_release(struct net *net, uint32_t
> ioctl_num,
> > dev->pthread = NULL;
> > }
> >
> > + kni_net_fifo_pa2va(dev);
> > kni_dev_remove(dev);
> > list_del(&dev->list);
> > ret = 0;
> > diff --git a/lib/librte_eal/linuxapp/kni/kni_net.c
> b/lib/librte_eal/linuxapp/kni/kni_net.c
> > index 9f9b798..662a527 100644
> > --- a/lib/librte_eal/linuxapp/kni/kni_net.c
> > +++ b/lib/librte_eal/linuxapp/kni/kni_net.c
> > @@ -73,6 +73,21 @@ va2pa(void *va, struct rte_kni_mbuf *m)
> > return pa;
> > }
> >
> > +/* convert physical addresses to virtual addresses in fifo for kni
> release */
> > +void
> > +kni_net_fifo_pa2va(struct kni_dev *kni)
> > +{
> > + void *fifo = kni->alloc_q;
> > + int i, count = kni_fifo_count(fifo);
> > + void *pa = NULL, *kva, *va;
> > + for (i = 0; i < count; ++i) {
> > + (void)kni_fifo_get(fifo, &pa, 1);
> > + kva = pa2kva(pa);
> > + va = pa2va(pa, kva);
> > + (void)kni_fifo_put(fifo, &va, 1);
>
> kni fifo are single producer, single consumer. For alloc_q kernel side is
> consumer, I aware at this stage applications should stop the traffic, but
> still
> I am not comfortable mixing producer/consumer roles here.
>
> Also alloc_q should have physical addresses this logic stores virtual
> addresses
> in it and not sure about this either to mix addressing logic in the queue.
>
> Instead of this conversion, what about moving from alloc_q to free_q?
> free_q
> already has virtual addresses and freed by userspace, so this will be safe.
> I suggest keeping alloc_q free logic in the userspace in any case, if
> alloc_q is
> free it won't cost anyway.
>
>
>
This is a very valuable suggestion. My implementation is not compatible
and needs to upgrade rte_kni.ko and application at the same time.
> And while checking for this I may found something else. We have same
> problem
> with rx_q, it has physical addresses which makes hard to free in
> userspace. The
> existing intention is to give some time to kernel to consume the rx_q so
> that it
> won't be an issue for userspace. But that logic can be wrong.
> During the time userspace waits the netdev may be already destroyed and
> there is
> nothing to receive the packet, perhaps we should move wait above the ioctl.
> Since you are already checking these parts perhaps you would like to
> comment :)
>
>
In the actual scene, its impact on time is small. The mbuf leak in
kni_release does not occur in single-threaded mode, but in
multi-threaded mode. I didn't realize this leak before because I've been
using single-threaded mode. I suggest moving rx_q to free_q first and then
moving alloc_q to free_q when kni release in kernel space.
> > + }
> > +}
> > +
> > /*
> > * It can be called to process the request.
> > */
> > diff --git a/lib/librte_kni/rte_kni.c b/lib/librte_kni/rte_kni.c
> > index 2867411..f8398a9 100644
> > --- a/lib/librte_kni/rte_kni.c
> > +++ b/lib/librte_kni/rte_kni.c
> > @@ -435,30 +435,6 @@ va2pa(struct rte_mbuf *m)
> > (unsigned long)m->buf_iova));
> > }
> >
> > -static void
> > -obj_free(struct rte_mempool *mp __rte_unused, void *opaque, void *obj,
> > - unsigned obj_idx __rte_unused)
> > -{
> > - struct rte_mbuf *m = obj;
> > - void *mbuf_phys = opaque;
> > -
> > - if (va2pa(m) == mbuf_phys)
> > - rte_pktmbuf_free(m);
> > -}
> > -
> > -static void
> > -kni_free_fifo_phy(struct rte_mempool *mp, struct rte_kni_fifo *fifo)
> > -{
> > - void *mbuf_phys;
> > - int ret;
> > -
> > - do {
> > - ret = kni_fifo_get(fifo, &mbuf_phys, 1);
> > - if (ret)
> > - rte_mempool_obj_iter(mp, obj_free, mbuf_phys);
> > - } while (ret);
> > -}
> > -
> > int
> > rte_kni_release(struct rte_kni *kni)
> > {
> > @@ -484,7 +460,7 @@ rte_kni_release(struct rte_kni *kni)
> > if (kni_fifo_count(kni->rx_q))
> > RTE_LOG(ERR, KNI, "Fail to free all Rx-q items\n");
> >
> > - kni_free_fifo_phy(kni->pktmbuf_pool, kni->alloc_q);
> > + kni_free_fifo(kni->alloc_q);
> > kni_free_fifo(kni->tx_q);
> > kni_free_fifo(kni->free_q);
> >
> >
>
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* [dpdk-dev] [PATCH] kni: optimize the kni release speed
@ 2018-02-06 10:23 zhouyangchao
0 siblings, 0 replies; 4+ messages in thread
From: zhouyangchao @ 2018-02-06 10:23 UTC (permalink / raw)
To: ferruh.yigit; +Cc: dev
Physical addresses in the fifo named alloc_q need to be traversed to release
in user space. The physical address to the virtual address conversion in
kernel space is much better.
Signed-off-by: Yangchao Zhou <zhouyates@gmail.com>
---
lib/librte_eal/linuxapp/kni/kni_dev.h | 1 +
lib/librte_eal/linuxapp/kni/kni_misc.c | 1 +
lib/librte_eal/linuxapp/kni/kni_net.c | 15 +++++++++++++++
lib/librte_kni/rte_kni.c | 26 +-------------------------
4 files changed, 18 insertions(+), 25 deletions(-)
diff --git a/lib/librte_eal/linuxapp/kni/kni_dev.h b/lib/librte_eal/linuxapp/kni/kni_dev.h
index c9393d8..7cd9bf8 100644
--- a/lib/librte_eal/linuxapp/kni/kni_dev.h
+++ b/lib/librte_eal/linuxapp/kni/kni_dev.h
@@ -92,6 +92,7 @@ struct kni_dev {
void *alloc_va[MBUF_BURST_SZ];
};
+void kni_net_fifo_pa2va(struct kni_dev *kni);
void kni_net_rx(struct kni_dev *kni);
void kni_net_init(struct net_device *dev);
void kni_net_config_lo_mode(char *lo_str);
diff --git a/lib/librte_eal/linuxapp/kni/kni_misc.c b/lib/librte_eal/linuxapp/kni/kni_misc.c
index 01574ec..668488b 100644
--- a/lib/librte_eal/linuxapp/kni/kni_misc.c
+++ b/lib/librte_eal/linuxapp/kni/kni_misc.c
@@ -507,6 +507,7 @@ kni_ioctl_release(struct net *net, uint32_t ioctl_num,
dev->pthread = NULL;
}
+ kni_net_fifo_pa2va(dev);
kni_dev_remove(dev);
list_del(&dev->list);
ret = 0;
diff --git a/lib/librte_eal/linuxapp/kni/kni_net.c b/lib/librte_eal/linuxapp/kni/kni_net.c
index 9f9b798..662a527 100644
--- a/lib/librte_eal/linuxapp/kni/kni_net.c
+++ b/lib/librte_eal/linuxapp/kni/kni_net.c
@@ -73,6 +73,21 @@ va2pa(void *va, struct rte_kni_mbuf *m)
return pa;
}
+/* convert physical addresses to virtual addresses in fifo for kni release */
+void
+kni_net_fifo_pa2va(struct kni_dev *kni)
+{
+ void *fifo = kni->alloc_q;
+ int i, count = kni_fifo_count(fifo);
+ void *pa = NULL, *kva, *va;
+ for (i = 0; i < count; ++i) {
+ (void)kni_fifo_get(fifo, &pa, 1);
+ kva = pa2kva(pa);
+ va = pa2va(pa, kva);
+ (void)kni_fifo_put(fifo, &va, 1);
+ }
+}
+
/*
* It can be called to process the request.
*/
diff --git a/lib/librte_kni/rte_kni.c b/lib/librte_kni/rte_kni.c
index 2867411..f8398a9 100644
--- a/lib/librte_kni/rte_kni.c
+++ b/lib/librte_kni/rte_kni.c
@@ -435,30 +435,6 @@ va2pa(struct rte_mbuf *m)
(unsigned long)m->buf_iova));
}
-static void
-obj_free(struct rte_mempool *mp __rte_unused, void *opaque, void *obj,
- unsigned obj_idx __rte_unused)
-{
- struct rte_mbuf *m = obj;
- void *mbuf_phys = opaque;
-
- if (va2pa(m) == mbuf_phys)
- rte_pktmbuf_free(m);
-}
-
-static void
-kni_free_fifo_phy(struct rte_mempool *mp, struct rte_kni_fifo *fifo)
-{
- void *mbuf_phys;
- int ret;
-
- do {
- ret = kni_fifo_get(fifo, &mbuf_phys, 1);
- if (ret)
- rte_mempool_obj_iter(mp, obj_free, mbuf_phys);
- } while (ret);
-}
-
int
rte_kni_release(struct rte_kni *kni)
{
@@ -484,7 +460,7 @@ rte_kni_release(struct rte_kni *kni)
if (kni_fifo_count(kni->rx_q))
RTE_LOG(ERR, KNI, "Fail to free all Rx-q items\n");
- kni_free_fifo_phy(kni->pktmbuf_pool, kni->alloc_q);
+ kni_free_fifo(kni->alloc_q);
kni_free_fifo(kni->tx_q);
kni_free_fifo(kni->free_q);
--
2.9.0.windows.1
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2018-04-03 13:30 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-02-06 10:33 [dpdk-dev] [PATCH] kni: optimize the kni release speed zhouyangchao
2018-03-26 16:44 ` Ferruh Yigit
2018-04-03 13:30 ` zhouyangchao
-- strict thread matches above, loose matches on Subject: below --
2018-02-06 10:23 zhouyangchao
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).