Re: [PATCH v6 1/4] drm: Use XArray instead of IDR for minors

2023-08-29 Thread Matthew Wilcox 
On Tue, Aug 29, 2023 at 02:35:34PM -0400, James Zhu wrote:
> 
> On 2023-08-29 14:33, Matthew Wilcox wrote:
> > On Tue, Aug 29, 2023 at 01:34:22PM -0400, James Zhu wrote:
> > > > > > @@ -1067,7 +1055,7 @@ static void drm_core_exit(void)
> > > > > > unregister_chrdev(DRM_MAJOR, "drm");
> > > > > > debugfs_remove(drm_debugfs_root);
> > > > > > drm_sysfs_destroy();
> > > > > > -   idr_destroy(_minors_idr);
> > > > > [JZ] Should we call xa_destroy instead here?
> > > > We could, if we expect the xarray to potentially not be empty.
> > > >   From what I understand - all minors should be released at this point.
> > > [JZ] In practice,  adding destroy here will be better.
> > Why do you say that?
> [JZ] In case, the future, INIT adds something, need DESTROY to free
> completely.

That isn't going to happen.

Re: [PATCH v6 1/4] drm: Use XArray instead of IDR for minors

2023-08-29 Thread James Zhu 



On 2023-08-29 14:33, Matthew Wilcox wrote:

On Tue, Aug 29, 2023 at 01:34:22PM -0400, James Zhu wrote:

@@ -1067,7 +1055,7 @@ static void drm_core_exit(void)
unregister_chrdev(DRM_MAJOR, "drm");
debugfs_remove(drm_debugfs_root);
drm_sysfs_destroy();
-   idr_destroy(_minors_idr);

[JZ] Should we call xa_destroy instead here?

We could, if we expect the xarray to potentially not be empty.
  From what I understand - all minors should be released at this point.

[JZ] In practice,  adding destroy here will be better.

Why do you say that?
[JZ] In case, the future, INIT adds something, need DESTROY to free 
completely.

Re: [PATCH v6 1/4] drm: Use XArray instead of IDR for minors

2023-08-29 Thread Matthew Wilcox 
On Tue, Aug 29, 2023 at 01:34:22PM -0400, James Zhu wrote:
> > > > @@ -1067,7 +1055,7 @@ static void drm_core_exit(void)
> > > > unregister_chrdev(DRM_MAJOR, "drm");
> > > > debugfs_remove(drm_debugfs_root);
> > > > drm_sysfs_destroy();
> > > > -   idr_destroy(_minors_idr);
> > > [JZ] Should we call xa_destroy instead here?
> > We could, if we expect the xarray to potentially not be empty.
> >  From what I understand - all minors should be released at this point.
> [JZ] In practice,  adding destroy here will be better.

Why do you say that?

Re: [PATCH v6 1/4] drm: Use XArray instead of IDR for minors

2023-08-29 Thread James Zhu 



On 2023-08-28 17:08, Michał Winiarski wrote:

On Fri, Aug 25, 2023 at 12:59:26PM -0400, James Zhu wrote:

On 2023-07-24 17:14, Michał Winiarski wrote:

IDR is deprecated, and since XArray manages its own state with internal
locking, it simplifies the locking on DRM side.
Additionally, don't use the IRQ-safe variant, since operating on drm
minor is not done in IRQ context.

Signed-off-by: Michał Winiarski
Suggested-by: Matthew Wilcox
---
   drivers/gpu/drm/drm_drv.c | 63 ---
   1 file changed, 25 insertions(+), 38 deletions(-)

diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 3eda026ffac6..3faecb01186f 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -34,6 +34,7 @@
   #include 
   #include 
   #include 
+#include 
   #include 
   #include 
@@ -54,8 +55,7 @@ MODULE_AUTHOR("Gareth Hughes, Leif Delgass, José Fonseca, Jon 
Smirl");
   MODULE_DESCRIPTION("DRM shared core routines");
   MODULE_LICENSE("GPL and additional rights");
-static DEFINE_SPINLOCK(drm_minor_lock);
-static struct idr drm_minors_idr;
+static DEFINE_XARRAY_ALLOC(drm_minors_xa);
   /*
* If the drm core fails to init for whatever reason,
@@ -101,26 +101,23 @@ static struct drm_minor **drm_minor_get_slot(struct 
drm_device *dev,
   static void drm_minor_alloc_release(struct drm_device *dev, void *data)
   {
struct drm_minor *minor = data;
-   unsigned long flags;
WARN_ON(dev != minor->dev);
put_device(minor->kdev);
-   if (minor->type == DRM_MINOR_ACCEL) {
+   if (minor->type == DRM_MINOR_ACCEL)
accel_minor_remove(minor->index);
-   } else {
-   spin_lock_irqsave(_minor_lock, flags);
-   idr_remove(_minors_idr, minor->index);
-   spin_unlock_irqrestore(_minor_lock, flags);
-   }
+   else
+   xa_erase(_minors_xa, minor->index);
   }
+#define DRM_MINOR_LIMIT(t) ({ typeof(t) _t = (t); XA_LIMIT(64 * _t, 64 * _t + 
63); })
+
   static int drm_minor_alloc(struct drm_device *dev, enum drm_minor_type type)
   {
struct drm_minor *minor;
-   unsigned long flags;
-   int r;
+   int index, r;
minor = drmm_kzalloc(dev, sizeof(*minor), GFP_KERNEL);
if (!minor)
@@ -129,24 +126,17 @@ static int drm_minor_alloc(struct drm_device *dev, enum 
drm_minor_type type)
minor->type = type;
minor->dev = dev;
-   idr_preload(GFP_KERNEL);
if (type == DRM_MINOR_ACCEL) {
r = accel_minor_alloc();
+   index = r;
} else {
-   spin_lock_irqsave(_minor_lock, flags);
-   r = idr_alloc(_minors_idr,
-   NULL,
-   64 * type,
-   64 * (type + 1),
-   GFP_NOWAIT);
-   spin_unlock_irqrestore(_minor_lock, flags);
+   r = xa_alloc(_minors_xa, , NULL, 
DRM_MINOR_LIMIT(type), GFP_KERNEL);
}
-   idr_preload_end();
if (r < 0)
return r;
-   minor->index = r;
+   minor->index = index;
r = drmm_add_action_or_reset(dev, drm_minor_alloc_release, minor);
if (r)
@@ -163,7 +153,7 @@ static int drm_minor_alloc(struct drm_device *dev, enum 
drm_minor_type type)
   static int drm_minor_register(struct drm_device *dev, enum drm_minor_type 
type)
   {
struct drm_minor *minor;
-   unsigned long flags;
+   void *entry;
int ret;
DRM_DEBUG("\n");
@@ -190,9 +180,12 @@ static int drm_minor_register(struct drm_device *dev, enum 
drm_minor_type type)
if (minor->type == DRM_MINOR_ACCEL) {
accel_minor_replace(minor, minor->index);
} else {
-   spin_lock_irqsave(_minor_lock, flags);
-   idr_replace(_minors_idr, minor, minor->index);
-   spin_unlock_irqrestore(_minor_lock, flags);
+   entry = xa_store(_minors_xa, minor->index, minor, 
GFP_KERNEL);
+   if (xa_is_err(entry)) {
+   ret = xa_err(entry);
+   goto err_debugfs;
+   }
+   WARN_ON(entry);

[JZ] would WARN_ON(entry != minor)be better?

We expect NULL here.
The same one that was previously stored here ⌄⌄⌄
r = xa_alloc(_minors_xa, >index, NULL, DRM_EXTENDED_MINOR_LIMIT, 
GFP_KERNEL);
in drm_minor_alloc.

[JZ] My mistake.



}
DRM_DEBUG("new minor registered %d\n", minor->index);
@@ -206,20 +199,16 @@ static int drm_minor_register(struct drm_device *dev, 
enum drm_minor_type type)
   static void drm_minor_unregister(struct drm_device *dev, enum drm_minor_type 
type)
   {
struct drm_minor *minor;
-   unsigned long flags;
minor = *drm_minor_get_slot(dev, type);
if (!minor || !device_is_registered(minor->kdev))
return;
/* replace @minor with NULL so lookups will fail from now on */
-   if (minor->type == DRM_MINOR_ACCEL) {
+

Re: [PATCH v6 1/4] drm: Use XArray instead of IDR for minors

2023-08-28 Thread Michał Winiarski 
On Fri, Aug 25, 2023 at 12:59:26PM -0400, James Zhu wrote:
> 
> On 2023-07-24 17:14, Michał Winiarski wrote:
> > IDR is deprecated, and since XArray manages its own state with internal
> > locking, it simplifies the locking on DRM side.
> > Additionally, don't use the IRQ-safe variant, since operating on drm
> > minor is not done in IRQ context.
> > 
> > Signed-off-by: Michał Winiarski
> > Suggested-by: Matthew Wilcox
> > ---
> >   drivers/gpu/drm/drm_drv.c | 63 ---
> >   1 file changed, 25 insertions(+), 38 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> > index 3eda026ffac6..3faecb01186f 100644
> > --- a/drivers/gpu/drm/drm_drv.c
> > +++ b/drivers/gpu/drm/drm_drv.c
> > @@ -34,6 +34,7 @@
> >   #include 
> >   #include 
> >   #include 
> > +#include 
> >   #include 
> >   #include 
> > @@ -54,8 +55,7 @@ MODULE_AUTHOR("Gareth Hughes, Leif Delgass, José Fonseca, 
> > Jon Smirl");
> >   MODULE_DESCRIPTION("DRM shared core routines");
> >   MODULE_LICENSE("GPL and additional rights");
> > -static DEFINE_SPINLOCK(drm_minor_lock);
> > -static struct idr drm_minors_idr;
> > +static DEFINE_XARRAY_ALLOC(drm_minors_xa);
> >   /*
> >* If the drm core fails to init for whatever reason,
> > @@ -101,26 +101,23 @@ static struct drm_minor **drm_minor_get_slot(struct 
> > drm_device *dev,
> >   static void drm_minor_alloc_release(struct drm_device *dev, void *data)
> >   {
> > struct drm_minor *minor = data;
> > -   unsigned long flags;
> > WARN_ON(dev != minor->dev);
> > put_device(minor->kdev);
> > -   if (minor->type == DRM_MINOR_ACCEL) {
> > +   if (minor->type == DRM_MINOR_ACCEL)
> > accel_minor_remove(minor->index);
> > -   } else {
> > -   spin_lock_irqsave(_minor_lock, flags);
> > -   idr_remove(_minors_idr, minor->index);
> > -   spin_unlock_irqrestore(_minor_lock, flags);
> > -   }
> > +   else
> > +   xa_erase(_minors_xa, minor->index);
> >   }
> > +#define DRM_MINOR_LIMIT(t) ({ typeof(t) _t = (t); XA_LIMIT(64 * _t, 64 * 
> > _t + 63); })
> > +
> >   static int drm_minor_alloc(struct drm_device *dev, enum drm_minor_type 
> > type)
> >   {
> > struct drm_minor *minor;
> > -   unsigned long flags;
> > -   int r;
> > +   int index, r;
> > minor = drmm_kzalloc(dev, sizeof(*minor), GFP_KERNEL);
> > if (!minor)
> > @@ -129,24 +126,17 @@ static int drm_minor_alloc(struct drm_device *dev, 
> > enum drm_minor_type type)
> > minor->type = type;
> > minor->dev = dev;
> > -   idr_preload(GFP_KERNEL);
> > if (type == DRM_MINOR_ACCEL) {
> > r = accel_minor_alloc();
> > +   index = r;
> > } else {
> > -   spin_lock_irqsave(_minor_lock, flags);
> > -   r = idr_alloc(_minors_idr,
> > -   NULL,
> > -   64 * type,
> > -   64 * (type + 1),
> > -   GFP_NOWAIT);
> > -   spin_unlock_irqrestore(_minor_lock, flags);
> > +   r = xa_alloc(_minors_xa, , NULL, 
> > DRM_MINOR_LIMIT(type), GFP_KERNEL);
> > }
> > -   idr_preload_end();
> > if (r < 0)
> > return r;
> > -   minor->index = r;
> > +   minor->index = index;
> > r = drmm_add_action_or_reset(dev, drm_minor_alloc_release, minor);
> > if (r)
> > @@ -163,7 +153,7 @@ static int drm_minor_alloc(struct drm_device *dev, enum 
> > drm_minor_type type)
> >   static int drm_minor_register(struct drm_device *dev, enum drm_minor_type 
> > type)
> >   {
> > struct drm_minor *minor;
> > -   unsigned long flags;
> > +   void *entry;
> > int ret;
> > DRM_DEBUG("\n");
> > @@ -190,9 +180,12 @@ static int drm_minor_register(struct drm_device *dev, 
> > enum drm_minor_type type)
> > if (minor->type == DRM_MINOR_ACCEL) {
> > accel_minor_replace(minor, minor->index);
> > } else {
> > -   spin_lock_irqsave(_minor_lock, flags);
> > -   idr_replace(_minors_idr, minor, minor->index);
> > -   spin_unlock_irqrestore(_minor_lock, flags);
> > +   entry = xa_store(_minors_xa, minor->index, minor, 
> > GFP_KERNEL);
> > +   if (xa_is_err(entry)) {
> > +   ret = xa_err(entry);
> > +   goto err_debugfs;
> > +   }
> > +   WARN_ON(entry);
> [JZ] would WARN_ON(entry != minor)be better?

We expect NULL here.
The same one that was previously stored here ⌄⌄⌄
r = xa_alloc(_minors_xa, >index, NULL, DRM_EXTENDED_MINOR_LIMIT, 
GFP_KERNEL);
in drm_minor_alloc.

> > }
> > DRM_DEBUG("new minor registered %d\n", minor->index);
> > @@ -206,20 +199,16 @@ static int drm_minor_register(struct drm_device *dev, 
> > enum drm_minor_type type)
> >   static void drm_minor_unregister(struct drm_device *dev, enum 
> > drm_minor_type type)
> >   {
> > struct drm_minor *minor;
> > -   unsigned long flags;
> > minor = *drm_minor_get_slot(dev, type);
> > if (!minor || !device_is_registered(minor->kdev))

Re: [PATCH v6 1/4] drm: Use XArray instead of IDR for minors

2023-08-25 Thread James Zhu 


On 2023-07-24 17:14, Michał Winiarski wrote:

IDR is deprecated, and since XArray manages its own state with internal
locking, it simplifies the locking on DRM side.
Additionally, don't use the IRQ-safe variant, since operating on drm
minor is not done in IRQ context.

Signed-off-by: Michał Winiarski
Suggested-by: Matthew Wilcox
---
  drivers/gpu/drm/drm_drv.c | 63 ---
  1 file changed, 25 insertions(+), 38 deletions(-)

diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 3eda026ffac6..3faecb01186f 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -34,6 +34,7 @@
  #include 
  #include 
  #include 
+#include 
  
  #include 

  #include 
@@ -54,8 +55,7 @@ MODULE_AUTHOR("Gareth Hughes, Leif Delgass, José Fonseca, Jon 
Smirl");
  MODULE_DESCRIPTION("DRM shared core routines");
  MODULE_LICENSE("GPL and additional rights");
  
-static DEFINE_SPINLOCK(drm_minor_lock);

-static struct idr drm_minors_idr;
+static DEFINE_XARRAY_ALLOC(drm_minors_xa);
  
  /*

   * If the drm core fails to init for whatever reason,
@@ -101,26 +101,23 @@ static struct drm_minor **drm_minor_get_slot(struct 
drm_device *dev,
  static void drm_minor_alloc_release(struct drm_device *dev, void *data)
  {
struct drm_minor *minor = data;
-   unsigned long flags;
  
  	WARN_ON(dev != minor->dev);
  
  	put_device(minor->kdev);
  
-	if (minor->type == DRM_MINOR_ACCEL) {

+   if (minor->type == DRM_MINOR_ACCEL)
accel_minor_remove(minor->index);
-   } else {
-   spin_lock_irqsave(_minor_lock, flags);
-   idr_remove(_minors_idr, minor->index);
-   spin_unlock_irqrestore(_minor_lock, flags);
-   }
+   else
+   xa_erase(_minors_xa, minor->index);
  }
  
+#define DRM_MINOR_LIMIT(t) ({ typeof(t) _t = (t); XA_LIMIT(64 * _t, 64 * _t + 63); })

+
  static int drm_minor_alloc(struct drm_device *dev, enum drm_minor_type type)
  {
struct drm_minor *minor;
-   unsigned long flags;
-   int r;
+   int index, r;
  
  	minor = drmm_kzalloc(dev, sizeof(*minor), GFP_KERNEL);

if (!minor)
@@ -129,24 +126,17 @@ static int drm_minor_alloc(struct drm_device *dev, enum 
drm_minor_type type)
minor->type = type;
minor->dev = dev;
  
-	idr_preload(GFP_KERNEL);

if (type == DRM_MINOR_ACCEL) {
r = accel_minor_alloc();
+   index = r;
} else {
-   spin_lock_irqsave(_minor_lock, flags);
-   r = idr_alloc(_minors_idr,
-   NULL,
-   64 * type,
-   64 * (type + 1),
-   GFP_NOWAIT);
-   spin_unlock_irqrestore(_minor_lock, flags);
+   r = xa_alloc(_minors_xa, , NULL, 
DRM_MINOR_LIMIT(type), GFP_KERNEL);
}
-   idr_preload_end();
  
  	if (r < 0)

return r;
  
-	minor->index = r;

+   minor->index = index;
  
  	r = drmm_add_action_or_reset(dev, drm_minor_alloc_release, minor);

if (r)
@@ -163,7 +153,7 @@ static int drm_minor_alloc(struct drm_device *dev, enum 
drm_minor_type type)
  static int drm_minor_register(struct drm_device *dev, enum drm_minor_type 
type)
  {
struct drm_minor *minor;
-   unsigned long flags;
+   void *entry;
int ret;
  
  	DRM_DEBUG("\n");

@@ -190,9 +180,12 @@ static int drm_minor_register(struct drm_device *dev, enum 
drm_minor_type type)
if (minor->type == DRM_MINOR_ACCEL) {
accel_minor_replace(minor, minor->index);
} else {
-   spin_lock_irqsave(_minor_lock, flags);
-   idr_replace(_minors_idr, minor, minor->index);
-   spin_unlock_irqrestore(_minor_lock, flags);
+   entry = xa_store(_minors_xa, minor->index, minor, 
GFP_KERNEL);
+   if (xa_is_err(entry)) {
+   ret = xa_err(entry);
+   goto err_debugfs;
+   }
+   WARN_ON(entry);

[JZ] would WARN_ON(entry != minor)be better?

}
  
  	DRM_DEBUG("new minor registered %d\n", minor->index);

@@ -206,20 +199,16 @@ static int drm_minor_register(struct drm_device *dev, 
enum drm_minor_type type)
  static void drm_minor_unregister(struct drm_device *dev, enum drm_minor_type 
type)
  {
struct drm_minor *minor;
-   unsigned long flags;
  
  	minor = *drm_minor_get_slot(dev, type);

if (!minor || !device_is_registered(minor->kdev))
return;
  
  	/* replace @minor with NULL so lookups will fail from now on */

-   if (minor->type == DRM_MINOR_ACCEL) {
+   if (minor->type == DRM_MINOR_ACCEL)
accel_minor_replace(NULL, minor->index);
-   } else {
-   spin_lock_irqsave(_minor_lock, flags);
-   idr_replace(_minors_idr, NULL, minor->index);
-   spin_unlock_irqrestore(_minor_lock, flags);
-   }
+   else
+