On Tue, Mar 27, 2018 at 12:44 AM Ferruh Yigit <ferruh.yi...@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 <zhouya...@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); > > > > > >