From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ferruh Yigit Subject: Re: [PATCH v3] kni: fix possible rx_q mbuf leaks and speed up alloc_q release Date: Tue, 17 Apr 2018 20:11:41 +0100 Message-ID: <9a57896f-a2db-7f4b-f5c4-cf04970f1636@intel.com> References: <2191a738-7443-464e-8600-eedcaf160593@zyc-PC.local> <1523937700-13716-1-git-send-email-zhouyates@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Cc: stable@dpdk.org, thomas@monjalon.net To: Yangchao Zhou , dev@dpdk.org Return-path: In-Reply-To: <1523937700-13716-1-git-send-email-zhouyates@gmail.com> Content-Language: en-US List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On 4/17/2018 5:01 AM, Yangchao Zhou wrote: > rx_q fifo can only be released by kernel thread. There may be > mbuf leaks in rx_q because kernel threads are randomly stopped. > > When the kni is released and netdev is unregisterd, convert the > physical address mbufs in rx_q to the virtual address in free_q. > By the way, alloc_q can be processed together to speed up the > release rate in userspace. Overall looks good to me. A few comments below. Do you observe a speed up in userspace related alloc_q free? > > Signed-off-by: Yangchao Zhou > Suggested-by: Ferruh Yigit > --- > kernel/linux/kni/kni_dev.h | 1 + > kernel/linux/kni/kni_misc.c | 2 ++ > kernel/linux/kni/kni_net.c | 40 ++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 43 insertions(+), 0 deletions(-) > > diff --git a/kernel/linux/kni/kni_dev.h b/kernel/linux/kni/kni_dev.h > index c9393d8..6275ef2 100644 > --- a/kernel/linux/kni/kni_dev.h > +++ b/kernel/linux/kni/kni_dev.h > @@ -92,6 +92,7 @@ struct kni_dev { > void *alloc_va[MBUF_BURST_SZ]; > }; > > +void kni_net_release_fifo_phy(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/kernel/linux/kni/kni_misc.c b/kernel/linux/kni/kni_misc.c > index 01574ec..fa69f8e 100644 > --- a/kernel/linux/kni/kni_misc.c > +++ b/kernel/linux/kni/kni_misc.c > @@ -192,6 +192,8 @@ struct kni_net { > free_netdev(dev->net_dev); > } > > + kni_net_release_fifo_phy(dev); > + > return 0; > } > > diff --git a/kernel/linux/kni/kni_net.c b/kernel/linux/kni/kni_net.c > index 9f9b798..1d64d78 100644 > --- a/kernel/linux/kni/kni_net.c > +++ b/kernel/linux/kni/kni_net.c > @@ -163,6 +163,46 @@ > return (ret == 0) ? req.result : ret; > } > > +static void > +kni_fifo_trans_pa2va(struct kni_dev *kni, > + struct rte_kni_fifo *src_pa, struct rte_kni_fifo *dst_va) > +{ > + uint32_t ret, i, num_fq, num_rx; > + void *kva; > + do { > + num_fq = kni_fifo_free_count(kni->free_q); Since this function made more generic with src, dst arguments, it is better to use dst_va here instead of kni->free_q > + if (num_fq == 0) And this variable name can be updated from free queue to dst. > + return; > + > + num_rx = min_t(uint32_t, num_fq, MBUF_BURST_SZ); > + > + num_rx = kni_fifo_get(src_pa, kni->pa, num_rx); > + if (num_rx == 0) > + return; > + > + for (i = 0; i < num_rx; i++) { > + kva = pa2kva(kni->pa[i]); > + kni->va[i] = pa2va(kni->pa[i], kva); > + } > + > + ret = kni_fifo_put(dst_va, kni->va, num_rx); > + if (ret != num_rx) { > + /* Failing should not happen */ > + pr_err("Fail to enqueue entries into dst_va\n"); > + return; > + } > + } while (1); > +} > + > +/* Try to release mbufs when kni release */ > +void kni_net_release_fifo_phy(struct kni_dev *kni) > +{ > + /* release rx_q first, because it can't release in userspace */ > + kni_fifo_trans_pa2va(kni, kni->rx_q, kni->free_q); > + /* release alloc_q for speeding up kni release in userspace */ > + kni_fifo_trans_pa2va(kni, kni->alloc_q, kni->free_q); > +} > + > /* > * Configuration changes (passed on by ifconfig) > */ >