LinuxLists.cc - [PATCH] staging: vt6656: Declare a few variables as __read

2020-03-01 11:29:46

Subject: [PATCH] staging: vt6656: Declare a few variables as __read_mostly

These include module parameters.

Signed-off-by: Oscar Carter <[email protected]>
---
drivers/staging/vt6656/main_usb.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/vt6656/main_usb.c b/drivers/staging/vt6656/main_usb.c
index 5e48b3ddb94c..701300202b21 100644
--- a/drivers/staging/vt6656/main_usb.c
+++ b/drivers/staging/vt6656/main_usb.c
@@ -49,12 +49,12 @@ MODULE_LICENSE("GPL");
MODULE_DESCRIPTION(DEVICE_FULL_DRV_NAM);

#define RX_DESC_DEF0 64
-static int vnt_rx_buffers = RX_DESC_DEF0;
+static int __read_mostly vnt_rx_buffers = RX_DESC_DEF0;
module_param_named(rx_buffers, vnt_rx_buffers, int, 0644);
MODULE_PARM_DESC(rx_buffers, "Number of receive usb rx buffers");

#define TX_DESC_DEF0 64
-static int vnt_tx_buffers = TX_DESC_DEF0;
+static int __read_mostly vnt_tx_buffers = TX_DESC_DEF0;
module_param_named(tx_buffers, vnt_tx_buffers, int, 0644);
MODULE_PARM_DESC(tx_buffers, "Number of receive usb tx buffers");

--
2.20.1

2020-03-01 12:26:43

by Greg Kroah-Hartman

[permalink] [raw]

Subject: Re: [PATCH] staging: vt6656: Declare a few variables as __read_mostly

On Sun, Mar 01, 2020 at 12:26:20PM +0100, Oscar Carter wrote:
> These include module parameters.
>
> Signed-off-by: Oscar Carter <[email protected]>
> ---
> drivers/staging/vt6656/main_usb.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/staging/vt6656/main_usb.c b/drivers/staging/vt6656/main_usb.c
> index 5e48b3ddb94c..701300202b21 100644
> --- a/drivers/staging/vt6656/main_usb.c
> +++ b/drivers/staging/vt6656/main_usb.c
> @@ -49,12 +49,12 @@ MODULE_LICENSE("GPL");
> MODULE_DESCRIPTION(DEVICE_FULL_DRV_NAM);
>
> #define RX_DESC_DEF0 64
> -static int vnt_rx_buffers = RX_DESC_DEF0;
> +static int __read_mostly vnt_rx_buffers = RX_DESC_DEF0;
> module_param_named(rx_buffers, vnt_rx_buffers, int, 0644);
> MODULE_PARM_DESC(rx_buffers, "Number of receive usb rx buffers");
>
> #define TX_DESC_DEF0 64
> -static int vnt_tx_buffers = TX_DESC_DEF0;
> +static int __read_mostly vnt_tx_buffers = TX_DESC_DEF0;
> module_param_named(tx_buffers, vnt_tx_buffers, int, 0644);
> MODULE_PARM_DESC(tx_buffers, "Number of receive usb tx buffers");
>

Why? What does this help with?

thanks,

greg k-h

2020-03-01 13:21:03

by Oscar Carter

[permalink] [raw]

Subject: Re: [PATCH] staging: vt6656: Declare a few variables as __read_mostly

On Sun, Mar 01, 2020 at 01:25:14PM +0100, Greg Kroah-Hartman wrote:
> On Sun, Mar 01, 2020 at 12:26:20PM +0100, Oscar Carter wrote:
> > These include module parameters.
> >
> > Signed-off-by: Oscar Carter <[email protected]>
> > ---
> > drivers/staging/vt6656/main_usb.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/staging/vt6656/main_usb.c b/drivers/staging/vt6656/main_usb.c
> > index 5e48b3ddb94c..701300202b21 100644
> > --- a/drivers/staging/vt6656/main_usb.c
> > +++ b/drivers/staging/vt6656/main_usb.c
> > @@ -49,12 +49,12 @@ MODULE_LICENSE("GPL");
> > MODULE_DESCRIPTION(DEVICE_FULL_DRV_NAM);
> >
> > #define RX_DESC_DEF0 64
> > -static int vnt_rx_buffers = RX_DESC_DEF0;
> > +static int __read_mostly vnt_rx_buffers = RX_DESC_DEF0;
> > module_param_named(rx_buffers, vnt_rx_buffers, int, 0644);
> > MODULE_PARM_DESC(rx_buffers, "Number of receive usb rx buffers");
> >
> > #define TX_DESC_DEF0 64
> > -static int vnt_tx_buffers = TX_DESC_DEF0;
> > +static int __read_mostly vnt_tx_buffers = TX_DESC_DEF0;
> > module_param_named(tx_buffers, vnt_tx_buffers, int, 0644);
> > MODULE_PARM_DESC(tx_buffers, "Number of receive usb tx buffers");
> >
>
> Why? What does this help with?

If we declare these variables __read_mostly we can improve the performance. If
these variables are read many more times than written, each core of a multicore
system can maintain a copy in a local cache and the time to access is less than
if they use the shared-cache.

>
> thanks,
>
> greg k-h

thanks,

oscar carter

2020-03-01 15:09:48

by Greg Kroah-Hartman

[permalink] [raw]

Subject: Re: [PATCH] staging: vt6656: Declare a few variables as __read_mostly

On Sun, Mar 01, 2020 at 02:17:01PM +0100, Oscar Carter wrote:
> On Sun, Mar 01, 2020 at 01:25:14PM +0100, Greg Kroah-Hartman wrote:
> > On Sun, Mar 01, 2020 at 12:26:20PM +0100, Oscar Carter wrote:
> > > These include module parameters.
> > >
> > > Signed-off-by: Oscar Carter <[email protected]>
> > > ---
> > > drivers/staging/vt6656/main_usb.c | 4 ++--
> > > 1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/staging/vt6656/main_usb.c b/drivers/staging/vt6656/main_usb.c
> > > index 5e48b3ddb94c..701300202b21 100644
> > > --- a/drivers/staging/vt6656/main_usb.c
> > > +++ b/drivers/staging/vt6656/main_usb.c
> > > @@ -49,12 +49,12 @@ MODULE_LICENSE("GPL");
> > > MODULE_DESCRIPTION(DEVICE_FULL_DRV_NAM);
> > >
> > > #define RX_DESC_DEF0 64
> > > -static int vnt_rx_buffers = RX_DESC_DEF0;
> > > +static int __read_mostly vnt_rx_buffers = RX_DESC_DEF0;
> > > module_param_named(rx_buffers, vnt_rx_buffers, int, 0644);
> > > MODULE_PARM_DESC(rx_buffers, "Number of receive usb rx buffers");
> > >
> > > #define TX_DESC_DEF0 64
> > > -static int vnt_tx_buffers = TX_DESC_DEF0;
> > > +static int __read_mostly vnt_tx_buffers = TX_DESC_DEF0;
> > > module_param_named(tx_buffers, vnt_tx_buffers, int, 0644);
> > > MODULE_PARM_DESC(tx_buffers, "Number of receive usb tx buffers");
> > >
> >
> > Why? What does this help with?
>
> If we declare these variables __read_mostly we can improve the performance. If
> these variables are read many more times than written, each core of a multicore
> system can maintain a copy in a local cache and the time to access is less than
> if they use the shared-cache.

This is a USB driver, performance is always limited to the hardware, not
the CPU location of variables.

Please always benchmark things to see if it actually makes sense to make
changes like this, before proposing them.

thanks,

greg k-h

2020-03-02 15:45:00

by Quentin Deslandes

[permalink] [raw]

Subject: Re: [PATCH] staging: vt6656: Declare a few variables as __read_mostly

2020-03-07 08:31:20

by Oscar Carter

[permalink] [raw]

Subject: Re: [PATCH] staging: vt6656: Declare a few variables as __read_mostly

On Sun, Mar 01, 2020 at 04:09:13PM +0100, Greg Kroah-Hartman wrote:
> On Sun, Mar 01, 2020 at 02:17:01PM +0100, Oscar Carter wrote:
> > On Sun, Mar 01, 2020 at 01:25:14PM +0100, Greg Kroah-Hartman wrote:
> > > On Sun, Mar 01, 2020 at 12:26:20PM +0100, Oscar Carter wrote:
> > > > These include module parameters.
> > > >
> > > > Signed-off-by: Oscar Carter <[email protected]>
> > > > ---
> > > > drivers/staging/vt6656/main_usb.c | 4 ++--
> > > > 1 file changed, 2 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/staging/vt6656/main_usb.c b/drivers/staging/vt6656/main_usb.c
> > > > index 5e48b3ddb94c..701300202b21 100644
> > > > --- a/drivers/staging/vt6656/main_usb.c
> > > > +++ b/drivers/staging/vt6656/main_usb.c
> > > > @@ -49,12 +49,12 @@ MODULE_LICENSE("GPL");
> > > > MODULE_DESCRIPTION(DEVICE_FULL_DRV_NAM);
> > > >
> > > > #define RX_DESC_DEF0 64
> > > > -static int vnt_rx_buffers = RX_DESC_DEF0;
> > > > +static int __read_mostly vnt_rx_buffers = RX_DESC_DEF0;
> > > > module_param_named(rx_buffers, vnt_rx_buffers, int, 0644);
> > > > MODULE_PARM_DESC(rx_buffers, "Number of receive usb rx buffers");
> > > >
> > > > #define TX_DESC_DEF0 64
> > > > -static int vnt_tx_buffers = TX_DESC_DEF0;
> > > > +static int __read_mostly vnt_tx_buffers = TX_DESC_DEF0;
> > > > module_param_named(tx_buffers, vnt_tx_buffers, int, 0644);
> > > > MODULE_PARM_DESC(tx_buffers, "Number of receive usb tx buffers");
> > > >
> > >
> > > Why? What does this help with?
> >
> > If we declare these variables __read_mostly we can improve the performance. If
> > these variables are read many more times than written, each core of a multicore
> > system can maintain a copy in a local cache and the time to access is less than
> > if they use the shared-cache.
>
> This is a USB driver, performance is always limited to the hardware, not
> the CPU location of variables.

Thank you for the explanation.

>
> Please always benchmark things to see if it actually makes sense to make
> changes like this, before proposing them.

I'm sorry.

>
> thanks,
>
> greg k-h

thanks,

Oscar

2020-03-09 09:33:00

by Quentin Deslandes

[permalink] [raw]

Subject: Re: [PATCH] staging: vt6656: Declare a few variables as __read_mostly

On Sat, Mar 07, 2020 at 09:29:06AM +0100, Oscar Carter wrote:
> On Sun, Mar 01, 2020 at 04:09:13PM +0100, Greg Kroah-Hartman wrote:
> > On Sun, Mar 01, 2020 at 02:17:01PM +0100, Oscar Carter wrote:
> > This is a USB driver, performance is always limited to the hardware, not
> > the CPU location of variables.
>
> Thank you for the explanation.
>
> >
> > Please always benchmark things to see if it actually makes sense to make
> > changes like this, before proposing them.
>
> I'm sorry.
>

I've been removed from CC list on Greg's answer, so I haven't seen the
explanation earlier and reviewed the patch the next day. I should have
know better, won't happen again.

Thank you,
Quentin

2020-03-09 10:34:43

by Greg Kroah-Hartman

[permalink] [raw]

Subject: Re: [PATCH] staging: vt6656: Declare a few variables as __read_mostly

On Mon, Mar 09, 2020 at 09:32:10AM +0000, Quentin Deslandes wrote:
> On Sat, Mar 07, 2020 at 09:29:06AM +0100, Oscar Carter wrote:
> > On Sun, Mar 01, 2020 at 04:09:13PM +0100, Greg Kroah-Hartman wrote:
> > > On Sun, Mar 01, 2020 at 02:17:01PM +0100, Oscar Carter wrote:
> > > This is a USB driver, performance is always limited to the hardware, not
> > > the CPU location of variables.
> >
> > Thank you for the explanation.
> >
> > >
> > > Please always benchmark things to see if it actually makes sense to make
> > > changes like this, before proposing them.
> >
> > I'm sorry.
> >
>
> I've been removed from CC list on Greg's answer, so I haven't seen the
> explanation earlier and reviewed the patch the next day. I should have
> know better, won't happen again.

You weren't on the original list of people on the patch, so I didn't
remove anything here that I can tell.

thanks,

greg k-h

2020-03-09 11:10:48

by Greg Kroah-Hartman

[permalink] [raw]

Subject: Re: [PATCH] staging: vt6656: Declare a few variables as __read_mostly

On Mon, Mar 09, 2020 at 11:34:07AM +0100, Greg Kroah-Hartman wrote:
> On Mon, Mar 09, 2020 at 09:32:10AM +0000, Quentin Deslandes wrote:
> > On Sat, Mar 07, 2020 at 09:29:06AM +0100, Oscar Carter wrote:
> > > On Sun, Mar 01, 2020 at 04:09:13PM +0100, Greg Kroah-Hartman wrote:
> > > > On Sun, Mar 01, 2020 at 02:17:01PM +0100, Oscar Carter wrote:
> > > > This is a USB driver, performance is always limited to the hardware, not
> > > > the CPU location of variables.
> > >
> > > Thank you for the explanation.
> > >
> > > >
> > > > Please always benchmark things to see if it actually makes sense to make
> > > > changes like this, before proposing them.
> > >
> > > I'm sorry.
> > >
> >
> > I've been removed from CC list on Greg's answer, so I haven't seen the
> > explanation earlier and reviewed the patch the next day. I should have
> > know better, won't happen again.
>
> You weren't on the original list of people on the patch, so I didn't
> remove anything here that I can tell.

Turns out you were, on the lkml one, but not the one that went through
the driver-devel list. odd...