Convert greybus/uart.c from IDR to XArray. The abstract data type XArray is more memory-efficient, parallelisable, and cache friendly. It takes advantage of RCU to perform lookups without locking. Furthermore, IDR is deprecated because XArray has a better (cleaner and more consistent) API.
Signed-off-by: Fabio M. De Francesco fmdefrancesco@gmail.com ---
v2->v3: Fix some issues according to a review by Alex Elder elder@ieee.org
v1->v2: Fix an issue found by the kernel test robot. It is due to passing to xa_*lock() the same old mutex that IDR used with the previous version of the code.
drivers/staging/greybus/uart.c | 34 ++++++++++++++++------------------ 1 file changed, 16 insertions(+), 18 deletions(-)
diff --git a/drivers/staging/greybus/uart.c b/drivers/staging/greybus/uart.c index 73f01ed1e5b7..815156c88005 100644 --- a/drivers/staging/greybus/uart.c +++ b/drivers/staging/greybus/uart.c @@ -22,7 +22,7 @@ #include <linux/serial.h> #include <linux/tty_driver.h> #include <linux/tty_flip.h> -#include <linux/idr.h> +#include <linux/xarray.h> #include <linux/fs.h> #include <linux/kdev_t.h> #include <linux/kfifo.h> @@ -32,8 +32,9 @@
#include "gbphy.h"
-#define GB_NUM_MINORS 16 /* 16 is more than enough */ -#define GB_NAME "ttyGB" +#define GB_NUM_MINORS 16 /* 16 is more than enough */ +#define GB_RANGE_MINORS XA_LIMIT(0, GB_NUM_MINORS) +#define GB_NAME "ttyGB"
#define GB_UART_WRITE_FIFO_SIZE PAGE_SIZE #define GB_UART_WRITE_ROOM_MARGIN 1 /* leave some space in fifo */ @@ -67,8 +68,7 @@ struct gb_tty { };
static struct tty_driver *gb_tty_driver; -static DEFINE_IDR(tty_minors); -static DEFINE_MUTEX(table_lock); +static DEFINE_XARRAY(tty_minors);
static int gb_uart_receive_data_handler(struct gb_operation *op) { @@ -341,8 +341,8 @@ static struct gb_tty *get_gb_by_minor(unsigned int minor) { struct gb_tty *gb_tty;
- mutex_lock(&table_lock); - gb_tty = idr_find(&tty_minors, minor); + xa_lock(&tty_minors); + gb_tty = xa_load(&tty_minors, minor); if (gb_tty) { mutex_lock(&gb_tty->mutex); if (gb_tty->disconnected) { @@ -353,19 +353,19 @@ static struct gb_tty *get_gb_by_minor(unsigned int minor) mutex_unlock(&gb_tty->mutex); } } - mutex_unlock(&table_lock); + xa_unlock(&tty_minors); return gb_tty; }
static int alloc_minor(struct gb_tty *gb_tty) { int minor; + int ret;
- mutex_lock(&table_lock); - minor = idr_alloc(&tty_minors, gb_tty, 0, GB_NUM_MINORS, GFP_KERNEL); - mutex_unlock(&table_lock); - if (minor >= 0) - gb_tty->minor = minor; + ret = xa_alloc(&tty_minors, &minor, gb_tty, GB_RANGE_MINORS, GFP_KERNEL); + if (ret) + return ret; + gb_tty->minor = minor; return minor; }
@@ -374,9 +374,7 @@ static void release_minor(struct gb_tty *gb_tty) int minor = gb_tty->minor;
gb_tty->minor = 0; /* Maybe should use an invalid value instead */ - mutex_lock(&table_lock); - idr_remove(&tty_minors, minor); - mutex_unlock(&table_lock); + xa_erase(&tty_minors, minor); }
static int gb_tty_install(struct tty_driver *driver, struct tty_struct *tty) @@ -837,7 +835,7 @@ static int gb_uart_probe(struct gbphy_device *gbphy_dev,
minor = alloc_minor(gb_tty); if (minor < 0) { - if (minor == -ENOSPC) { + if (minor == -EBUSY) { dev_err(&gbphy_dev->dev, "no more free minor numbers\n"); retval = -ENODEV; @@ -982,7 +980,7 @@ static void gb_tty_exit(void) { tty_unregister_driver(gb_tty_driver); put_tty_driver(gb_tty_driver); - idr_destroy(&tty_minors); + xa_destroy(&tty_minors); }
static const struct gbphy_device_id gb_uart_id_table[] = {
On 8/16/21 2:50 PM, Fabio M. De Francesco wrote:
Convert greybus/uart.c from IDR to XArray. The abstract data type XArray is more memory-efficient, parallelisable, and cache friendly. It takes advantage of RCU to perform lookups without locking. Furthermore, IDR is deprecated because XArray has a better (cleaner and more consistent) API.
Signed-off-by: Fabio M. De Francesco fmdefrancesco@gmail.com
I have one more comment, below. Generally, I don't think it is important to make this change, but I think it's fine to switch to the newer XArray interface. The result is a little simpler.
v2->v3: Fix some issues according to a review by Alex Elder elder@ieee.org
v1->v2: Fix an issue found by the kernel test robot. It is due to passing to xa_*lock() the same old mutex that IDR used with the previous version of the code.
drivers/staging/greybus/uart.c | 34 ++++++++++++++++------------------ 1 file changed, 16 insertions(+), 18 deletions(-)
diff --git a/drivers/staging/greybus/uart.c b/drivers/staging/greybus/uart.c index 73f01ed1e5b7..815156c88005 100644 --- a/drivers/staging/greybus/uart.c +++ b/drivers/staging/greybus/uart.c @@ -22,7 +22,7 @@ #include <linux/serial.h> #include <linux/tty_driver.h> #include <linux/tty_flip.h> -#include <linux/idr.h> +#include <linux/xarray.h> #include <linux/fs.h> #include <linux/kdev_t.h> #include <linux/kfifo.h> @@ -32,8 +32,9 @@ #include "gbphy.h" -#define GB_NUM_MINORS 16 /* 16 is more than enough */ -#define GB_NAME "ttyGB" +#define GB_NUM_MINORS 16 /* 16 is more than enough */ +#define GB_RANGE_MINORS XA_LIMIT(0, GB_NUM_MINORS) +#define GB_NAME "ttyGB" #define GB_UART_WRITE_FIFO_SIZE PAGE_SIZE #define GB_UART_WRITE_ROOM_MARGIN 1 /* leave some space in fifo */ @@ -67,8 +68,7 @@ struct gb_tty { }; static struct tty_driver *gb_tty_driver; -static DEFINE_IDR(tty_minors); -static DEFINE_MUTEX(table_lock); +static DEFINE_XARRAY(tty_minors); static int gb_uart_receive_data_handler(struct gb_operation *op) { @@ -341,8 +341,8 @@ static struct gb_tty *get_gb_by_minor(unsigned int minor) { struct gb_tty *gb_tty;
- mutex_lock(&table_lock);
- gb_tty = idr_find(&tty_minors, minor);
- xa_lock(&tty_minors);
I'm basically new to using the XArray interface, but I
don't think you really need the xa_lock()/xa_unlock()
calls here. You are not relying on reference counting
to control when the allocated minor device numbers are
freed, so I'm pretty sure you can simply call xa_load()
to look up the gb_tty for the given minor device.
But please don't only take my word for it; investigate
it for yourself, and if needed ask others about it so
you're confident it's correct. There is no harm in taking the lock, but if it's not needed, it would be nice to avoid it.
If you conclude the locks are necessary, just say so, and explain why, and I'll probably just accept it. Otherwise, please explain why you are sure they are not needed when you send version 4. Thank you.
-Alex
- gb_tty = xa_load(&tty_minors, minor); if (gb_tty) { mutex_lock(&gb_tty->mutex); if (gb_tty->disconnected) {
@@ -353,19 +353,19 @@ static struct gb_tty *get_gb_by_minor(unsigned int minor) mutex_unlock(&gb_tty->mutex); } }
- mutex_unlock(&table_lock);
- xa_unlock(&tty_minors); return gb_tty;
} static int alloc_minor(struct gb_tty *gb_tty) { int minor;
- int ret;
- mutex_lock(&table_lock);
- minor = idr_alloc(&tty_minors, gb_tty, 0, GB_NUM_MINORS, GFP_KERNEL);
- mutex_unlock(&table_lock);
- if (minor >= 0)
gb_tty->minor = minor;
- ret = xa_alloc(&tty_minors, &minor, gb_tty, GB_RANGE_MINORS, GFP_KERNEL);
- if (ret)
return ret;- gb_tty->minor = minor; return minor;
} @@ -374,9 +374,7 @@ static void release_minor(struct gb_tty *gb_tty) int minor = gb_tty->minor; gb_tty->minor = 0; /* Maybe should use an invalid value instead */
- mutex_lock(&table_lock);
- idr_remove(&tty_minors, minor);
- mutex_unlock(&table_lock);
- xa_erase(&tty_minors, minor);
} static int gb_tty_install(struct tty_driver *driver, struct tty_struct *tty) @@ -837,7 +835,7 @@ static int gb_uart_probe(struct gbphy_device *gbphy_dev, minor = alloc_minor(gb_tty); if (minor < 0) {
if (minor == -ENOSPC) {
if (minor == -EBUSY) { dev_err(&gbphy_dev->dev, "no more free minor numbers\n"); retval = -ENODEV;@@ -982,7 +980,7 @@ static void gb_tty_exit(void) { tty_unregister_driver(gb_tty_driver); put_tty_driver(gb_tty_driver);
- idr_destroy(&tty_minors);
- xa_destroy(&tty_minors);
} static const struct gbphy_device_id gb_uart_id_table[] = {
On Saturday, August 28, 2021 5:43:49 PM CEST Alex Elder wrote:
On 8/16/21 2:50 PM, Fabio M. De Francesco wrote:
Convert greybus/uart.c from IDR to XArray. The abstract data type XArray is more memory-efficient, parallelisable, and cache friendly. It takes advantage of RCU to perform lookups without locking. Furthermore, IDR is deprecated because XArray has a better (cleaner and more consistent) API.
Signed-off-by: Fabio M. De Francesco fmdefrancesco@gmail.com
I have one more comment, below. Generally, I don't think it is important to make this change, but I think it's fine to switch to the newer XArray interface. The result is a little simpler.
I agree that the result of using XArray is a little simpler and readable. As far as performance is regarded (memory-efficiency, cache friendliness, parallelization improvements) I have to take for true the words of Matthew W.. Some time ago I did a similar conversion for staging/unisys/visorhba after discussing with him on IRC; he confirmed that the driver would have got several benefits. This is why I decided to do this work on staging/greybus too.
I cannot affirm the same for IDA to XArray conversions, since IDA are relatively lighter and efficient than IDR. Unfortunately, I cannot profile such conversions in order to prove/disprove they *really* gain on execution time and/or memory footprint.
[]
static int gb_uart_receive_data_handler(struct gb_operation *op) { @@ -341,8 +341,8 @@ static struct gb_tty *get_gb_by_minor(unsigned int minor) { struct gb_tty *gb_tty;
- mutex_lock(&table_lock);
- gb_tty = idr_find(&tty_minors, minor);
- xa_lock(&tty_minors);
I'm basically new to using the XArray interface, but I don't think you really need the xa_lock()/xa_unlock() calls here. You are not relying on reference counting to control when the allocated minor device numbers are freed, so I'm pretty sure you can simply call xa_load() to look up the gb_tty for the given minor device.
I haven't yet had time to understand how driver works. However, I can attempt a response mostly due to logic than to a real knowledge of how drivers work...
(1) I see that alloc_minor is called at "probe" (that I suppose it means the the kernel "feels" that a new device has been added and so it should initialize it somehow and make it ready to operate properly - I hope I'm not too far from the truth :)).
(2) I see that xa_alloc() finds an *unused* identifier and, if it succeeds, that identifier is used as the "minor". So, we have one minor per device and that the same minor cannot be re-assigned to other devices. It also should mean that there's no need for reference counting because that "minor" is not shared.
(3) If the logic above is sound, we have a 1:1 correspondence between minors and devices (max 16 gb_tty's) and therefore we don't need to lock tty_minors because concurrent code passes different minors to xa_load() which always returns different gb_tty's.
If the above argument is wrong I think I should read a book on device drivers for the first time. I have Greg's but I haven't yet opened it for reading :)
Thanks,
Fabio
But please don't only take my word for it; investigate it for yourself, and if needed ask others about it so you're confident it's correct. There is no harm in taking the lock, but if it's not needed, it would be nice to avoid it.
If you conclude the locks are necessary, just say so, and explain why, and I'll probably just accept it. Otherwise, please explain why you are sure they are not needed when you send version 4. Thank you.
-Alex
- gb_tty = xa_load(&tty_minors, minor); if (gb_tty) { mutex_lock(&gb_tty->mutex); if (gb_tty->disconnected) {
@@ -353,19 +353,19 @@ static struct gb_tty *get_gb_by_minor(unsigned int minor) mutex_unlock(&gb_tty->mutex); } }
- mutex_unlock(&table_lock);
- xa_unlock(&tty_minors); return gb_tty;
}
On Mon, Aug 16, 2021 at 09:50:00PM +0200, Fabio M. De Francesco wrote:
Convert greybus/uart.c from IDR to XArray. The abstract data type XArray is more memory-efficient, parallelisable, and cache friendly. It takes advantage of RCU to perform lookups without locking. Furthermore, IDR is deprecated because XArray has a better (cleaner and more consistent) API.
Signed-off-by: Fabio M. De Francesco fmdefrancesco@gmail.com
v2->v3: Fix some issues according to a review by Alex Elder elder@ieee.org
v1->v2: Fix an issue found by the kernel test robot. It is due to passing to xa_*lock() the same old mutex that IDR used with the previous version of the code.
drivers/staging/greybus/uart.c | 34 ++++++++++++++++------------------ 1 file changed, 16 insertions(+), 18 deletions(-)
diff --git a/drivers/staging/greybus/uart.c b/drivers/staging/greybus/uart.c index 73f01ed1e5b7..815156c88005 100644 --- a/drivers/staging/greybus/uart.c +++ b/drivers/staging/greybus/uart.c @@ -22,7 +22,7 @@ #include <linux/serial.h> #include <linux/tty_driver.h> #include <linux/tty_flip.h> -#include <linux/idr.h> +#include <linux/xarray.h> #include <linux/fs.h> #include <linux/kdev_t.h> #include <linux/kfifo.h> @@ -32,8 +32,9 @@ #include "gbphy.h" -#define GB_NUM_MINORS 16 /* 16 is more than enough */ -#define GB_NAME "ttyGB" +#define GB_NUM_MINORS 16 /* 16 is more than enough */ +#define GB_RANGE_MINORS XA_LIMIT(0, GB_NUM_MINORS) +#define GB_NAME "ttyGB" #define GB_UART_WRITE_FIFO_SIZE PAGE_SIZE #define GB_UART_WRITE_ROOM_MARGIN 1 /* leave some space in fifo */ @@ -67,8 +68,7 @@ struct gb_tty { }; static struct tty_driver *gb_tty_driver; -static DEFINE_IDR(tty_minors); -static DEFINE_MUTEX(table_lock); +static DEFINE_XARRAY(tty_minors); static int gb_uart_receive_data_handler(struct gb_operation *op) { @@ -341,8 +341,8 @@ static struct gb_tty *get_gb_by_minor(unsigned int minor) { struct gb_tty *gb_tty;
- mutex_lock(&table_lock);
- gb_tty = idr_find(&tty_minors, minor);
- xa_lock(&tty_minors);
- gb_tty = xa_load(&tty_minors, minor); if (gb_tty) { mutex_lock(&gb_tty->mutex); if (gb_tty->disconnected) {
@@ -353,19 +353,19 @@ static struct gb_tty *get_gb_by_minor(unsigned int minor) mutex_unlock(&gb_tty->mutex); } }
- mutex_unlock(&table_lock);
- xa_unlock(&tty_minors); return gb_tty;
} static int alloc_minor(struct gb_tty *gb_tty) { int minor;
- int ret;
- mutex_lock(&table_lock);
- minor = idr_alloc(&tty_minors, gb_tty, 0, GB_NUM_MINORS, GFP_KERNEL);
- mutex_unlock(&table_lock);
- if (minor >= 0)
gb_tty->minor = minor;
- ret = xa_alloc(&tty_minors, &minor, gb_tty, GB_RANGE_MINORS, GFP_KERNEL);
- if (ret)
return ret;- gb_tty->minor = minor; return minor;
} @@ -374,9 +374,7 @@ static void release_minor(struct gb_tty *gb_tty) int minor = gb_tty->minor; gb_tty->minor = 0; /* Maybe should use an invalid value instead */
- mutex_lock(&table_lock);
- idr_remove(&tty_minors, minor);
- mutex_unlock(&table_lock);
- xa_erase(&tty_minors, minor);
} static int gb_tty_install(struct tty_driver *driver, struct tty_struct *tty) @@ -837,7 +835,7 @@ static int gb_uart_probe(struct gbphy_device *gbphy_dev, minor = alloc_minor(gb_tty); if (minor < 0) {
if (minor == -ENOSPC) {
if (minor == -EBUSY) { dev_err(&gbphy_dev->dev, "no more free minor numbers\n"); retval = -ENODEV;@@ -982,7 +980,7 @@ static void gb_tty_exit(void) { tty_unregister_driver(gb_tty_driver); put_tty_driver(gb_tty_driver);
- idr_destroy(&tty_minors);
- xa_destroy(&tty_minors);
} static const struct gbphy_device_id gb_uart_id_table[] = { -- 2.32.0
I'm going to drop this from my review queue as I do not see the need to do this type of conversion at this point in time. Perhaps if "real" drivers in the kernel get converted over to this new api we should also do it here, but for now, there's much more "real" issues with the greybus drivers that should be done that is keeping them from being merged into the main part of the kernel tree, that the churn here is not necessary at this point in time (especially if you have not tested this.)
thanks,
greg k-h
-
Alex Elder -
Fabio M. De Francesco -
Greg Kroah-Hartman