[dpdk-dev] [PATCH v2 1/7] vhost: refactor rte_vhost_dequeue_burst

[Xie, Huawei]

On 3/7/2016 10:47 AM, Yuanhan Liu wrote:
> On Mon, Mar 07, 2016 at 02:32:46AM +0000, Xie, Huawei wrote:
>> On 3/4/2016 10:15 AM, Yuanhan Liu wrote:
>>> On Thu, Mar 03, 2016 at 04:30:42PM +0000, Xie, Huawei wrote:
>>>> On 2/18/2016 9:48 PM, Yuanhan Liu wrote:
>>>>> +	mbuf_avail  = 0;
>>>>> +	mbuf_offset = 0;
>>>>> +	while (desc_avail || (desc->flags & VRING_DESC_F_NEXT) != 0) {
>>>>> +		/* This desc reachs to its end, get the next one */
>>>>> +		if (desc_avail == 0) {
>>>>> +			desc = &vq->desc[desc->next];
>>>>> +
>>>>> +			desc_addr = gpa_to_vva(dev, desc->addr);
>>>>> +			rte_prefetch0((void *)(uintptr_t)desc_addr);
>>>>> +
>>>>> +			desc_offset = 0;
>>>>> +			desc_avail  = desc->len;
>>>>> +
>>>>> +			PRINT_PACKET(dev, (uintptr_t)desc_addr, desc->len, 0);
>>>>> +		}
>>>>> +
>>>>> +		/*
>>>>> +		 * This mbuf reachs to its end, get a new one
>>>>> +		 * to hold more data.
>>>>> +		 */
>>>>> +		if (mbuf_avail == 0) {
>>>>> +			cur = rte_pktmbuf_alloc(mbuf_pool);
>>>>> +			if (unlikely(!cur)) {
>>>>> +				RTE_LOG(ERR, VHOST_DATA, "Failed to "
>>>>> +					"allocate memory for mbuf.\n");
>>>>> +				if (head)
>>>>> +					rte_pktmbuf_free(head);
>>>>> +				return NULL;
>>>>> +			}
>>>> We could always allocate the head mbuf before the loop, then we save the
>>>> following branch and make the code more streamlined.
>>>> It reminds me that this change prevents the possibility of mbuf bulk
>>>> allocation, one solution is we pass the head mbuf from an additional
>>>> parameter.
>>> Yep, that's also something I have thought of.
>>>
>>>> Btw, put unlikely before the check of mbuf_avail and checks elsewhere.
>>> I don't think so. It would benifit for the small packets. What if,
>>> however, when TSO or jumbo frame is enabled that we have big packets?
>> Prefer to favor the path that packet could fit in one mbuf.
> Hmmm, why? While I know that TSO and mergeable buf is disable by default
> in our vhost example vhost-switch, they are enabled by default in real
> life.

mergable is only meaningful in RX path.
If you mean big packets in TX path, i personally favor the path that
packet fits in one mbuf.


>> Btw, not specially to this, return "m = copy_desc_to_mbuf(dev, vq,
>> desc_indexes[i], mbuf_pool)", failure case is unlikely to happen, so add
>> unlikely for the check if (m == NULL) there. Please check all branches
>> elsewhere.
> Thanks for the remind, will have a detail check.
>
> 	--yliu
>