[dpdk-dev] kni: optimize the kni release speed

  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(-)
  

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

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