Re: [dpdk-dev] [PATCH] vhost: disable indirect descriptors feature

[Maxime Coquelin <maxime.coquelin@redhat.com>]

On 10/18/2016 10:12 AM, Yuanhan Liu wrote:
> On Tue, Oct 18, 2016 at 09:04:44AM +0200, Maxime Coquelin wrote:
>> Hi Yuanhan,
>>
>> On 10/17/2016 05:10 PM, Maxime Coquelin wrote:
>>> Commit 2304dd73d287 ("vhost: support indirect Tx descriptors")
>>> adds support for indirect descriptors for Tx, but not for Rx.
>>>
>>> The problem is that it does not work with windows guests, which
>>> uses indirect descriptors for the Rx, and also with Linux guests
>>> when using kernel driver with mergeable buffers feature disabled.
>>>
>>> While indirect descriptors support is also added to the Rx path,
>>> let's disable the feature.
>>>
>>> Reported-by: Zhihong Wang <zhihong.wang@intel.com>
>>> Reported-by: Ciara Loftus <ciara.loftus@intel.com>
>>> Cc: Yuanhan Liu <yuanhan.liu@linux.intel.com>
>>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>>> ---
>>> lib/librte_vhost/vhost.c | 3 +--
>>> 1 file changed, 1 insertion(+), 2 deletions(-)
>>>
>>> diff --git a/lib/librte_vhost/vhost.c b/lib/librte_vhost/vhost.c
>>> index 469117a..f5f8f92 100644
>>> --- a/lib/librte_vhost/vhost.c
>>> +++ b/lib/librte_vhost/vhost.c
>>> @@ -65,8 +65,7 @@
>>> 				(1ULL << VIRTIO_NET_F_CSUM)    | \
>>> 				(1ULL << VIRTIO_NET_F_GUEST_CSUM) | \
>>> 				(1ULL << VIRTIO_NET_F_GUEST_TSO4) | \
>>> -				(1ULL << VIRTIO_NET_F_GUEST_TSO6) | \
>>> -				(1ULL << VIRTIO_RING_F_INDIRECT_DESC))
>>> +				(1ULL << VIRTIO_NET_F_GUEST_TSO6))
>>>
>>> uint64_t VHOST_FEATURES = VHOST_SUPPORTED_FEATURES;
>>
>> I have implemented Indirect descs for the Rx path yesterday.
>> It deserves more testing, but early tests show it fix the issues found
>> with VIRTIO_RING_F_INDIRECT_DESC (both with and without mergeable
>> buffers).
>>
>>
>> Thanks to Zhihong series you reworked, the changes to be done for
>> mergeable buffers case is greatly simplified.
>> I'll send the series later today.
>
> Do you mean the v6 from Zhihong? Unluckily, it will not be merged. That
> series has been simplified to not rewrite the enqueue from scratch. See
> V7.
No, I meant the v7.

>
> For this patch, I think you should also update (or remove?) the related
> section in the release note.
Yes, sure.

Thanks,
Maxime