From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) by dpdk.org (Postfix) with ESMTP id F1F3337B2 for ; Thu, 28 Apr 2016 00:35:22 +0200 (CEST) Received: from orsmga002.jf.intel.com ([10.7.209.21]) by orsmga103.jf.intel.com with ESMTP; 27 Apr 2016 15:35:21 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.24,543,1455004800"; d="scan'208";a="964176696" Received: from yliu-dev.sh.intel.com (HELO yliu-dev) ([10.239.67.162]) by orsmga002.jf.intel.com with ESMTP; 27 Apr 2016 15:35:20 -0700 Date: Wed, 27 Apr 2016 15:37:53 -0700 From: Yuanhan Liu To: Jianfeng Tan Cc: dev@dpdk.org, huawei.xie@intel.com Message-ID: <20160427223753.GB25677@yliu-dev.sh.intel.com> References: <1461673932-128879-1-git-send-email-jianfeng.tan@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1461673932-128879-1-git-send-email-jianfeng.tan@intel.com> User-Agent: Mutt/1.5.23 (2014-03-12) Subject: Re: [dpdk-dev] [PATCH] virtio: fix memory leak of virtqueue memzones X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 27 Apr 2016 22:35:23 -0000 On Tue, Apr 26, 2016 at 12:32:12PM +0000, Jianfeng Tan wrote: > Issue: When virtio was proposed in DPDK, there is no API to free memzones. > But this has changed since rte_memzone_free() has been implemented by > commit ff909fe21f. The more proper way to reference a commit is commit_id ("$commit_subject") Like what the fixline does. > This patch is to make sure memzones in struct virtqueue, like mz and > virtio_net_hdr_mz, are freed when queue is released or setup fails. > > Signed-off-by: Jianfeng Tan > --- > drivers/net/virtio/virtio_ethdev.c | 69 ++++++++++++++++++++------------------ > drivers/net/virtio/virtio_ethdev.h | 2 +- > drivers/net/virtio/virtio_rxtx.c | 4 +-- > 3 files changed, 40 insertions(+), 35 deletions(-) > > diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c > index 63a368a..54eacf6 100644 > --- a/drivers/net/virtio/virtio_ethdev.c > +++ b/drivers/net/virtio/virtio_ethdev.c > @@ -261,12 +261,18 @@ virtio_set_multiple_queues(struct rte_eth_dev *dev, uint16_t nb_queues) > } > > void > -virtio_dev_queue_release(struct virtqueue *vq) { > +virtio_dev_queue_release(struct virtqueue *vq, int io_related) > +{ > struct virtio_hw *hw; > > if (vq) { > hw = vq->hw; > - hw->vtpci_ops->del_queue(hw, vq); > + if (io_related) > + hw->vtpci_ops->del_queue(hw, vq); What is "io_related" supposed to mean here, queue has been started/set up? If so, "started" might be better. And remember to put it into the vq struct: we don't need an extra parameter for that. > + > + rte_memzone_free(vq->mz); > + if (vq->virtio_net_hdr_mz) > + rte_memzone_free(vq->virtio_net_hdr_mz); > > rte_free(vq->sw_ring); > rte_free(vq); > @@ -286,6 +292,7 @@ int virtio_dev_queue_setup(struct rte_eth_dev *dev, > unsigned int vq_size, size; > struct virtio_hw *hw = dev->data->dev_private; > struct virtqueue *vq = NULL; > + const char *queue_names[] = {"rvq", "txq", "cvq"}; > > PMD_INIT_LOG(DEBUG, "setting up queue: %u", vtpci_queue_idx); > > @@ -305,34 +312,34 @@ int virtio_dev_queue_setup(struct rte_eth_dev *dev, > return -EINVAL; > } > > - if (queue_type == VTNET_RQ) { > - snprintf(vq_name, sizeof(vq_name), "port%d_rvq%d", > - dev->data->port_id, queue_idx); > - vq = rte_zmalloc(vq_name, sizeof(struct virtqueue) + > - vq_size * sizeof(struct vq_desc_extra), RTE_CACHE_LINE_SIZE); > - vq->sw_ring = rte_zmalloc_socket("rxq->sw_ring", > - (RTE_PMD_VIRTIO_RX_MAX_BURST + vq_size) * > - sizeof(vq->sw_ring[0]), RTE_CACHE_LINE_SIZE, socket_id); > - } else if (queue_type == VTNET_TQ) { > - snprintf(vq_name, sizeof(vq_name), "port%d_tvq%d", > - dev->data->port_id, queue_idx); > - vq = rte_zmalloc(vq_name, sizeof(struct virtqueue) + > - vq_size * sizeof(struct vq_desc_extra), RTE_CACHE_LINE_SIZE); > - } else if (queue_type == VTNET_CQ) { > - snprintf(vq_name, sizeof(vq_name), "port%d_cvq", > - dev->data->port_id); > - vq = rte_zmalloc(vq_name, sizeof(struct virtqueue) + > - vq_size * sizeof(struct vq_desc_extra), > - RTE_CACHE_LINE_SIZE); > + if (queue_type < VTNET_RQ || queue_type > VTNET_RQ) { > + PMD_INIT_LOG(ERR, "invalid queue type: %d", queue_type); > + return -EINVAL; > } > + > + snprintf(vq_name, sizeof(vq_name), "port%d_%s%d", > + dev->data->port_id, queue_names[queue_type], queue_idx); > + vq = rte_zmalloc(vq_name, sizeof(struct virtqueue) + > + vq_size * sizeof(struct vq_desc_extra), > + RTE_CACHE_LINE_SIZE); This is a cleanup, a good cleanup. So, make a patch for that, and do NOT mix cleanup and fix in one single patch, which is something I have told you quite few times, right? --yliu