Re: [dpdk-dev] [PATCH] virtio: split virtio rx/tx queue

[Yuanhan Liu <yuanhan.liu@linux.intel.com>]

On Tue, May 24, 2016 at 09:38:32PM +0800, Huawei Xie wrote:
>  	vq->vq_ring_mem = mz->phys_addr;
>  	vq->vq_ring_virt_mem = mz->addr;
> -	PMD_INIT_LOG(DEBUG, "vq->vq_ring_mem:      0x%"PRIx64, (uint64_t)mz->phys_addr);
> -	PMD_INIT_LOG(DEBUG, "vq->vq_ring_virt_mem: 0x%"PRIx64, (uint64_t)(uintptr_t)mz->addr);
> -	vq->virtio_net_hdr_mz  = NULL;
> -	vq->virtio_net_hdr_mem = 0;
> +	PMD_INIT_LOG(DEBUG, "vq->vq_ring_mem:      0x%"PRIx64,
> +		     (uint64_t)mz->phys_addr);
> +	PMD_INIT_LOG(DEBUG, "vq->vq_ring_virt_mem: 0x%"PRIx64,
> +		     (uint64_t)(uintptr_t)mz->addr);
> +
> +	hdr_mz = rte_memzone_reserve_aligned(vq_hdr_name, sz_hdr_mz, socket_id,
> +					     0, RTE_CACHE_LINE_SIZE);

We don't need allocate hdr_mz for Rx queue, and in such case, sz_hdr_mz
is 0. I'm wondering what hdr_mz would be then, NULL?

Anyway, you should skip the hdr_mz allocation for Rx queue, and I also
would suggest you to move the vq_hdr_name setup here.

> +	if (hdr_mz == NULL) {
> +		if (rte_errno == EEXIST)
> +			hdr_mz = rte_memzone_lookup(vq_hdr_name);
> +		if (hdr_mz == NULL) {
> +			ret = -ENOMEM;
> +			goto fail_q_alloc;
> +		}
> +	}
>  
...
>  
>  	PMD_INIT_FUNC_TRACE();
>  	ret = virtio_dev_queue_setup(dev, VTNET_CQ, VTNET_SQ_CQ_QUEUE_IDX,
> -			vtpci_queue_idx, 0, socket_id, &vq);
> +			vtpci_queue_idx, 0, socket_id, (void **)&cvq);

Unnecessary cast. Note that there are few others like that in this
patch.


> -		PMD_RX_LOG(DEBUG, "dequeue:%d", num);
> -		PMD_RX_LOG(DEBUG, "packet len:%d", len[0]);
> +		PMD_RX_LOG(DEBUG, "dequeue:%d\n", num);
> +		PMD_RX_LOG(DEBUG, "packet len:%d\n", len[0]);

We should not append "\n" for PMD_RX_LOG; this macro alreadys does it.

Another note is that you might want to run checkpatch; I saw quite many
warnings.

	--yliu