From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-io0-f194.google.com (mail-io0-f194.google.com [209.85.223.194]) by dpdk.org (Postfix) with ESMTP id 642C625A1 for ; Tue, 3 Apr 2018 15:30:16 +0200 (CEST) Received: by mail-io0-f194.google.com with SMTP id b20so22022697iof.5 for ; Tue, 03 Apr 2018 06:30:16 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=6JZVn946W/HPvuw462P4Eza6FByrLgm0FfkKtwgL4f4=; b=R4iQY2bN1ezExYtStA5VdHeshRZyA29WEDgpsh0lb7QKKDQiY19Y31QNasIHaG6ekA X0oG3dbOnvSCdkvUsGSpAHAjZ7+9sMhUSn3zSY1K7nJMxxFCPp1Dy8hgiI7Y3mSM9GSU +UOMf7SjyITz2DNCu/peOmsP+l7GU0y1+ByuAd/yTvzZm/6Eq0bSXeIgFVq/gpSA2g46 m98NPIyyiXvUAZ59iyCvELRnkXh1V4jGTSki1QWLZWi/D832vOI7nfKrYCXK7jFTay8m CbYhV1++w+E0GBwShw3VtM0skIRZUYaXiGVEfrGmcg6C+j/0qp4D1i9RjggKVCtUlVIH L0tg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=6JZVn946W/HPvuw462P4Eza6FByrLgm0FfkKtwgL4f4=; b=Mws/gXorkCJt3CYR1wgEExotH1ijGuPY8MVB01Gw3L4Qg+mlLG2B8T1I6AxQvnGery 9Hs1ws7eWvoIJACptkn6UthVZXB97fdHjNenS9pOMNbwvP9HnVfzI93Wndu9KEPtb/0E fZ9Um2rYxKikQaEAaQTryFmQmYZTKkPfBCqlPMz5+Ji8ftQ2Qk31cITktJT+25RBrwzp QsK6PtGUtZWaLHef7JEgabOv2YyigPpn9ojkZRs6EUQm4f5Zy+xnzLYKXH3wL6EP6/9M rfc70PiS6Ntv7tXf3tsXD7N4ERPjOHVyzKIA1HlPew+sPoHKQWwM7tiVaq4khVAFDPqK OteA== X-Gm-Message-State: ALQs6tAIkPJFni/jB+qCaZxF/fJGqtXgU9iDXgJp+yshAa/bW34yDTZf NBZjK6Di/nTWbXdrGza1wMiBdjjuPw/7dUsXBGE= X-Google-Smtp-Source: AIpwx4+4d4Rcww4wF5CcNfLS/tJWBw2HA1dUOjxY8Lw/oOrqDTT1FhMPHlCFGl1McHZXah9FPR0T4iBBOE8z+qF1Vos= X-Received: by 10.107.34.197 with SMTP id i188mr12185362ioi.287.1522762215335; Tue, 03 Apr 2018 06:30:15 -0700 (PDT) MIME-Version: 1.0 References: In-Reply-To: From: zhouyangchao Date: Tue, 03 Apr 2018 13:30:04 +0000 Message-ID: To: Ferruh Yigit Cc: dev@dpdk.org Content-Type: text/plain; charset="UTF-8" X-Content-Filtered-By: Mailman/MimeDel 2.1.15 Subject: Re: [dpdk-dev] [PATCH] kni: optimize the kni release speed X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 03 Apr 2018 13:30:16 -0000 On Tue, Mar 27, 2018 at 12:44 AM Ferruh Yigit 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 > > --- > > 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); > > > > > >