[PATCH v2] staging: greybus: Convert uart.c from IDR to XArray

[PATCH v2] staging: greybus: Convert uart.c from IDR to XArray

[Fabio M. De Francesco]

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[] = {
-- 
2.32.0

Re: [PATCH v2] staging: greybus: Convert uart.c from IDR to XArray

[Alex Elder]

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[] = {
> 

Re: [PATCH v2] staging: greybus: Convert uart.c from IDR to XArray

[Greg Kroah-Hartman]

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.
> 
> 					-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>...).

You are correct, "Reported-by:" does not make sense here.

thanks,

greg k-h

Re: [PATCH v2] staging: greybus: Convert uart.c from IDR to XArray

[Dan Carpenter]

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.
> > 
> > 					-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>...).
> 
> 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

Re: [greybus-dev] [PATCH v2] staging: greybus: Convert uart.c from IDR to XArray

[Alex Elder]

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.
>>>
>>> 					-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>...).
>>
>> 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
> 

Re: [PATCH v2] staging: greybus: Convert uart.c from IDR to XArray

[Fabio M. De Francesco]

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.
> 
> 					-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>...).

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
 

Re: [greybus-dev] [PATCH v2] staging: greybus: Convert uart.c from IDR to XArray

[Dan Carpenter]

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.
> > > > 
> > > > 					-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>...).
> > > 
> > > 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

Re: [greybus-dev] [PATCH v2] staging: greybus: Convert uart.c from IDR to XArray

[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.
> > > > > 
> > > > > 					-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>...).
> > > > 
> > > > 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

Re: [greybus-dev] [PATCH v2] staging: greybus: Convert uart.c from IDR to XArray

[Alex Elder]

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 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 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

Re: [greybus-dev] [PATCH v2] staging: greybus: Convert uart.c from IDR to XArray

[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

Re: [PATCH v2] staging: greybus: Convert uart.c from IDR to XArray

[Fabio M. De Francesco]

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

Re: [PATCH v2] staging: greybus: Convert uart.c from IDR to XArray

[Fabio M. De Francesco]

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.

  

Re: [greybus-dev] [PATCH v2] staging: greybus: Convert uart.c from IDR to XArray

[Alex Elder]

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.
>>
>> 					-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.

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
> 

Re: [greybus-dev] [PATCH v2] staging: greybus: Convert uart.c from IDR to XArray

[Fabio M. De Francesco]

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.
> >>
> >> 					-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.
> 
> 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
> > 
> 
>