From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by dpdk.org (Postfix) with ESMTP id BB63C2C37 for ; Thu, 28 Apr 2016 04:01:08 +0200 (CEST) Received: from orsmga002.jf.intel.com ([10.7.209.21]) by orsmga101.jf.intel.com with ESMTP; 27 Apr 2016 19:01:07 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.24,545,1455004800"; d="scan'208";a="964286001" Received: from shwdeisgchi083.ccr.corp.intel.com (HELO [10.239.67.193]) ([10.239.67.193]) by orsmga002.jf.intel.com with ESMTP; 27 Apr 2016 19:01:06 -0700 To: Yuanhan Liu References: <1461673932-128879-1-git-send-email-jianfeng.tan@intel.com> <20160427223753.GB25677@yliu-dev.sh.intel.com> Cc: dev@dpdk.org, huawei.xie@intel.com From: "Tan, Jianfeng" Message-ID: Date: Thu, 28 Apr 2016 10:01:06 +0800 User-Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.0 MIME-Version: 1.0 In-Reply-To: <20160427223753.GB25677@yliu-dev.sh.intel.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit 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: Thu, 28 Apr 2016 02:01:10 -0000 Hi Yuanhan, On 4/28/2016 6:37 AM, Yuanhan Liu wrote: > 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. OK. > >> 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. Good suggestion. > >> + >> + 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? You mean submit a specific patch for cleanup or just another commit inside this patch set? Thanks, Jianfeng > > > --yliu