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.
Reported-by: kernel test robot lkp@intel.com Signed-off-by: Fabio M. De Francesco fmdefrancesco@gmail.com ---
v1->v2: Fixed an issue found by the kernel test robot. It was 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 | 29 ++++++++++++++--------------- 1 file changed, 14 insertions(+), 15 deletions(-)
diff --git a/drivers/staging/greybus/uart.c b/drivers/staging/greybus/uart.c index 73f01ed1e5b7..5bf993e40f84 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> @@ -33,6 +33,7 @@ #include "gbphy.h"
#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 @@ -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) { @@ -77,6 +77,7 @@ static int gb_uart_receive_data_handler(struct gb_operation *op) struct tty_port *port = &gb_tty->port; struct gb_message *request = op->request; struct gb_uart_recv_data_request *receive_data; + u16 recv_data_size; int count; unsigned long tty_flags = TTY_NORMAL; @@ -341,8 +342,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 +354,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 +375,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) @@ -982,7 +981,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/14/21 1:11 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.
I haven't verified the use of the new API (yet) but I have a few comments on your patch, below.
-Alex
Reported-by: kernel test robot lkp@intel.com Signed-off-by: Fabio M. De Francesco fmdefrancesco@gmail.com
I'm not sure I'm right about this... But the actual change you're making has nothing to do with what the Intel test robot reported. I personally find the "Reported-by" here a little misleading, but maybe the "Link" line that gets added will provide explanation.
Anyway, unless someone else contradicts/corrects me, I'd rather not have the "Reported-by" here (despite wanting to provide much credit to lkp@intel.com...).
v1->v2: Fixed an issue found by the kernel test robot. It was 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 | 29 ++++++++++++++--------------- 1 file changed, 14 insertions(+), 15 deletions(-)
diff --git a/drivers/staging/greybus/uart.c b/drivers/staging/greybus/uart.c index 73f01ed1e5b7..5bf993e40f84 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> @@ -33,6 +33,7 @@ #include "gbphy.h" #define GB_NUM_MINORS 16 /* 16 is more than enough */ +#define GB_RANGE_MINORS XA_LIMIT(0, GB_NUM_MINORS) #define GB_NAME "ttyGB"
Please align the right-hand side of all three definitions here.
#define GB_UART_WRITE_FIFO_SIZE PAGE_SIZE @@ -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) { @@ -77,6 +77,7 @@ static int gb_uart_receive_data_handler(struct gb_operation *op) struct tty_port *port = &gb_tty->port; struct gb_message *request = op->request; struct gb_uart_recv_data_request *receive_data;
Please do not add a blank line amid the local variable definitions.
I'm not sure it checks for this, but you should run your patch through "checkpatch.pl" before you send it. E.g.: ./scripts/checkpatch.pl idr_to_xarray.patch
The error reported in the build of your first version of this patch makes me think you might not have test- built the code. I don't know if that's the case, but (at least) building the code is expected before you submit a patch for review.
u16 recv_data_size; int count; unsigned long tty_flags = TTY_NORMAL; @@ -341,8 +342,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 +354,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;
The caller of alloc_minor() (gb_uart_probe()) checks the return value, and if it's -ENOSPC it logs a device error indicating there are no remaining free device minor numbers. For xa_alloc() this is indicated by returning -EBUSY. Please update the caller to print the error message based on the updated error code.
- gb_tty->minor = minor; return minor; }
@@ -374,9 +375,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) @@ -982,7 +981,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 Mon, Aug 16, 2021 at 09:46:08AM -0500, Alex Elder wrote:
On 8/14/21 1:11 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.
I haven't verified the use of the new API (yet) but I have a few comments on your patch, below.
-AlexReported-by: kernel test robot lkp@intel.com Signed-off-by: Fabio M. De Francesco fmdefrancesco@gmail.com
I'm not sure I'm right about this... But the actual change you're making has nothing to do with what the Intel test robot reported. I personally find the "Reported-by" here a little misleading, but maybe the "Link" line that gets added will provide explanation.
Anyway, unless someone else contradicts/corrects me, I'd rather not have the "Reported-by" here (despite wanting to provide much credit to lkp@intel.com...).
You are correct, "Reported-by:" does not make sense here.
thanks,
greg k-h
On Mon, Aug 16, 2021 at 05:01:02PM +0200, Greg Kroah-Hartman wrote:
On Mon, Aug 16, 2021 at 09:46:08AM -0500, Alex Elder wrote:
On 8/14/21 1:11 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.
I haven't verified the use of the new API (yet) but I have a few comments on your patch, below.
-AlexReported-by: kernel test robot lkp@intel.com Signed-off-by: Fabio M. De Francesco fmdefrancesco@gmail.com
I'm not sure I'm right about this... But the actual change you're making has nothing to do with what the Intel test robot reported. I personally find the "Reported-by" here a little misleading, but maybe the "Link" line that gets added will provide explanation.
Anyway, unless someone else contradicts/corrects me, I'd rather not have the "Reported-by" here (despite wanting to provide much credit to lkp@intel.com...).
You are correct, "Reported-by:" does not make sense here.
There should be a Fixes-from: tag for bugs found in review (not style issues) but when I suggest it then people just say to use the Reported-by tag.
regards, dan carpenter
On 8/16/21 10:06 AM, Dan Carpenter wrote:
On Mon, Aug 16, 2021 at 05:01:02PM +0200, Greg Kroah-Hartman wrote:
On Mon, Aug 16, 2021 at 09:46:08AM -0500, Alex Elder wrote:
On 8/14/21 1:11 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.
I haven't verified the use of the new API (yet) but I have a few comments on your patch, below.
-AlexReported-by: kernel test robot lkp@intel.com Signed-off-by: Fabio M. De Francesco fmdefrancesco@gmail.com
I'm not sure I'm right about this... But the actual change you're making has nothing to do with what the Intel test robot reported. I personally find the "Reported-by" here a little misleading, but maybe the "Link" line that gets added will provide explanation.
Anyway, unless someone else contradicts/corrects me, I'd rather not have the "Reported-by" here (despite wanting to provide much credit to lkp@intel.com...).
You are correct, "Reported-by:" does not make sense here.
There should be a Fixes-from: tag for bugs found in review (not style issues) but when I suggest it then people just say to use the Reported-by tag.
I think things caught during review aren't normally worthy of specific mention in the commit message (though maybe in the non-committed part under "---"). I mean, that's what review is for. And in the case of what lkp@intel.com does, that's effectively a technical aspect of "review."
So I don't think "Fixes-from" (whatever that means) or "Reported-by" make sense for this type of update.
-Alex
regards, dan carpenter
greybus-dev mailing list greybus-dev@lists.linaro.org https://lists.linaro.org/mailman/listinfo/greybus-dev
On Mon, Aug 16, 2021 at 10:10:04AM -0500, Alex Elder wrote:
On 8/16/21 10:06 AM, Dan Carpenter wrote:
On Mon, Aug 16, 2021 at 05:01:02PM +0200, Greg Kroah-Hartman wrote:
On Mon, Aug 16, 2021 at 09:46:08AM -0500, Alex Elder wrote:
On 8/14/21 1:11 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.
I haven't verified the use of the new API (yet) but I have a few comments on your patch, below.
-AlexReported-by: kernel test robot lkp@intel.com Signed-off-by: Fabio M. De Francesco fmdefrancesco@gmail.com
I'm not sure I'm right about this... But the actual change you're making has nothing to do with what the Intel test robot reported. I personally find the "Reported-by" here a little misleading, but maybe the "Link" line that gets added will provide explanation.
Anyway, unless someone else contradicts/corrects me, I'd rather not have the "Reported-by" here (despite wanting to provide much credit to lkp@intel.com...).
You are correct, "Reported-by:" does not make sense here.
There should be a Fixes-from: tag for bugs found in review (not style issues) but when I suggest it then people just say to use the Reported-by tag.
I think things caught during review aren't normally worthy of specific mention in the commit message (though maybe in the non-committed part under "---"). I mean, that's what review is for. And in the case of what lkp@intel.com does, that's effectively a technical aspect of "review."
I'm not talking about stuff like intending or naming schemes, I'm talking about real bugs like missing error codes or NULL dereferences. People do count tags so we might as well add them for worthwhile behavior.
So I don't think "Fixes-from" (whatever that means) or "Reported-by" make sense for this type of update.
Earlier today I forwarded a kbuild Smatch warning where someone had used "sizeof(0)" instead of "0" but because the patch was already applied, that means I got Reported-by credit. If the kbuild-bot could have reported the bug before the networking people applied it that's more valuable but I get less credit. It's a perverse incentive.
Also I sort of don't like the Reviewed-by tag. I see a lot of people adding Reviewed-by but I've never seen them point out a bug during the review process so that seems pretty worthless. But Fixes-from means that person knows what they're talking about.
regards, dan carpenter
On Mon, Aug 16, 2021 at 09:36:39PM +0300, Dan Carpenter wrote:
On Mon, Aug 16, 2021 at 10:10:04AM -0500, Alex Elder wrote:
On 8/16/21 10:06 AM, Dan Carpenter wrote:
On Mon, Aug 16, 2021 at 05:01:02PM +0200, Greg Kroah-Hartman wrote:
On Mon, Aug 16, 2021 at 09:46:08AM -0500, Alex Elder wrote:
On 8/14/21 1:11 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.
I haven't verified the use of the new API (yet) but I have a few comments on your patch, below.
-AlexReported-by: kernel test robot lkp@intel.com Signed-off-by: Fabio M. De Francesco fmdefrancesco@gmail.com
I'm not sure I'm right about this... But the actual change you're making has nothing to do with what the Intel test robot reported. I personally find the "Reported-by" here a little misleading, but maybe the "Link" line that gets added will provide explanation.
Anyway, unless someone else contradicts/corrects me, I'd rather not have the "Reported-by" here (despite wanting to provide much credit to lkp@intel.com...).
You are correct, "Reported-by:" does not make sense here.
There should be a Fixes-from: tag for bugs found in review (not style issues) but when I suggest it then people just say to use the Reported-by tag.
I think things caught during review aren't normally worthy of specific mention in the commit message (though maybe in the non-committed part under "---"). I mean, that's what review is for. And in the case of what lkp@intel.com does, that's effectively a technical aspect of "review."
I'm not talking about stuff like intending or naming schemes, I'm
*indenting*... *sigh*.
regards, dan carpenter
On 8/16/21 1:36 PM, Dan Carpenter wrote:
There should be a Fixes-from: tag for bugs found in review (not style issues) but when I suggest it then people just say to use the Reported-by tag.
I think things caught during review aren't normally worthy of specific mention in the commit message (though maybe in the non-committed part under "---"). I mean, that's what review is for. And in the case of whatlkp@intel.com does, that's effectively a technical aspect of "review."
I'm not talking about stuff like intending or naming schemes, I'm talking about real bugs like missing error codes or NULL dereferences. People do count tags so we might as well add them for worthwhile behavior.
So you're saying that things caught during review *should* be given credit, as opposed to acknowledging the credit for catching it only when the bug slips by the reviewers, caught after commit.
I understand that, and I get your point about the incentives (which take the form of tags with acknowledgement).
As I indicated earlier, I'm all for showering credit on everyone that helps. But I still think doing so for input taken during the review phase is too much, and full of fuzzy cases (how do you judge whether a suggestion is worth acknowledging?).
I think what you do with Smatch is outstanding, and you deserve a lot of credit for it. But like checkpatch.pl, it would be even better if people used it to catch things *before* they ever went out for review. That option would give *no* credit to Smatch for catching problems early. Yet catching issues as early as possible is a good thing. Should we acknowledge checkpatch.pl when it tells us to fix something it finds; if so, which of them?
So I don't think "Fixes-from" (whatever that means) or "Reported-by" make sense for this type of update.
Earlier today I forwarded a kbuild Smatch warning where someone had used "sizeof(0)" instead of "0" but because the patch was already applied, that means I got Reported-by credit. If the kbuild-bot could have reported the bug before the networking people applied it that's more valuable but I get less credit. It's a perverse incentive.
It's a perverse incentive for you as Smatch developer. But I think the better place to put an incentive is on getting people to avoid sending patches at all until they have used tools available to automatically find issues before they get out for review.
Also I sort of don't like the Reviewed-by tag. I see a lot of people adding Reviewed-by but I've never seen them point out a bug during the review process so that seems pretty worthless. But Fixes-from means that person knows what they're talking about.
That's not a problem with Reviewed-by, it's a problem with people misusing it. Are you suggesting that "Fixes-from" would be applied by the developer, not reviewer? Regardless, Reviewed-by is *supposed* to carry meaning. "Documentation/process/submitting-patches.rst" has a section that describes what the "Reviewer's statement of oversight" represents.
I think it would be nice to recognize review feedback. It's actually more valuable than the summary statement "I have reviewed this and find it acceptable." But I don't believe adding new acknowledgement tags is a good way to do it.
-Alex
regards, dan carpenter
I'm about to leave for a few days of PTO...
Under my scheme checkpatch.pl is not a fix. We would only credit people where the code would get a Fixes tag. Complaining about style choices is its own reward and we don't need to encourage it.
And it's not really about me, either... I'm happy with amount of credit I get. :P LOL. Plus even if everyone used Smatch, I'm always writing new code.
Anyway, I hear you but disagree. :) I'm off for a bit.
regards, dan carpenter
Hi Alex,
On Monday, August 16, 2021 4:46:08 PM CEST Alex Elder wrote:
On 8/14/21 1:11 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.
I haven't verified the use of the new API (yet) but I have a few comments on your patch, below.
-AlexReported-by: kernel test robot lkp@intel.com Signed-off-by: Fabio M. De Francesco fmdefrancesco@gmail.com
I'm not sure I'm right about this... But the actual change you're making has nothing to do with what the Intel test robot reported. I personally find the "Reported-by" here a little misleading, but maybe the "Link" line that gets added will provide explanation. Anyway, unless someone else contradicts/corrects me, I'd rather not have the "Reported-by" here (despite wanting to provide much credit to lkp@intel.com...).
I'm going to remove that tag and send a v3. I too had doubts about using it in this case and I was about to omit it (please consider I have just a few months of experience with kernel hacking and, as far as I can remember, I haven't had more than one other occasion to deal with the kernel test robot).
Now I think I understand when I should use the Reported-by tag and I'll use it accordingly to what you and the others explained in this thread.
v1->v2: Fixed an issue found by the kernel test robot. It was 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 | 29 ++++++++++++++--------------- 1 file changed, 14 insertions(+), 15 deletions(-)
diff --git a/drivers/staging/greybus/uart.c b/drivers/staging/greybus/
uart.c
index 73f01ed1e5b7..5bf993e40f84 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>
@@ -33,6 +33,7 @@
#include "gbphy.h" #define GB_NUM_MINORS 16 /* 16 is more than enough */
+#define GB_RANGE_MINORS XA_LIMIT(0, GB_NUM_MINORS)
#define GB_NAME "ttyGB"
Please align the right-hand side of all three definitions here.
Yes, sure.
#define GB_UART_WRITE_FIFO_SIZE PAGE_SIZE
@@ -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) {
@@ -77,6 +77,7 @@ static int gb_uart_receive_data_handler(struct
gb_operation *op)
struct tty_port *port = &gb_tty->port; struct gb_message *request = op->request; struct gb_uart_recv_data_request *receive_data;
Please do not add a blank line amid the local variable definitions.
I didn't notice that addition (it was not intentional). I'll delete the line in v3.
I'm not sure it checks for this, but you should run your patch through "checkpatch.pl" before you send it. E.g.: ./scripts/checkpatch.pl idr_to_xarray.patch
I've configured an automatic run of checkpatch.pl a long time ago. It runs (automatically) every time I save a "git commit -s -v". Unfortunately, sometimes happens that I'm distracted by something else and I don't see its output (at least I don't read it in its entirety). My fault, obviously. I'll be more focused on what I'm doing when I'm working on the next patches.
The error reported in the build of your first version of this patch makes me think you might not have test- built the code. I don't know if that's the case, but (at least) building the code is expected before you submit a patch for review.
As said above, I have little experience. So, believe me, I don't minimally trust my own code and I wouldn't dare to submit patches without building with "make C=2 -j8 drivers/staging/greybus/ W=1".
I'm not entirely sure of what happened, because I ran make at least a couple of times, maybe more. I suppose it has to do with some greybus related options in .config that only this evening I noticed I had to enable. When today I ran "make menuconfig" I saw that a couple of them were not set but I can't remember which.
Now that they are set, GCC fails with the v1 of my patch (downloaded and installed on a new test branch based on Greg's staging-testing). Yesterday it didn't fail.
u16 recv_data_size; int count; unsigned long tty_flags = TTY_NORMAL;
@@ -341,8 +342,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 +354,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;The caller of alloc_minor() (gb_uart_probe()) checks the return value, and if it's -ENOSPC it logs a device error indicating there are no remaining free device minor numbers. For xa_alloc() this is indicated by returning -EBUSY. Please update the caller to print the error message based on the updated error code.
Correct, I should have made it since v1. This will also go in v3.
gb_tty->minor = minor;
return minor;
}
@@ -374,9 +375,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)
@@ -982,7 +981,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[] = {
Thanks for your kind review and the time you spent on it.
Regards,
Fabio
On Monday, August 16, 2021 4:46:08 PM CEST Alex Elder wrote:
On 8/14/21 1:11 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.
I haven't verified the use of the new API (yet) but I have a few comments on your patch, below.
-Alex
Hi Alex,
As I promised in another message, I've already submitted a v3 of this patch: https://lkml.org/lkml/2021/8/16/1188
While I'm pretty sure that XArray should be used in place of the older and less efficient IDR (some time ago Matthew Wilcox agreed and confirmed that this is true), I'm not entirely sure if we should also prefer XArray over IDA for this particular driver.
Initially I had decided to convert the other greybus file from IDA to XArray but then I stopped because of the above-mentioned doubts.
I really don't know if it is worth doing this work. As far as I understand these API, IDA (although it is not as versatile as IDR is) is more memory efficient than IDR. In documentation I read: "The IDA is an ID allocator which does not provide the ability to associate an ID with a pointer. As such, it only needs to store one bit per ID, and so is more space efficient than an IDR.".
May you please say if you think that the driver would also benefit by the conversion from IDA to XArray?
Thanks,
Fabio
On Monday, August 16, 2021 4:46:08 PM CEST Alex Elder wrote:
On 8/14/21 1:11 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.
I haven't verified the use of the new API (yet) but I have a few comments on your patch, below.
-Alex
Dear Alex,
On August 16th I submitted the v3 of my patch ("staging: greybus: Convert uart.c from IDR to XArray"), with changes based on the comments you provided.
Could you please take a few minutes to review this too? I would really appreciate it.
The v3 patch is at https://lore.kernel.org/lkml/20210816195000.736-1-fmdefrancesco@gmail.com/
Thanks,
Fabio
P.S.: I'd also like to know if you think it's worth converting IDA to XArray in order to improve the Greybus driver in staging.
On 8/25/21 12:20 AM, Fabio M. De Francesco wrote:
On Monday, August 16, 2021 4:46:08 PM CEST Alex Elder wrote:
On 8/14/21 1:11 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.
I haven't verified the use of the new API (yet) but I have a few comments on your patch, below.
-AlexDear Alex,
On August 16th I submitted the v3 of my patch ("staging: greybus: Convert uart.c from IDR to XArray"), with changes based on the comments you provided.
Yes, I intend to review version 3. I'm sorry I didn't respond to your earlier message; I am on vacation this week.
-Alex
Could you please take a few minutes to review this too? I would really appreciate it.
The v3 patch is at https://lore.kernel.org/lkml/20210816195000.736-1-fmdefrancesco@gmail.com/
Thanks,
Fabio
P.S.: I'd also like to know if you think it's worth converting IDA to XArray in order to improve the Greybus driver in staging.
greybus-dev mailing list greybus-dev@lists.linaro.org https://lists.linaro.org/mailman/listinfo/greybus-dev
On Wednesday, August 25, 2021 3:45:13 PM CEST Alex Elder wrote:
On 8/25/21 12:20 AM, Fabio M. De Francesco wrote:
On Monday, August 16, 2021 4:46:08 PM CEST Alex Elder wrote:
On 8/14/21 1:11 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.
I haven't verified the use of the new API (yet) but I have a few comments on your patch, below.
-AlexDear Alex,
On August 16th I submitted the v3 of my patch ("staging: greybus: Convert uart.c from IDR to XArray"), with changes based on the comments you provided.
Yes, I intend to review version 3. I'm sorry I didn't respond to your earlier message; I am on vacation this week.
-Alex
Oh, there's no hurry, sorry to bother you while on vacation. Even kernel hackers deserve a vacation at least once a year or two... :-)
Thanks,
Fabio
Could you please take a few minutes to review this too? I would really appreciate it.
The v3 patch is at https://lore.kernel.org/lkml/20210816195000.736-1-fmdefrancesco@gmail.com/
Thanks,
Fabio
P.S.: I'd also like to know if you think it's worth converting IDA to XArray in order to improve the Greybus driver in staging.
greybus-dev mailing list greybus-dev@lists.linaro.org https://lists.linaro.org/mailman/listinfo/greybus-dev
-
Alex Elder -
Alex Elder -
Dan Carpenter -
Fabio M. De Francesco -
Greg Kroah-Hartman