linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] staging: vt6656: Declare a few variables as __read_mostly
@ 2020-03-01 11:26 Oscar Carter
  2020-03-01 12:25 ` Greg Kroah-Hartman
  2020-03-02 15:43 ` Quentin Deslandes
  0 siblings, 2 replies; 9+ messages in thread
From: Oscar Carter @ 2020-03-01 11:26 UTC (permalink / raw)
  To: Forest Bond, Greg Kroah-Hartman
  Cc: Quentin Deslandes, Malcolm Priestley, Gabriela Bittencourt,
	Colin Ian King, Oscar Carter, devel, linux-kernel

These include module parameters.

Signed-off-by: Oscar Carter <oscar.carter@gmx.com>
---
 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


^ permalink raw reply related	[flat|nested] 9+ messages in thread
* Re: [PATCH] staging: vt6656: Declare a few variables as __read_mostly
  2020-03-01 11:26 [PATCH] staging: vt6656: Declare a few variables as __read_mostly Oscar Carter
@ 2020-03-01 12:25 ` Greg Kroah-Hartman
  2020-03-01 13:17   ` Oscar Carter
  2020-03-02 15:43 ` Quentin Deslandes
  1 sibling, 1 reply; 9+ messages in thread
From: Greg Kroah-Hartman @ 2020-03-01 12:25 UTC (permalink / raw)
  To: Oscar Carter
  Cc: Forest Bond, devel, Malcolm Priestley, linux-kernel,
	Gabriela Bittencourt, Colin Ian King

On Sun, Mar 01, 2020 at 12:26:20PM +0100, Oscar Carter wrote:
> These include module parameters.
> 
> Signed-off-by: Oscar Carter <oscar.carter@gmx.com>
> ---
>  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

^ permalink raw reply	[flat|nested] 9+ messages in thread
* Re: [PATCH] staging: vt6656: Declare a few variables as __read_mostly
  2020-03-01 12:25 ` Greg Kroah-Hartman
@ 2020-03-01 13:17   ` Oscar Carter
  2020-03-01 15:09     ` Greg Kroah-Hartman
  0 siblings, 1 reply; 9+ messages in thread
From: Oscar Carter @ 2020-03-01 13:17 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Forest Bond, devel, Malcolm Priestley, linux-kernel,
	Gabriela Bittencourt, Colin Ian King, Oscar Carter

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 <oscar.carter@gmx.com>
> > ---
> >  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

^ permalink raw reply	[flat|nested] 9+ messages in thread
* Re: [PATCH] staging: vt6656: Declare a few variables as __read_mostly
  2020-03-01 13:17   ` Oscar Carter
@ 2020-03-01 15:09     ` Greg Kroah-Hartman
  2020-03-07  8:29       ` Oscar Carter
  0 siblings, 1 reply; 9+ messages in thread
From: Greg Kroah-Hartman @ 2020-03-01 15:09 UTC (permalink / raw)
  To: Oscar Carter
  Cc: devel, Malcolm Priestley, linux-kernel, Forest Bond,
	Gabriela Bittencourt, Colin Ian King

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 <oscar.carter@gmx.com>
> > > ---
> > >  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

^ permalink raw reply	[flat|nested] 9+ messages in thread
* Re: [PATCH] staging: vt6656: Declare a few variables as __read_mostly
  2020-03-01 11:26 [PATCH] staging: vt6656: Declare a few variables as __read_mostly Oscar Carter
  2020-03-01 12:25 ` Greg Kroah-Hartman
@ 2020-03-02 15:43 ` Quentin Deslandes
  1 sibling, 0 replies; 9+ messages in thread
From: Quentin Deslandes @ 2020-03-02 15:43 UTC (permalink / raw)
  To: Oscar Carter
  Cc: Forest Bond, Greg Kroah-Hartman, Malcolm Priestley,
	Gabriela Bittencourt, Colin Ian King, devel, linux-kernel

On Sun, Mar 01, 2020 at 12:26:20PM +0100, Oscar Carter wrote:
> These include module parameters.
> 
> Signed-off-by: Oscar Carter <oscar.carter@gmx.com>
> ---
>  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
> 

Looks good to me.

Reviewed-by: Quentin Deslandes <quentin.deslandes@itdev.co.uk>


^ permalink raw reply	[flat|nested] 9+ messages in thread
* Re: [PATCH] staging: vt6656: Declare a few variables as __read_mostly
  2020-03-01 15:09     ` Greg Kroah-Hartman
@ 2020-03-07  8:29       ` Oscar Carter
  2020-03-09  9:32         ` Quentin Deslandes
  0 siblings, 1 reply; 9+ messages in thread
From: Oscar Carter @ 2020-03-07  8:29 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Forest Bond, Quentin Deslandes, Malcolm Priestley,
	Gabriela Bittencourt, Colin Ian King, Oscar Carter, devel,
	linux-kernel

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 <oscar.carter@gmx.com>
> > > > ---
> > > >  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

^ permalink raw reply	[flat|nested] 9+ messages in thread
* Re: [PATCH] staging: vt6656: Declare a few variables as __read_mostly
  2020-03-07  8:29       ` Oscar Carter
@ 2020-03-09  9:32         ` Quentin Deslandes
  2020-03-09 10:34           ` Greg Kroah-Hartman
  0 siblings, 1 reply; 9+ messages in thread
From: Quentin Deslandes @ 2020-03-09  9:32 UTC (permalink / raw)
  To: Oscar Carter
  Cc: Greg Kroah-Hartman, Forest Bond, Malcolm Priestley,
	Gabriela Bittencourt, Colin Ian King, devel, linux-kernel

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

^ permalink raw reply	[flat|nested] 9+ messages in thread
* Re: [PATCH] staging: vt6656: Declare a few variables as __read_mostly
  2020-03-09  9:32         ` Quentin Deslandes
@ 2020-03-09 10:34           ` Greg Kroah-Hartman
  2020-03-09 11:09             ` Greg Kroah-Hartman
  0 siblings, 1 reply; 9+ messages in thread
From: Greg Kroah-Hartman @ 2020-03-09 10:34 UTC (permalink / raw)
  To: Quentin Deslandes
  Cc: Oscar Carter, Forest Bond, Malcolm Priestley,
	Gabriela Bittencourt, Colin Ian King, devel, linux-kernel

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

^ permalink raw reply	[flat|nested] 9+ messages in thread
* Re: [PATCH] staging: vt6656: Declare a few variables as __read_mostly
  2020-03-09 10:34           ` Greg Kroah-Hartman
@ 2020-03-09 11:09             ` Greg Kroah-Hartman
  0 siblings, 0 replies; 9+ messages in thread
From: Greg Kroah-Hartman @ 2020-03-09 11:09 UTC (permalink / raw)
  To: Quentin Deslandes
  Cc: devel, Oscar Carter, Malcolm Priestley, linux-kernel,
	Forest Bond, Gabriela Bittencourt, Colin Ian King

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


^ permalink raw reply	[flat|nested] 9+ messages in thread
end of thread, other threads:[~2020-03-09 11:09 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-01 11:26 [PATCH] staging: vt6656: Declare a few variables as __read_mostly Oscar Carter
2020-03-01 12:25 ` Greg Kroah-Hartman
2020-03-01 13:17   ` Oscar Carter
2020-03-01 15:09     ` Greg Kroah-Hartman
2020-03-07  8:29       ` Oscar Carter
2020-03-09  9:32         ` Quentin Deslandes
2020-03-09 10:34           ` Greg Kroah-Hartman
2020-03-09 11:09             ` Greg Kroah-Hartman
2020-03-02 15:43 ` Quentin Deslandes

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).