Re: [PATCH v3 5/5] vhost-user: add skip_drain param to GET_VRING_BASE

[Raphael Norwitz <rnorwitz@nvidia.com>]

On 5/26/26 2:48 PM, Stefan Hajnoczi wrote:
> On Mon, May 25, 2026 at 05:34:37PM -0400, Raphael Norwitz wrote:
>> Apologies for the late reply.
>>
>> On 5/12/26 1:55 AM, Alexandr Moshkov wrote:
>>> Gentle ping :)
>>>
>>
>> [...]
>>
>>>>>>> On 4/16/26 5:26 AM, Alexandr Moshkov wrote:
>>>>>>>> Greetings! Thanks for reply!
>>>>>>>>
>>>>>>>>
>>>>>>>> On 4/15/26 20:21, Raphael Norwitz wrote:
>>>>>>>>> I’m not sure I like using the num field in
>>>>>>>>> vhost_vring_state to set magic values which
>>>>>>>>> affect protocol behavior for GET_VRING_BASE. It
>>>>>>>>> feels like a hack to me. I would think the
>>>>>>>>> proper solution if we want to support migration
>>>>>>>>> from new to old would be to have new use a
>>>>>>>>> different new message entirely. Can we do that?
>>>>>>>>
>>>>>>>> I think we can, but I thought at first that this
>>>>>>>> will be almost a complete copy of GET_VRING_BASE
>>>>>>>> message, with the exception of waiting for drain of
>>>>>>>> requests, so I choose to expand existing message.
>>>>>>>>
>>>>>>>
>>>>>>> I think a new message would be cleaner. Anyone else have thoughts?
>>>>>>>
>>>>>>>> If this is not an appropriate approach, is it better
>>>>>>>> to make a new message like GET_VRING_BASE or a
>>>>>>>> separate message used together with default
>>>>>>>> GET_VRING_BASE message (for example, message for
>>>>>>>> setting some kind of status on the server)? What
>>>>>>>> should I name this new message?
>>>>>>>>
>>>>>>>
>>>>>>> Maybe GET_VRING_BASE_SKIP_DRAIN? How would a separate
>>>>>>> message used together with default GET_VRING_BASE
>>>>>>> message work?
>>>>>>
>>>>>> I was thinking about a message something like
>>>>>> SET_VRING_ENABLE - for example SET_SKIP_DRAIN_ENABLE, that
>>>>>> would enable/disable some state (skip_drain) in the backend.
>>>>>
>>>>> I'd need to see the flow in more detail but sounds promising.
>>>>
>>>> On migration vhost-user-blk firstly send SET_SKIP_DRAIN_ENABLE with
>>>> num = 1 message if `inflight-migration` device parameter
>>>> and VHOST_USER_PROTOCOL_F_GET_VRING_BASE_INFLIGHT enabled. Then send
>>>> GET_VRING_BASE and continue work as usual.
>>>>
>>>> On SET_SKIP_DRAIN_ENABLE backend sets inner state (skip_drain) so
>>>> when GET_VRING_BASE is called and skip_drain is true the drain will
>>>> skipped.
>>>>
>>>> What do you think?
>>>>
>>
>> I'm happy with that in theory but would like more clarity on the details. In
>> particular, I'm not sure what you mean by "vhost-user-blk firstly send
>> SET_SKIP_DRAIN_ENABLE". To add a new message I would think we would have to
>> do a vhost-user protocol feature negotiation to gate it.
>>
>> Also what are the precise semantics for SET_SKIP_DRAIN_ENABLE? Would it be
>> sent on backend connect or when we're about to migrate?
>>
>> I was hoping others would comment but at this point I'd suggest drafting the
>> code and then we can re-review.
> 
> Adding a new GET_VRING_BASE_SKIP_DRAIN message seems simpler to me than
> a stateful SET_SKIP_DRAIN_ENABLE where the back-end needs to stash the
> enable_skip_drain state and the front-end would have to toggle the state
> if it switches between skip drain and classic behavior.

Agreed - that sounds cleaner.


> 
> Stefan