Re: [PATCH v17 08/15] s390/vfio-ap: keep track of active guests

[Tony Krowiak <akrowiak@linux.ibm.com>]

On 12/29/21 22:33, Halil Pasic wrote:
> On Thu, 21 Oct 2021 11:23:25 -0400
> Tony Krowiak <akrowiak@linux.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.

>
> 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;