Re: [dpdk-dev] [PATCH 3/5] virtio: use indirect ring elements

["Xie, Huawei" <huawei.xie@intel.com>]

On 10/19/2015 11:47 PM, Stephen Hemminger wrote:
> On Mon, 19 Oct 2015 13:19:50 +0000
> "Xie, Huawei" <huawei.xie@intel.com> wrote:
>
>>>  static int
>>> -virtqueue_enqueue_xmit(struct virtqueue *txvq, struct rte_mbuf *cookie)
>>> +virtqueue_enqueue_xmit(struct virtqueue *txvq, struct rte_mbuf *cookie,
>>> +		       int use_indirect)
>>>  {
>>>  	struct vq_desc_extra *dxp;
>>>  	struct vring_desc *start_dp;
>>>  	uint16_t seg_num = cookie->nb_segs;
>>> -	uint16_t needed = 1 + seg_num;
>>> +	uint16_t needed = use_indirect ? 1 : 1 + seg_num;
>>>  	uint16_t head_idx, idx;
>>> -	uint16_t head_size = txvq->hw->vtnet_hdr_size;
>>> +	unsigned long offs;
>>>  
>>>  	if (unlikely(txvq->vq_free_cnt == 0))
>>>  		return -ENOSPC;
>>> @@ -220,12 +221,29 @@ virtqueue_enqueue_xmit(struct virtqueue *txvq, struct rte_mbuf *cookie)
>>>  	dxp = &txvq->vq_descx[idx];
>>>  	dxp->cookie = (void *)cookie;
>>>  	dxp->ndescs = needed;
>>> -
>>>  	start_dp = txvq->vq_ring.desc;
>>> -	start_dp[idx].addr =
>>> -		txvq->virtio_net_hdr_mem + idx * head_size;
>>> -	start_dp[idx].len = (uint32_t)head_size;
>>> -	start_dp[idx].flags = VRING_DESC_F_NEXT;
>>> +
>>> +	if (use_indirect) {
>>> +		struct virtio_tx_region *txr
>>> +			= txvq->virtio_net_hdr_mz->addr;
>>> +
>>> +		offs = idx * sizeof(struct virtio_tx_region)
>>> +			+ offsetof(struct virtio_tx_region, tx_indir);
>>> +
>>> +		start_dp[idx].addr  = txvq->virtio_net_hdr_mem + offs;
>>> +		start_dp[idx].len   = (seg_num + 1) * sizeof(struct vring_desc);  
>> a. In indirect mode, as we always use one descriptor, could we allocate
>> one fixed descriptor as what i did in RX/TX ring layout optimization? :).
> The code can not assume all packets will be in indirect mode. If using
> any_layout, then some packets will use that. Also if you give a packet
> where nb_segs is very large, then it falls back to old mode.
> Also some hosts (like vhost) don't support indirect.
Agree.
With always one descriptor, it is ok.
For the packets with more than VIRTIO_MAX_INDIRECT segs, we fall to old
mode.
>
>> b. If not a, we could cache the descriptor, avoid update unless the
>> fields are different. In current implementation of free desc list, we
>> could make them always use the same tx desc for the same ring slot. I am
>> to submit a patch for normal rx path.
> See above
I don't mean using fixed descriptors. Even in general implementation,
the desc allocated for the ring entry would be normally the same.
So we cache the descriptor id(in the case only one desc is used) last
allocated for each avail ring entry , compare, if the same, skip the store.
This introduces some overhead, but considering vhost has to fetch this
L1M cache line from virtio's processing core, it might be worth.
Mst has posted a patch for virtio's testcase.
Maybe it makes more sense for RX as currently we always uses one descriptor.
If you mean the "only one descriptor" again, skip this comment.
>
>> c. Could we initialize the length of all tx descriptors to be
>> VIRTIO_MAX_INDIRECT * sizeof(struct vring_desc)? Is maximum length ok
>> here? Does the spec require that the length field reflects the length of
>> real used descs, as we already have the next field to indicate the last
>> descriptor.
> The largest VIRTIO_MAX_INDIRECT possible is very large 4K
instead of

start_dp[idx].len   = (seg_num + 1) * sizeof(struct vring_desc)
Is it ok to use
start_dp[idx].len   = 4096 * sizeof(struct vring_desc)?

>
>
>>> +		start_dp[idx].flags = VRING_DESC_F_INDIRECT;
>>> +
>>> +		start_dp = txr[idx].tx_indir;
>>> +		idx = 0;
>>> +	} else {
>>> +		offs = idx * sizeof(struct virtio_tx_region)
>>> +			+ offsetof(struct virtio_tx_region, tx_hdr);
>>> +
>>> +		start_dp[idx].addr  = txvq->virtio_net_hdr_mem + offs;
>>> +		start_dp[idx].len   = txvq->hw->vtnet_hdr_size;
>>> +		start_dp[idx].flags = VRING_DESC_F_NEXT;
>>> +	}
>>>  
>>>  	for (; ((seg_num > 0) && (cookie != NULL)); seg_num--) {
>>>  		idx = start_dp[idx].next;  
>> This looks weird to me. Why skip the first user provided descriptor?
>> idx = 0
>> idx = start_dp[idx].next
>> start_dp[idx].addr = ...
> The first descriptor (0) is initialized once to point to the static
> all zeros tx header. Then code skips to second entry to initailize the
> first data block.
Ah, forget the header.
>>> @@ -236,7 +254,12 @@ virtqueue_enqueue_xmit(struct virtqueue *txvq, struct rte_mbuf *cookie)
>>>  	}
>>>  
>>>  	start_dp[idx].flags &= ~VRING_DESC_F_NEXT;
>>> -	idx = start_dp[idx].next;
>>> +
>>> +	if (use_indirect)
>>> +		idx = txvq->vq_ring.desc[head_idx].next;
>>> +	else
>>> +		idx = start_dp[idx].next;
>>> +
>>>  	txvq->vq_desc_head_idx = idx;
>>>  	if (txvq->vq_desc_head_idx == VQ_RING_DESC_CHAIN_END)
>>>  		txvq->vq_desc_tail_idx = idx;
>>> @@ -261,7 +284,7 @@ static void
>>>  virtio_dev_vring_start(struct virtqueue *vq, int queue_type)
>>>  {
>>>  	struct rte_mbuf *m;
>>> -	int i, nbufs, error, size = vq->vq_nentries;
>>> +	int nbufs, error, size = vq->vq_nentries;
>>>  	struct vring *vr = &vq->vq_ring;
>>>  	uint8_t *ring_mem = vq->vq_ring_virt_mem;
>>>  
>>> @@ -279,10 +302,7 @@ virtio_dev_vring_start(struct virtqueue *vq, int queue_type)
>>>  	vq->vq_free_cnt = vq->vq_nentries;
>>>  	memset(vq->vq_descx, 0, sizeof(struct vq_desc_extra) * vq->vq_nentries);
>>>  
>>> -	/* Chain all the descriptors in the ring with an END */
>>> -	for (i = 0; i < size - 1; i++)
>>> -		vr->desc[i].next = (uint16_t)(i + 1);
>>> -	vr->desc[i].next = VQ_RING_DESC_CHAIN_END;
>>> +	vring_desc_init(vr->desc, size);
>>>  
>>>  	/*
>>>  	 * Disable device(host) interrupting guest
>>> @@ -760,7 +780,15 @@ virtio_xmit_pkts(void *tx_queue, struct rte_mbuf **tx_pkts, uint16_t nb_pkts)
>>>  
>>>  	for (nb_tx = 0; nb_tx < nb_pkts; nb_tx++) {
>>>  		struct rte_mbuf *txm = tx_pkts[nb_tx];
>>> -		int need = txm->nb_segs - txvq->vq_free_cnt + 1;
>>> +		int use_indirect, slots, need;
>>> +
>>> +		use_indirect = vtpci_with_feature(txvq->hw,
>>> +						  VIRTIO_RING_F_INDIRECT_DESC)
>>> +			&& (txm->nb_segs < VIRTIO_MAX_TX_INDIRECT);
>>> +
>>> +		/* How many ring entries are needed to this Tx? */  
>> "how many descs" is more accurate , at least to me, ring entries/slots
>> means entries/slots in avail ring.
>> If it is OK, s/slots/descs/ as well.
> The virtio spec doesn't use the words descriptors. that is more an Intel
> driver terminolgy.
>
>>> +		slots = use_indirect ? 1 : 1 + txm->nb_segs;
>>> +		need = slots - txvq->vq_free_cnt;
>>>  
>>>  		/* Positive value indicates it need free vring descriptors */
>>>  		if (need > 0) {
>>> @@ -769,7 +797,7 @@ virtio_xmit_pkts(void *tx_queue, struct rte_mbuf **tx_pkts, uint16_t nb_pkts)
>>>  			need = RTE_MIN(need, (int)nb_used);
>>>  
>>>  			virtio_xmit_cleanup(txvq, need);
>>> -			need = txm->nb_segs - txvq->vq_free_cnt + 1;
>>> +			need = slots - txvq->vq_free_cnt;
>>>  			if (unlikely(need > 0)) {
>>>  				PMD_TX_LOG(ERR,
>>>  					   "No free tx descriptors to transmit");
>>> @@ -787,7 +815,7 @@ virtio_xmit_pkts(void *tx_queue, struct rte_mbuf **tx_pkts, uint16_t nb_pkts)
>>>  		}
>>>  
>>>  		/* Enqueue Packet buffers */
>>> -		error = virtqueue_enqueue_xmit(txvq, txm);
>>> +		error = virtqueue_enqueue_xmit(txvq, txm, use_indirect);
>>>  		if (unlikely(error)) {
>>>  			if (error == ENOSPC)
>>>  				PMD_TX_LOG(ERR, "virtqueue_enqueue Free count = 0");
>>> diff --git a/drivers/net/virtio/virtqueue.h b/drivers/net/virtio/virtqueue.h
>>> index 7789411..fe3fa66 100644
>>> --- a/drivers/net/virtio/virtqueue.h
>>> +++ b/drivers/net/virtio/virtqueue.h
>>> @@ -237,6 +237,25 @@ struct virtio_net_hdr_mrg_rxbuf {
>>>  	uint16_t num_buffers; /**< Number of merged rx buffers */
>>>  };
>>>  
>>> +/* Region reserved to allow for transmit header and indirect ring */
>>> +#define VIRTIO_MAX_TX_INDIRECT 8
>>> +struct virtio_tx_region {
>>> +	struct virtio_net_hdr_mrg_rxbuf tx_hdr;  
>> Any reason to use merge-able header here?
>>> +	struct vring_desc tx_indir[VIRTIO_MAX_TX_INDIRECT]
>>> +			   __attribute__((__aligned__(16)));  
>> WARNING: __aligned(size) is preferred over __attribute__((aligned(size)))
>> [...]
>