Re: [PATCH v17 08/15] s390/vfio-ap: keep track of active guests
From: Tony Krowiak
Date: Fri Jan 14 2022 - 19:29:40 EST
On 1/12/22 09:25, Halil Pasic wrote:
On Tue, 11 Jan 2022 16:58:13 -0500
Tony Krowiak <akrowiak@xxxxxxxxxxxxx> wrote:
On 12/29/21 22:33, Halil Pasic wrote:
On Thu, 21 Oct 2021 11:23:25 -0400
Tony Krowiak <akrowiak@xxxxxxxxxxxxx> 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).
Yes reading the code, my impression was, that this is more about the
ap_guest.kvm that about the list.
My understanding is that struct ap_gurest is basically about the
marriage between a matrix_mdev and a kvm. Basically a link between the
two.
But then, it probably does not make a sense for this link to outlive
either kvm or matrix_mdev.
Thus I don't quite understand why do we need the extra allocation? If
we want a list, why don't we just the pointers to matrix_mdev?
We could still protect that stuff with a separate lock.
I think this may be a good idea. We already have a list of matrix_mdev
stored in matrix_dev. I'll explore this further.
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.
Well if the guests_lock is only protecting the list, then that should not
be true. In that case, you can be only sure about the nodes that you
reached by traversing the list with he lock held. Right.
If only the list is protected, then one could do
down_write(guests_lock)
list_del(element)
up_write(guests_lock)
fancy_free(element)
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.
Yeah, that is what gets rid of the circular lock dependency. If we had
to take guests_lock there we would have guests_lock in the same role
as matrix_dev->lock before.
But the thing is you do
kvm = q->matrix_mdev->guest->kvm;
in the pqap_handler (more precisely in a function called by it).
So you do access the struct ap_guest object and its kvm member
without the guests_lock being held. That is where things become very
muddy to me.
I was thinking about this the other day, that the kvm pointer is
needed when the IRQ is disabled to clean up the gisa stuff and
the pinned memory. I'm going to revisit this.
It looks to me that the kvm pointer is changed with both the
guests_lock and the matrix_dev->lock held in write mode. And accessing
such stuff read only is safe with either of the two locks held.
Thus I do believe that the general idea is viable. I've pointed that out
in a later email.
But the information you give the unsuspecting reader to aid him in
understanding our new locking scheme is severely lacking.
I'll try to clear up the patch description.
[..]
+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.
Well, but guests is a member of struct ap_matrix_dev and not the whole
list including all the nodes.
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.
I agree that having a lock that we take before kvm->lock is taken,
and another one that we take with the kvm->lock taken is a good idea.
I was referring to having ap_guest objects which are separately
allocated, and have a decoupled lifecycle. Please see above!
I'm thinking about looking into getting rid of the struct ap_guest and
the guests list as I said above. I think I can rework this.
Regards,
Halil
[..]