diff mbox series

[02/34] drm: Convert aux_idr to XArray

Message ID 20190221184226.2149-4-willy@infradead.org (mailing list archive)
State New, archived
Headers show
Series Convert DRM to XArray | expand

Commit Message

Matthew Wilcox Feb. 21, 2019, 6:41 p.m. UTC 
Signed-off-by: Matthew Wilcox <willy@infradead.org>
---
 drivers/gpu/drm/drm_dp_aux_dev.c | 41 +++++++++++++-------------------
 1 file changed, 17 insertions(+), 24 deletions(-)

Comments

Ville Syrjälä Feb. 25, 2019, 5:57 p.m. UTC | #1 
On Thu, Feb 21, 2019 at 10:41:23AM -0800, Matthew Wilcox wrote:
> Signed-off-by: Matthew Wilcox <willy@infradead.org>
> ---
>  drivers/gpu/drm/drm_dp_aux_dev.c | 41 +++++++++++++-------------------
>  1 file changed, 17 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_dp_aux_dev.c b/drivers/gpu/drm/drm_dp_aux_dev.c
> index 0e4f25d63fd2..393a32976900 100644
> --- a/drivers/gpu/drm/drm_dp_aux_dev.c
> +++ b/drivers/gpu/drm/drm_dp_aux_dev.c
> @@ -49,8 +49,7 @@ struct drm_dp_aux_dev {
>  
>  #define DRM_AUX_MINORS	256
>  #define AUX_MAX_OFFSET	(1 << 20)
> -static DEFINE_IDR(aux_idr);
> -static DEFINE_MUTEX(aux_idr_mutex);
> +static DEFINE_XARRAY_ALLOC(aux_xa);
>  static struct class *drm_dp_aux_dev_class;
>  static int drm_dev_major = -1;
>  
> @@ -58,19 +57,21 @@ static struct drm_dp_aux_dev *drm_dp_aux_dev_get_by_minor(unsigned index)
>  {
>  	struct drm_dp_aux_dev *aux_dev = NULL;
>  
> -	mutex_lock(&aux_idr_mutex);
> -	aux_dev = idr_find(&aux_idr, index);
> +	xa_lock(&aux_xa);
> +	aux_dev = xa_load(&aux_xa, index);
>  	if (!kref_get_unless_zero(&aux_dev->refcount))
>  		aux_dev = NULL;
> -	mutex_unlock(&aux_idr_mutex);
> +	xa_unlock(&aux_xa);
>  
>  	return aux_dev;
>  }
>  
>  static struct drm_dp_aux_dev *alloc_drm_dp_aux_dev(struct drm_dp_aux *aux)
>  {
> +	static u32 next;

Is there a particular reason for that being static?

> +
>  	struct drm_dp_aux_dev *aux_dev;
> -	int index;
> +	int err;
>  
>  	aux_dev = kzalloc(sizeof(*aux_dev), GFP_KERNEL);
>  	if (!aux_dev)
> @@ -79,15 +80,12 @@ static struct drm_dp_aux_dev *alloc_drm_dp_aux_dev(struct drm_dp_aux *aux)
>  	atomic_set(&aux_dev->usecount, 1);
>  	kref_init(&aux_dev->refcount);
>  
> -	mutex_lock(&aux_idr_mutex);
> -	index = idr_alloc_cyclic(&aux_idr, aux_dev, 0, DRM_AUX_MINORS,
> -				 GFP_KERNEL);
> -	mutex_unlock(&aux_idr_mutex);
> -	if (index < 0) {
> +	err = xa_alloc_cyclic(&aux_xa, &aux_dev->index, aux_dev,
> +			XA_LIMIT(0, DRM_AUX_MINORS), &next, GFP_KERNEL);
> +	if (err < 0) {
>  		kfree(aux_dev);
> -		return ERR_PTR(index);
> +		return ERR_PTR(err);
>  	}
> -	aux_dev->index = index;
>  
>  	return aux_dev;
>  }
> @@ -246,22 +244,19 @@ static const struct file_operations auxdev_fops = {
>  
>  static struct drm_dp_aux_dev *drm_dp_aux_dev_get_by_aux(struct drm_dp_aux *aux)
>  {
> -	struct drm_dp_aux_dev *iter, *aux_dev = NULL;
> -	int id;
> +	struct drm_dp_aux_dev *aux_dev;
> +	unsigned long id;
>  
>  	/* don't increase kref count here because this function should only be
>  	 * used by drm_dp_aux_unregister_devnode. Thus, it will always have at
>  	 * least one reference - the one that drm_dp_aux_register_devnode
>  	 * created
>  	 */
> -	mutex_lock(&aux_idr_mutex);
> -	idr_for_each_entry(&aux_idr, iter, id) {
> -		if (iter->aux == aux) {
> -			aux_dev = iter;
> +	xa_for_each(&aux_xa, id, aux_dev) {
> +		if (aux_dev->aux == aux)
>  			break;
> -		}
>  	}
> -	mutex_unlock(&aux_idr_mutex);
> +
>  	return aux_dev;
>  }
>  
> @@ -274,9 +269,7 @@ void drm_dp_aux_unregister_devnode(struct drm_dp_aux *aux)
>  	if (!aux_dev) /* attach must have failed */
>  		return;
>  
> -	mutex_lock(&aux_idr_mutex);
> -	idr_remove(&aux_idr, aux_dev->index);
> -	mutex_unlock(&aux_idr_mutex);
> +	xa_erase(&aux_xa, aux_dev->index);
>  
>  	atomic_dec(&aux_dev->usecount);
>  	wait_var_event(&aux_dev->usecount, !atomic_read(&aux_dev->usecount));
> -- 
> 2.20.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
Matthew Wilcox Feb. 25, 2019, 6:42 p.m. UTC | #2 
On Mon, Feb 25, 2019 at 07:57:33PM +0200, Ville Syrjälä wrote:
> On Thu, Feb 21, 2019 at 10:41:23AM -0800, Matthew Wilcox wrote:
> > @@ -49,8 +49,7 @@ struct drm_dp_aux_dev {
> >  
> >  #define DRM_AUX_MINORS	256
> >  #define AUX_MAX_OFFSET	(1 << 20)
> > -static DEFINE_IDR(aux_idr);
> > -static DEFINE_MUTEX(aux_idr_mutex);
> > +static DEFINE_XARRAY_ALLOC(aux_xa);
> >  static struct class *drm_dp_aux_dev_class;
> >  static int drm_dev_major = -1;
[...]
> >  static struct drm_dp_aux_dev *alloc_drm_dp_aux_dev(struct drm_dp_aux *aux)
> >  {
> > +	static u32 next;
> 
> Is there a particular reason for that being static?

It needs to maintain its value between function calls so that we know
where to start allocating from the next time we call xa_alloc_cyclic().
It can either live here with a short name, or we could put it at file
scope with a descriptive name (probably aux_xa_next).  It's up to you
which you think is better; it's your driver.

The IDR embedded the 'next' value in the struct idr, but that was
overhead paid by all users of the IDR rather than the very few that
called idr_alloc_cyclic().

> > +	err = xa_alloc_cyclic(&aux_xa, &aux_dev->index, aux_dev,
> > +			XA_LIMIT(0, DRM_AUX_MINORS), &next, GFP_KERNEL);
> > +	if (err < 0) {
> >  		kfree(aux_dev);
Ville Syrjälä Feb. 25, 2019, 6:50 p.m. UTC | #3 
On Mon, Feb 25, 2019 at 10:42:46AM -0800, Matthew Wilcox wrote:
> On Mon, Feb 25, 2019 at 07:57:33PM +0200, Ville Syrjälä wrote:
> > On Thu, Feb 21, 2019 at 10:41:23AM -0800, Matthew Wilcox wrote:
> > > @@ -49,8 +49,7 @@ struct drm_dp_aux_dev {
> > >  
> > >  #define DRM_AUX_MINORS	256
> > >  #define AUX_MAX_OFFSET	(1 << 20)
> > > -static DEFINE_IDR(aux_idr);
> > > -static DEFINE_MUTEX(aux_idr_mutex);
> > > +static DEFINE_XARRAY_ALLOC(aux_xa);
> > >  static struct class *drm_dp_aux_dev_class;
> > >  static int drm_dev_major = -1;
> [...]
> > >  static struct drm_dp_aux_dev *alloc_drm_dp_aux_dev(struct drm_dp_aux *aux)
> > >  {
> > > +	static u32 next;
> > 
> > Is there a particular reason for that being static?
> 
> It needs to maintain its value between function calls so that we know
> where to start allocating from the next time we call xa_alloc_cyclic().
> It can either live here with a short name, or we could put it at file
> scope with a descriptive name (probably aux_xa_next).  It's up to you
> which you think is better; it's your driver.

File scope would seem a bit more clear to me. I'm slightly worried
someone might do some cleanup here and drop the static without
thinking. Alterntively a short comment would probably suffice.

> 
> The IDR embedded the 'next' value in the struct idr, but that was
> overhead paid by all users of the IDR rather than the very few that
> called idr_alloc_cyclic().
> 
> > > +	err = xa_alloc_cyclic(&aux_xa, &aux_dev->index, aux_dev,
> > > +			XA_LIMIT(0, DRM_AUX_MINORS), &next, GFP_KERNEL);
> > > +	if (err < 0) {
> > >  		kfree(aux_dev);
diff mbox series

Patch

diff --git a/drivers/gpu/drm/drm_dp_aux_dev.c b/drivers/gpu/drm/drm_dp_aux_dev.c
index 0e4f25d63fd2..393a32976900 100644
--- a/drivers/gpu/drm/drm_dp_aux_dev.c
+++ b/drivers/gpu/drm/drm_dp_aux_dev.c
@@ -49,8 +49,7 @@  struct drm_dp_aux_dev {
 
 #define DRM_AUX_MINORS	256
 #define AUX_MAX_OFFSET	(1 << 20)
-static DEFINE_IDR(aux_idr);
-static DEFINE_MUTEX(aux_idr_mutex);
+static DEFINE_XARRAY_ALLOC(aux_xa);
 static struct class *drm_dp_aux_dev_class;
 static int drm_dev_major = -1;
 
@@ -58,19 +57,21 @@  static struct drm_dp_aux_dev *drm_dp_aux_dev_get_by_minor(unsigned index)
 {
 	struct drm_dp_aux_dev *aux_dev = NULL;
 
-	mutex_lock(&aux_idr_mutex);
-	aux_dev = idr_find(&aux_idr, index);
+	xa_lock(&aux_xa);
+	aux_dev = xa_load(&aux_xa, index);
 	if (!kref_get_unless_zero(&aux_dev->refcount))
 		aux_dev = NULL;
-	mutex_unlock(&aux_idr_mutex);
+	xa_unlock(&aux_xa);
 
 	return aux_dev;
 }
 
 static struct drm_dp_aux_dev *alloc_drm_dp_aux_dev(struct drm_dp_aux *aux)
 {
+	static u32 next;
+
 	struct drm_dp_aux_dev *aux_dev;
-	int index;
+	int err;
 
 	aux_dev = kzalloc(sizeof(*aux_dev), GFP_KERNEL);
 	if (!aux_dev)
@@ -79,15 +80,12 @@  static struct drm_dp_aux_dev *alloc_drm_dp_aux_dev(struct drm_dp_aux *aux)
 	atomic_set(&aux_dev->usecount, 1);
 	kref_init(&aux_dev->refcount);
 
-	mutex_lock(&aux_idr_mutex);
-	index = idr_alloc_cyclic(&aux_idr, aux_dev, 0, DRM_AUX_MINORS,
-				 GFP_KERNEL);
-	mutex_unlock(&aux_idr_mutex);
-	if (index < 0) {
+	err = xa_alloc_cyclic(&aux_xa, &aux_dev->index, aux_dev,
+			XA_LIMIT(0, DRM_AUX_MINORS), &next, GFP_KERNEL);
+	if (err < 0) {
 		kfree(aux_dev);
-		return ERR_PTR(index);
+		return ERR_PTR(err);
 	}
-	aux_dev->index = index;
 
 	return aux_dev;
 }
@@ -246,22 +244,19 @@  static const struct file_operations auxdev_fops = {
 
 static struct drm_dp_aux_dev *drm_dp_aux_dev_get_by_aux(struct drm_dp_aux *aux)
 {
-	struct drm_dp_aux_dev *iter, *aux_dev = NULL;
-	int id;
+	struct drm_dp_aux_dev *aux_dev;
+	unsigned long id;
 
 	/* don't increase kref count here because this function should only be
 	 * used by drm_dp_aux_unregister_devnode. Thus, it will always have at
 	 * least one reference - the one that drm_dp_aux_register_devnode
 	 * created
 	 */
-	mutex_lock(&aux_idr_mutex);
-	idr_for_each_entry(&aux_idr, iter, id) {
-		if (iter->aux == aux) {
-			aux_dev = iter;
+	xa_for_each(&aux_xa, id, aux_dev) {
+		if (aux_dev->aux == aux)
 			break;
-		}
 	}
-	mutex_unlock(&aux_idr_mutex);
+
 	return aux_dev;
 }
 
@@ -274,9 +269,7 @@  void drm_dp_aux_unregister_devnode(struct drm_dp_aux *aux)
 	if (!aux_dev) /* attach must have failed */
 		return;
 
-	mutex_lock(&aux_idr_mutex);
-	idr_remove(&aux_idr, aux_dev->index);
-	mutex_unlock(&aux_idr_mutex);
+	xa_erase(&aux_xa, aux_dev->index);
 
 	atomic_dec(&aux_dev->usecount);
 	wait_var_event(&aux_dev->usecount, !atomic_read(&aux_dev->usecount));