Date:   Tue, 11 Jan 2022 17:19:35 -0500
From:   Tony Krowiak <akrowiak@...ux.ibm.com>
To:     Halil Pasic <pasic@...ux.ibm.com>
Cc:     linux-s390@...r.kernel.org, linux-kernel@...r.kernel.org,
        kvm@...r.kernel.org, jjherne@...ux.ibm.com, freude@...ux.ibm.com,
        borntraeger@...ibm.com, cohuck@...hat.com, mjrosato@...ux.ibm.com,
        alex.williamson@...hat.com, kwankhede@...dia.com,
        fiuczy@...ux.ibm.com
Subject: Re: [PATCH v17 08/15] s390/vfio-ap: keep track of active guests



On 1/11/22 16:58, Tony Krowiak wrote:
>
>
> On 12/29/21 22:33, Halil Pasic wrote:
>> On Thu, 21 Oct 2021 11:23:25 -0400
>> Tony Krowiak <akrowiak@...ux.ibm.com> wrote:
>>
>>> The vfio_ap device driver registers for notification when the 
>>> pointer to
>>> the KVM object for a guest is set. Let's store the KVM pointer as 
>>> well as
>>> the pointer to the mediated device when the KVM pointer is set.
>> [..]
>>
>>
>>> struct ap_matrix_dev {
>>>          ...
>>>          struct rw_semaphore guests_lock;
>>>          struct list_head guests;
>>>         ...
>>> }
>>>
>>> The 'guests_lock' field is a r/w semaphore to control access to the
>>> 'guests' field. The 'guests' field is a list of ap_guest
>>> structures containing the KVM and matrix_mdev pointers for each active
>>> guest. An ap_guest structure will be stored into the list whenever the
>>> vfio_ap device driver is notified that the KVM pointer has been set and
>>> removed when notified that the KVM pointer has been cleared.
>>>
>> Is this about the field or about the list including all the nodes? This
>> reads lie guests_lock only protects the head element, which makes no
>> sense to me. Because of how these lists work.
>
> It locks the list, I can rewrite the description.

Ignore this response and read the answers to your comments below.

>
>
>>
>> The narrowest scope that could make sense is all the list_head stuff
>> in the entire list. I.e. one would only need the lock to traverse or
>> manipulate the list, while the payload would still be subject to
>> the matrix_dev->lock mutex.
>
> The matrix_dev->guests lock is needed whenever the kvm->lock
> is needed because the struct ap_guest object is created and the
> struct kvm assigned to it when the kvm pointer is set
> (vfio_ap_mdev_set_kvm function). So, in order to access the
> ap_guest object and retrieve the kvm pointer, we have to ensure
> the ap_guest_object is still available. The fact we can get the
> kvm pointer from the ap_matrix_mdev object just makes things
> more efficient - i.e., we won't have to traverse the list.
>
> Whenever the kvm->lock and matrix_dev->lock mutexes must
> be held, the order is:
>
>     matrix_dev->guests_lock
>     matrix_dev->guests->kvm->lock
>     matrix_dev->lock
>
> There are times where all three locks are not required; for example,
> the handle_pqap and vfio_ap_mdev_probe/remove functions only
> require the matrix_dev->lock because it does not need to lock kvm.
>
>>
>> [..]
>>
>>> +struct ap_guest {
>>> +    struct kvm *kvm;
>>> +    struct list_head node;
>>> +};
>>> +
>>>   /**
>>>    * struct ap_matrix_dev - Contains the data for the matrix device.
>>>    *
>>> @@ -39,6 +44,9 @@
>>>    *        single ap_matrix_mdev device. It's quite coarse but we 
>>> don't
>>>    *        expect much contention.
>>>    * @vfio_ap_drv: the vfio_ap device driver
>>> + * @guests_lock: r/w semaphore for protecting access to @guests
>>> + * @guests:    list of guests (struct ap_guest) using AP devices 
>>> bound to the
>>> + *        vfio_ap device driver.
>> Please compare the above. Also if it is only about the access to the
>> list, then you could drop the lock right after create, and not keep it
>> till the very end of vfio_ap_mdev_set_kvm(). Right?
>
> That would be true if it only controlled access to the list, but as I
> explained above, that is not its sole purpose.
>
>>
>> In any case I'm skeptical about this whole struct ap_guest business. To
>> me, it looks like something that just makes things more obscure and
>> complicated without any real benefit.
>
> I'm open to other ideas, but you'll have to come up with a way
> to take the kvm->lock before the matrix_mdev->lock in the
> vfio_ap_mdev_probe_queue and vfio_ap_mdev_remove_queue
> functions where we don't have access to the ap_matrix_mdev
> object to which the APQN is assigned and has the pointer to the
> kvm object.
>
> In order to retrieve the matrix_mdev, we need the matrix_dev->lock.
> In order to hot plug/unplug the queue, we need the kvm->lock.
> There's your catch-22 that needs to be solved. This design is my
> attempt to solve that.
>
>>
>> Regards,
>> Halil
>>
>>>    */
>>>   struct ap_matrix_dev {
>>>       struct device device;
>>> @@ -47,6 +55,8 @@ struct ap_matrix_dev {
>>>       struct list_head mdev_list;
>>>       struct mutex lock;
>>>       struct ap_driver  *vfio_ap_drv;
>>> +    struct rw_semaphore guests_lock;
>>> +    struct list_head guests;
>>>   };
>>>     extern struct ap_matrix_dev *matrix_dev;
>

Powered by blists - more mailing lists