Messages in this thread | | | Date | Tue, 11 Jan 2022 17:19:35 -0500 | Subject | Re: [PATCH v17 08/15] s390/vfio-ap: keep track of active guests | From | Tony Krowiak <> |
| |
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@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.
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; >
| |