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

[permalink] [raw]

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

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 <[email protected]>
> Signed-off-by: Fabio M. De Francesco <[email protected]>

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 <[email protected]>...).

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

2021-08-16 15:05:58

by Greg Kroah-Hartman

[permalink] [raw]

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

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 <[email protected]>
> > Signed-off-by: Fabio M. De Francesco <[email protected]>
>
> 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 <[email protected]>...).

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

thanks,

greg k-h

2021-08-16 15:10:43

[permalink] [raw]

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

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 <[email protected]>
> > > Signed-off-by: Fabio M. De Francesco <[email protected]>
> >
> > 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 <[email protected]>...).
>
> 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

2021-08-16 15:14:16

[permalink] [raw]

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

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 <[email protected]>
>>>> Signed-off-by: Fabio M. De Francesco <[email protected]>
>>>
>>> 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 <[email protected]>...).
>>
>> 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 <[email protected]>
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
> [email protected]
> https://lists.linaro.org/mailman/listinfo/greybus-dev
>

2021-08-16 16:57:55

[permalink] [raw]

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

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 <[email protected]>
> > Signed-off-by: Fabio M. De Francesco <[email protected]>
>
> 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 <[email protected]>...).

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

2021-08-16 18:39:25

[permalink] [raw]

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

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 <[email protected]>
> > > > > Signed-off-by: Fabio M. De Francesco <[email protected]>
> > > >
> > > > 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 <[email protected]>...).
> > >
> > > 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 <[email protected]>
> 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

2021-08-16 18:41:29

[permalink] [raw]

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

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 <[email protected]>
> > > > > > Signed-off-by: Fabio M. De Francesco <[email protected]>
> > > > >
> > > > > 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 <[email protected]>...).
> > > >
> > > > 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 <[email protected]>
> > 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

2021-08-16 19:18:10

[permalink] [raw]

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

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<[email protected]>
>> 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

2021-08-17 05:00:24

[permalink] [raw]

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

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

2021-08-17 10:19:54

[permalink] [raw]

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

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

2021-08-25 05:22:00

[permalink] [raw]

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

2021-08-25 14:29:03

[permalink] [raw]

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

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/[email protected]/
>
> 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
> [email protected]
> https://lists.linaro.org/mailman/listinfo/greybus-dev
>

2021-08-26 15:58:03