Message-ID: <fcce7cc6-6ac7-b22a-a957-80e59a0f4e83@linux.ibm.com>
Date:   Tue, 11 Jan 2022 16:58:13 -0500
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.3.0
Subject: Re: [PATCH v17 08/15] s390/vfio-ap: keep track of active guests
Content-Language: en-US
To:     Halil Pasic <pasic@linux.ibm.com>
Cc:     linux-s390@vger.kernel.org, linux-kernel@vger.kernel.org,
        kvm@vger.kernel.org, jjherne@linux.ibm.com, freude@linux.ibm.com,
        borntraeger@de.ibm.com, cohuck@redhat.com, mjrosato@linux.ibm.com,
        alex.williamson@redhat.com, kwankhede@nvidia.com,
        fiuczy@linux.ibm.com
References: <20211021152332.70455-1-akrowiak@linux.ibm.com>
 <20211021152332.70455-9-akrowiak@linux.ibm.com>
 <20211230043322.2ba19bbd.pasic@linux.ibm.com>
From:   Tony Krowiak <akrowiak@linux.ibm.com>
In-Reply-To: <20211230043322.2ba19bbd.pasic@linux.ibm.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk


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;