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

[Halil Pasic]

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.

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.

[..]

> +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?

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.

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;