diff mbox series

[v16,02/14] s390/vfio-ap: use new AP bus interface to search for queue devices

Message ID 20210510164423.346858-3-akrowiak@linux.ibm.com (mailing list archive)
State New, archived
Headers show
Series s390/vfio-ap: dynamic configuration support | expand

Commit Message

Anthony Krowiak May 10, 2021, 4:44 p.m. UTC 
This patch refactors the vfio_ap device driver to use the AP bus's
ap_get_qdev() function to retrieve the vfio_ap_queue struct containing
information about a queue that is bound to the vfio_ap device driver.
The bus's ap_get_qdev() function retrieves the queue device from a
hashtable keyed by APQN. This is much more efficient than looping over
the list of devices attached to the AP bus by several orders of
magnitude.

Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com>
Reviewed-by: Halil Pasic <pasic@linux.ibm.com>
---
 drivers/s390/crypto/vfio_ap_ops.c | 23 +++++++++--------------
 1 file changed, 9 insertions(+), 14 deletions(-)

Comments

Jason J. Herne June 9, 2021, 1:52 p.m. UTC | #1 
On 5/10/21 12:44 PM, Tony Krowiak wrote:
> This patch refactors the vfio_ap device driver to use the AP bus's
> ap_get_qdev() function to retrieve the vfio_ap_queue struct containing
> information about a queue that is bound to the vfio_ap device driver.
> The bus's ap_get_qdev() function retrieves the queue device from a
> hashtable keyed by APQN. This is much more efficient than looping over
> the list of devices attached to the AP bus by several orders of
> magnitude.
> 
> Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com>
> Reviewed-by: Halil Pasic <pasic@linux.ibm.com>
> ---
>   drivers/s390/crypto/vfio_ap_ops.c | 23 +++++++++--------------
>   1 file changed, 9 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> index 757166da947e..8a50aa650b65 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> @@ -27,13 +27,6 @@
>   static int vfio_ap_mdev_reset_queues(struct mdev_device *mdev);
>   static struct vfio_ap_queue *vfio_ap_find_queue(int apqn);
>   
> -static int match_apqn(struct device *dev, const void *data)
> -{
> -	struct vfio_ap_queue *q = dev_get_drvdata(dev);
> -
> -	return (q->apqn == *(int *)(data)) ? 1 : 0;
> -}
> -
>   /**
>    * vfio_ap_get_queue: Retrieve a queue with a specific APQN from a list
>    * @matrix_mdev: the associated mediated matrix
> @@ -1253,15 +1246,17 @@ static int vfio_ap_mdev_group_notifier(struct notifier_block *nb,
>   
>   static struct vfio_ap_queue *vfio_ap_find_queue(int apqn)
>   {
> -	struct device *dev;
> +	struct ap_queue *queue;
>   	struct vfio_ap_queue *q = NULL;

The use of q and queue as variable names was a little confusing to me at first. I tried 
renaming them a few times, the best I could come up with was this:

struct ap_queue *queue;
struct vfio_ap_queue *vfio_queue = NULL;

Take it or leave it :) Other than that, LGTM.
Reviewed-by: Jason J. Herne <jjherne@linux.ibm.com>


> -	dev = driver_find_device(&matrix_dev->vfio_ap_drv->driver, NULL,
> -				 &apqn, match_apqn);
> -	if (dev) {
> -		q = dev_get_drvdata(dev);
> -		put_device(dev);
> -	}
> +	queue = ap_get_qdev(apqn);
> +	if (!queue)
> +		return NULL;
> +
> +	if (queue->ap_dev.device.driver == &matrix_dev->vfio_ap_drv->driver)
> +		q = dev_get_drvdata(&queue->ap_dev.device);
> +
> +	put_device(&queue->ap_dev.device);
>   
>   	return q;
>   }
>
Anthony Krowiak June 9, 2021, 6:23 p.m. UTC | #2 
On 6/9/21 9:52 AM, Jason J. Herne wrote:
> On 5/10/21 12:44 PM, Tony Krowiak wrote:
>> This patch refactors the vfio_ap device driver to use the AP bus's
>> ap_get_qdev() function to retrieve the vfio_ap_queue struct containing
>> information about a queue that is bound to the vfio_ap device driver.
>> The bus's ap_get_qdev() function retrieves the queue device from a
>> hashtable keyed by APQN. This is much more efficient than looping over
>> the list of devices attached to the AP bus by several orders of
>> magnitude.
>>
>> Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com>
>> Reviewed-by: Halil Pasic <pasic@linux.ibm.com>
>> ---
>>   drivers/s390/crypto/vfio_ap_ops.c | 23 +++++++++--------------
>>   1 file changed, 9 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/s390/crypto/vfio_ap_ops.c 
>> b/drivers/s390/crypto/vfio_ap_ops.c
>> index 757166da947e..8a50aa650b65 100644
>> --- a/drivers/s390/crypto/vfio_ap_ops.c
>> +++ b/drivers/s390/crypto/vfio_ap_ops.c
>> @@ -27,13 +27,6 @@
>>   static int vfio_ap_mdev_reset_queues(struct mdev_device *mdev);
>>   static struct vfio_ap_queue *vfio_ap_find_queue(int apqn);
>>   -static int match_apqn(struct device *dev, const void *data)
>> -{
>> -    struct vfio_ap_queue *q = dev_get_drvdata(dev);
>> -
>> -    return (q->apqn == *(int *)(data)) ? 1 : 0;
>> -}
>> -
>>   /**
>>    * vfio_ap_get_queue: Retrieve a queue with a specific APQN from a 
>> list
>>    * @matrix_mdev: the associated mediated matrix
>> @@ -1253,15 +1246,17 @@ static int vfio_ap_mdev_group_notifier(struct 
>> notifier_block *nb,
>>     static struct vfio_ap_queue *vfio_ap_find_queue(int apqn)
>>   {
>> -    struct device *dev;
>> +    struct ap_queue *queue;
>>       struct vfio_ap_queue *q = NULL;
>
> The use of q and queue as variable names was a little confusing to me 
> at first. I tried renaming them a few times, the best I could come up 
> with was this:

I am (was) not a fan of the naming convention, but using 'q' to name a 
struct vfio_ap_queue
was introduced (not by me) quite some time ago, so rather than 
introducing a variable name
change at this juncture, I stuck with it to be consistent with the other 
places it is used.

>
> struct ap_queue *queue;
> struct vfio_ap_queue *vfio_queue = NULL;
>
> Take it or leave it :) Other than that, LGTM.
> Reviewed-by: Jason J. Herne <jjherne@linux.ibm.com>

Thanks for your review.

>
>
>> -    dev = driver_find_device(&matrix_dev->vfio_ap_drv->driver, NULL,
>> -                 &apqn, match_apqn);
>> -    if (dev) {
>> -        q = dev_get_drvdata(dev);
>> -        put_device(dev);
>> -    }
>> +    queue = ap_get_qdev(apqn);
>> +    if (!queue)
>> +        return NULL;
>> +
>> +    if (queue->ap_dev.device.driver == 
>> &matrix_dev->vfio_ap_drv->driver)
>> +        q = dev_get_drvdata(&queue->ap_dev.device);
>> +
>> +    put_device(&queue->ap_dev.device);
>>         return q;
>>   }
>>
>
>
diff mbox series

Patch

diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index 757166da947e..8a50aa650b65 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -27,13 +27,6 @@ 
 static int vfio_ap_mdev_reset_queues(struct mdev_device *mdev);
 static struct vfio_ap_queue *vfio_ap_find_queue(int apqn);
 
-static int match_apqn(struct device *dev, const void *data)
-{
-	struct vfio_ap_queue *q = dev_get_drvdata(dev);
-
-	return (q->apqn == *(int *)(data)) ? 1 : 0;
-}
-
 /**
  * vfio_ap_get_queue: Retrieve a queue with a specific APQN from a list
  * @matrix_mdev: the associated mediated matrix
@@ -1253,15 +1246,17 @@  static int vfio_ap_mdev_group_notifier(struct notifier_block *nb,
 
 static struct vfio_ap_queue *vfio_ap_find_queue(int apqn)
 {
-	struct device *dev;
+	struct ap_queue *queue;
 	struct vfio_ap_queue *q = NULL;
 
-	dev = driver_find_device(&matrix_dev->vfio_ap_drv->driver, NULL,
-				 &apqn, match_apqn);
-	if (dev) {
-		q = dev_get_drvdata(dev);
-		put_device(dev);
-	}
+	queue = ap_get_qdev(apqn);
+	if (!queue)
+		return NULL;
+
+	if (queue->ap_dev.device.driver == &matrix_dev->vfio_ap_drv->driver)
+		q = dev_get_drvdata(&queue->ap_dev.device);
+
+	put_device(&queue->ap_dev.device);
 
 	return q;
 }