Re: [dpdk-dev] [PATCH v3 07/21] net/virtio: implement transmit path for packed queues

[Jens Freimann <jfreimann@redhat.com>]

On Sun, Apr 08, 2018 at 12:51:32PM +0800, Tiwei Bie wrote:
>On Thu, Apr 05, 2018 at 12:10:17PM +0200, Jens Freimann wrote:
>[...]
>> diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c
>> index a8aa87b32..9f9b5a8f8 100644
>> --- a/drivers/net/virtio/virtio_rxtx.c
>> +++ b/drivers/net/virtio/virtio_rxtx.c
>> @@ -38,6 +38,101 @@
>>  #define  VIRTIO_DUMP_PACKET(m, len) do { } while (0)
>>  #endif
>>
>> +#define VIRTIO_SIMPLE_FLAGS ((uint32_t)ETH_TXQ_FLAGS_NOMULTSEGS | \
>> +	ETH_TXQ_FLAGS_NOOFFLOADS)
>
>Why add this?

mistake during rebase, will fix.
>
>> +
>> +/* Cleanup from completed transmits. */
>> +static void
>> +virtio_xmit_cleanup_packed(struct virtqueue *vq)
>> +{
>> +	uint16_t idx;
>> +	uint16_t size = vq->vq_nentries;
>> +	struct vring_desc_packed *desc = vq->vq_ring.desc_packed;
>> +
>> +	idx = vq->vq_used_cons_idx & (size - 1);
>> +	while (desc_is_used(&desc[idx]) &&
>> +	       vq->vq_free_cnt < size) {
>> +		vq->vq_free_cnt++;
>> +		idx = ++vq->vq_used_cons_idx & (size - 1);
>
>Driver needs to keep track of the number of descriptors
>to be skipped for each used descriptor.

I've added support for this due already (because of your comment on
the rxvq flush). Will be in v4.

>> +	}
>> +}
>> +
>> +uint16_t
>> +virtio_xmit_pkts_packed(void *tx_queue, struct rte_mbuf **tx_pkts,
>> +		     uint16_t nb_pkts)
>> +{
>[...]
>> +}
>>  int
>>  virtio_dev_rx_queue_done(void *rxq, uint16_t offset)
>>  {
>
>Please add an empty line between the functions.

ok.

>
>
>> @@ -547,6 +642,10 @@ virtio_dev_tx_queue_setup_finish(struct rte_eth_dev *dev,
>>
>>  	PMD_INIT_FUNC_TRACE();
>>
>> +	if (vtpci_packed_queue(hw)) {
>> +		vq->vq_ring.avail_wrap_counter = 1;
>
>virtio_dev_tx_queue_setup_finish() will be called during a
>dev_stop()/dev_start(). The problem is that, the dev_stop()
>doesn't really stop the device. So we can't reset the wrap
>counter to 1 in dev_start().

I see, would virtio_init_device() work? 

thanks!

regards,
Jens 
>
>> +	}
>> +
>>  	if (hw->use_simple_tx) {
>>  		for (desc_idx = 0; desc_idx < mid_idx; desc_idx++) {
>>  			vq->vq_ring.avail->ring[desc_idx] =
>> @@ -567,7 +666,8 @@ virtio_dev_tx_queue_setup_finish(struct rte_eth_dev *dev,
>>  			vq->vq_ring.avail->ring[desc_idx] = desc_idx;
>>  	}
>>
>> -	VIRTQUEUE_DUMP(vq);
>> +	if (!vtpci_packed_queue(hw))
>
>The check isn't needed.
>
>
>> +		VIRTQUEUE_DUMP(vq);
>>
>>  	return 0;
>>  }
>> --
>> 2.14.3
>>