Re: [Qemu-devel] [virtio-dev] Re: [virtio-dev] Re: [virtio-dev] Re: [PATCH v1] virtio-net: enable configurable tx queue size

[Wei Wang <wei.w.wang@intel.com>]

On 06/15/2017 12:16 PM, Jason Wang wrote:
>
>
> On 2017年06月14日 23:22, Michael S. Tsirkin wrote:
>> On Wed, Jun 14, 2017 at 07:26:54PM +0800, Jason Wang wrote:
>>>
>>> On 2017年06月13日 18:46, Jason Wang wrote:
>>>>
>>>> On 2017年06月13日 17:50, Wei Wang wrote:
>>>>> On 06/13/2017 05:04 PM, Jason Wang wrote:
>>>>>>
>>>>>> On 2017年06月13日 15:17, Wei Wang wrote:
>>>>>>> On 06/13/2017 02:29 PM, Jason Wang wrote:
>>>>>>>> The issue is what if there's a mismatch of max #sgs between 
>>>>>>>> qemu and
>>>>>>>>>>> When the vhost backend is used, QEMU is not
>>>>>>>>>>> involved in the data path.
>>>>>>>>>>> The vhost backend
>>>>>>>>>>> directly gets what is offered by the guest from the vq. Why 
>>>>>>>>>>> would
>>>>>>>>>>> there be a mismatch of
>>>>>>>>>>> max #sgs between QEMU and vhost, and what is
>>>>>>>>>>> the QEMU side max #sgs
>>>>>>>>>>> used for? Thanks.
>>>>>>>>>> You need query the backend max #sgs in this case
>>>>>>>>>> at least. no? If not
>>>>>>>>>> how do you know the value is supported by the backend?
>>>>>>>>>>
>>>>>>>>>> Thanks
>>>>>>>>>>
>>>>>>>>> Here is my thought: vhost backend has already been
>>>>>>>>> supporting 1024 sgs,
>>>>>>>>> so I think it might not be necessary to query the
>>>>>>>>> max sgs that the vhost
>>>>>>>>> backend supports. In the setup phase, when QEMU
>>>>>>>>> detects the backend is
>>>>>>>>> vhost, it assumes 1024 max sgs is supported, instead
>>>>>>>>> of giving an extra
>>>>>>>>> call to query.
>>>>>>>> We can probably assume vhost kernel supports up to 1024
>>>>>>>> sgs. But how about for other vhost-user backends?
>>>>>>>>
>>>>>>> So far, I haven't seen any vhost backend implementation
>>>>>>> supporting less than 1024 sgs.
>>>>>> Since vhost-user is an open protocol we can not check each
>>>>>> implementation (some may be even close sourced). For safety, we
>>>>>> need an explicit clarification on this.
>>>>>>
>>>>>>>
>>>>>>>> And what you said here makes me ask one of my questions in the 
>>>>>>>> past:
>>>>>>>>
>>>>>>>> Do we have plan to extend 1024 to a larger value or 1024
>>>>>>>> looks good for the future years? If we only care about
>>>>>>>> 1024, there's even no need for a new config filed, a
>>>>>>>> feature flag is more than enough. If we want to extend
>>>>>>>> it to e.g 2048, we definitely need to query vhost
>>>>>>>> backend's limit (even for vhost-kernel).
>>>>>>>>
>>>>>>> According to virtio spec (e.g. 2.4.4), unreasonably large
>>>>>>> descriptors are
>>>>>>> not encouraged to be used by the guest. If possible, I would
>>>>>>> suggest to use
>>>>>>> 1024 as the largest number of descriptors that the guest can
>>>>>>> chain, even when
>>>>>>> we have larger queue size in the future. That is,
>>>>>>> if (backend == QEMU backend)
>>>>>>>      config.max_chain_size = 1023 (defined by the qemu
>>>>>>> backend implementation);
>>>>>>> else if (backend == vhost)
>>>>>>>      config.max_chain_size = 1024;
>>>>>>>
>>>>>>> It is transparent to the guest. From the guest's point of
>>>>>>> view, all it knows is a value
>>>>>>> given to him via reading config.max_chain_size.
>>>>>> So not transparent actually, guest at least guest need to see
>>>>>> and check for this. So the question still, since you only care
>>>>>> about two cases in fact:
>>>>>>
>>>>>> - backend supports 1024
>>>>>> - backend supports <1024 (qemu or whatever other backends)
>>>>>>
>>>>>> So it looks like a new feature flag is more than enough. If
>>>>>> device(backends) support this feature, it can make sure 1024 sgs
>>>>>> is supported?
>>>>>>
>>>>> That wouldn't be enough. For example, QEMU3.0 backend supports
>>>>> max_chain_size=1023,
>>>>> while QEMU4.0 backend supports max_chain_size=1021. How would the
>>>>> guest know
>>>>> the max size with the same feature flag? Would it still chain 1023
>>>>> descriptors with QEMU4.0?
>>>>>
>>>>> Best,
>>>>> Wei
>>>> I believe we won't go back to less than 1024 in the future. It may be
>>>> worth to add a unittest for this to catch regression early.
>>>>
>>>> Thanks
>> I think I disagree with that. Smaller pipes a better (e.g. less cache
>> pressure) and you only need huge pipes because host thread gets
>> scheduled out for too long. With more CPUs there's less of a chance of
>> an overcommit so we'll be able to get by with smaller pipes in the
>> future.
>
> Agree, but we are talking about the upper limit. Even if 1024 is 
> supported, small number of #sgs is still encouraged.
>
>>
>>> Consider the queue size is 256 now, I think maybe we can first make 
>>> tx queue
>>> size configurable up to 1024, and then do the #sg stuffs on top.
>>>
>>> What's your opinion, Michael?
>>>
>>> Thanks
>> With a kernel backend, 1024 is problematic since we are then unable
>> to add any entries or handle cases where an entry crosses an MR region
>> boundary. We could support up to 512 with a kernel backend but no one
>> seems to want that :)
>
> Then I see issues with indirect descriptors.
>
> We try to allow up 1024 chained descriptors implicitly since 
> e0e9b406470b ("vhost: max s/g to match qemu"). If guest can submit 
> crossing MR descs, I'm afraid we've already had this bug since this 
> commit. And actually this seems conflict to what spec said in 2.4.5:
>
> """
> The number of descriptors in the table is defined by the queue size 
> for this virtqueue: this is the maximum possible descriptor chain length.
> """
>
> Technically, we had the same issue for rx since we allow 1024 queue 
> size now.
>
> So actually, allowing the size to 1024 does not introduce any new 
> trouble?
>>
>> With vhost-user the backend might be able to handle that. So an
>> acceptable option would be to allow 1K with vhost-user backends
>> only, trim it back with other backends.
>>
>
> I believe the idea is to clarify the maximum chain size instead of 
> having any assumption.
>
>

I think the issues can be solved by VIRTIO_F_MAX_CHAIN_SIZE.

For now, how about splitting it into two series of patches:
1) enable 1024 tx queue size for vhost-user, to let the users of 
vhost-user to easily use 1024 queue size.
2) enable VIRTIO_F_MAX_CHAIN_SIZE,  to enhance robustness.

Best,
Wei