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 <jianfeng.tan at intel.com> >> --- >> 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