Re: [RFC v3 4/5] KVM: add ioregionfd context

Jason Wang
2021-03-26T06:48:55Z

在 2021/3/17 下午6:46, Elena Afanasova 写道:
> On Thu, 2021-03-11 at 11:04 +0800, Jason Wang wrote:
>> On 2021/3/11 12:41 上午, Elena Afanasova wrote:
>>> On Wed, 2021-03-10 at 15:11 +0100, Paolo Bonzini wrote:
>>>> On 10/03/21 14:20, Elena Afanasova wrote:
>>>>> On Tue, 2021-03-09 at 09:01 +0100, Paolo Bonzini wrote:
>>>>>> On 09/03/21 08:54, Jason Wang wrote:
>>>>>>>> + return;
>>>>>>>> +
>>>>>>>> + spin_lock(&ctx->wq.lock);
>>>>>>>> + wait_event_interruptible_exclusive_locked(ctx->wq,
>>>>>>>> !ctx-
>>>>>>>>> busy);
>>>>>>> Any reason that a simple mutex_lock_interruptible() can't
>>>>>>> work
>>>>>>> here?
>>>>>> Or alternatively why can't the callers just take the
>>>>>> spinlock.
>>>>>>
>>>>> I'm not sure I understand your question. Do you mean why locked
>>>>> version
>>>>> of wait_event() is used?
>>>> No, I mean why do you need to use ctx->busy and wait_event,
>>>> instead
>>>> of
>>>> operating directly on the spinlock or on a mutex.
>>>>
>>> When ioregionfd communication is interrupted by a signal
>>> ioctl(KVM_RUN)
>>> has to return to userspace. I'm not sure it's ok to do that with
>>> the
>>> spinlock/mutex being held.
>>
>> So you don't answer my question. Why you can't use
>> mutex_lock_interruptible() here?
>>
>> It looks can do exactly what you want here.
>>
>> Thanks
>>
> I think mutex could work here. I used it for the first implementation.
> But is it ok to hold a mutex on kernel->user transitions? Is it correct
> pattern in this case?

I may miss something but the semantic is the same for the following of
the two?

A:
spin_lock(&ctx->wq.lock);
wait_event_interruptible_exclusive_locked(ctx->wq, !ctx->busy);
ctx->busy = true;
spin_unlock(&ctx->wq.lock);

B:
mutex_lock_interruptible(&ctx->lock);

> If ioctl returns to userspace and then ioregionfd is deleted or vcpu fd
> is closed, with a mutex held it will be necessary to unlock it.

It's something nature and anything different with your current code that
uses spinlock + waitqueue? And we can know whehter or not we're
interrupt by a singal (I notice you don't check the return value of
wait_event_interruptible_exclusive_locked() and set ctx->busy to true
unconditonally which is probably wrong.

> But I
> think it’s a bit clearer to use wake_up in e.g. kvm_vcpu_release
> instead of mutex_unlock.

I am not sure I undersatnd here (I dont' this how it is doen in this patch).

Thanks

> Paolo, could you please also comment on this?
>
>>