[PATCH -mm] lib/ts_fsm.c: constify structs

[PATCH -mm] lib/ts_fsm.c: constify structs

[Andreas Mohr]

Constify two structs.
Correct some typos.

Compile-tested and run-tested (module inserted) on 2.6.18-rc4-mm3.


Signed-off-by: Andreas Mohr <andi@lisas.de>


--- linux-2.6.18-rc4-mm3.orig/lib/ts_fsm.c	2006-08-22 21:09:55.000000000 +0200
+++ linux-2.6.18-rc4-mm3/lib/ts_fsm.c	2006-09-08 19:46:07.000000000 +0200
@@ -12,13 +12,13 @@
  *
  *   A finite state machine consists of n states (struct ts_fsm_token)
  *   representing the pattern as a finite automation. The data is read
- *   sequentially on a octet basis. Every state token specifies the number
+ *   sequentially on an octet basis. Every state token specifies the number
  *   of recurrences and the type of value accepted which can be either a
  *   specific character or ctype based set of characters. The available
  *   type of recurrences include 1, (0|1), [0 n], and [1 n].
  *
- *   The algorithm differs between strict/non-strict mode specyfing
- *   whether the pattern has to start at the first octect. Strict mode
+ *   The algorithm differs between strict/non-strict mode specifying
+ *   whether the pattern has to start at the first octet. Strict mode
  *   is enabled by default and can be disabled by inserting
  *   TS_FSM_HEAD_IGNORE as the first token in the chain.
  *
@@ -44,7 +44,7 @@
 #define _W		0x200 /* wildcard */
 
 /* Map to _ctype flags and some magic numbers */
-static u16 token_map[TS_FSM_TYPE_MAX+1] = {
+static const u16 token_map[TS_FSM_TYPE_MAX+1] = {
 	[TS_FSM_SPECIFIC] = 0,
 	[TS_FSM_WILDCARD] = _W,
 	[TS_FSM_CNTRL]	  = _C,
@@ -61,7 +61,7 @@
 	[TS_FSM_ASCII]	  = _A,
 };
 
-static u16 token_lookup_tbl[256] = {
+static const u16 token_lookup_tbl[256] = {
 _W|_A|_C,      _W|_A|_C,     _W|_A|_C,     _W|_A|_C,		/*   0-  3 */
 _W|_A|_C,      _W|_A|_C,     _W|_A|_C,     _W|_A|_C,		/*   4-  7 */
 _W|_A|_C,      _W|_A|_C|_S,  _W|_A|_C|_S,  _W|_A|_C|_S,		/*   8- 11 */

Re: [PATCH -mm] lib/ts_fsm.c: constify structs

[Thomas Graf]

* Andreas Mohr <andi@rhlx01.fht-esslingen.de> 2006-08-31 20:27
> @@ -44,7 +44,7 @@
>  #define _W		0x200 /* wildcard */
>  
>  /* Map to _ctype flags and some magic numbers */
> -static u16 token_map[TS_FSM_TYPE_MAX+1] = {
> +static const u16 token_map[TS_FSM_TYPE_MAX+1] = {
>  	[TS_FSM_SPECIFIC] = 0,
>  	[TS_FSM_WILDCARD] = _W,
>  	[TS_FSM_CNTRL]	  = _C,
> @@ -61,7 +61,7 @@
>  	[TS_FSM_ASCII]	  = _A,
>  };
>  
> -static u16 token_lookup_tbl[256] = {
> +static const u16 token_lookup_tbl[256] = {
>  _W|_A|_C,      _W|_A|_C,     _W|_A|_C,     _W|_A|_C,		/*   0-  3 */
>  _W|_A|_C,      _W|_A|_C,     _W|_A|_C,     _W|_A|_C,		/*   4-  7 */
>  _W|_A|_C,      _W|_A|_C|_S,  _W|_A|_C|_S,  _W|_A|_C|_S,		/*   8- 11 */

You could mark them __read_mostly at this opportunity.

Re: [PATCH -mm] lib/ts_fsm.c: constify structs

[Andreas Mohr]

Hi,

On Fri, Sep 01, 2006 at 01:31:37PM +0200, Thomas Graf wrote:
> * Andreas Mohr <andi@rhlx01.fht-esslingen.de> 2006-08-31 20:27
> > @@ -44,7 +44,7 @@
> >  #define _W		0x200 /* wildcard */
> >  
> >  /* Map to _ctype flags and some magic numbers */
> > -static u16 token_map[TS_FSM_TYPE_MAX+1] = {
> > +static const u16 token_map[TS_FSM_TYPE_MAX+1] = {
> >  	[TS_FSM_SPECIFIC] = 0,
> >  	[TS_FSM_WILDCARD] = _W,
> >  	[TS_FSM_CNTRL]	  = _C,
> > @@ -61,7 +61,7 @@
> >  	[TS_FSM_ASCII]	  = _A,
> >  };
> >  
> > -static u16 token_lookup_tbl[256] = {
> > +static const u16 token_lookup_tbl[256] = {
> >  _W|_A|_C,      _W|_A|_C,     _W|_A|_C,     _W|_A|_C,		/*   0-  3 */
> >  _W|_A|_C,      _W|_A|_C,     _W|_A|_C,     _W|_A|_C,		/*   4-  7 */
> >  _W|_A|_C,      _W|_A|_C|_S,  _W|_A|_C|_S,  _W|_A|_C|_S,		/*   8- 11 */
> 
> You could mark them __read_mostly at this opportunity.

Thanks for the review!

However being const already implies __read_mostly if I'm not mistaken,
due to its const nature which doesn't allow any modification. ;)

[ ...
__read_mostly is a way to cleanly separate rarely-modified
writable data from data which is being modified very often which thus
always gets "occupied" by the local CPU cache - then other CPUs would have to
wait for the modified data to make it back into main memory before they're
able to access it. That's why you want to mark frequently-read data
as __read_mostly to make sure it's not in the same (dirtied) cache line
of some frequently-modified variables but instead in an always-unmodified
cache line which thus always remains instantly available due to being
consistent with its main memory content (not dirty).
... ]

Re: [PATCH -mm] lib/ts_fsm.c: constify structs

[Thomas Graf]

* Andreas Mohr <andi@rhlx01.fht-esslingen.de> 2006-09-01 14:51
> Hi,
> 
> On Fri, Sep 01, 2006 at 01:31:37PM +0200, Thomas Graf wrote:
> > * Andreas Mohr <andi@rhlx01.fht-esslingen.de> 2006-08-31 20:27
> > > @@ -44,7 +44,7 @@
> > >  #define _W		0x200 /* wildcard */
> > >  
> > >  /* Map to _ctype flags and some magic numbers */
> > > -static u16 token_map[TS_FSM_TYPE_MAX+1] = {
> > > +static const u16 token_map[TS_FSM_TYPE_MAX+1] = {
> > >  	[TS_FSM_SPECIFIC] = 0,
> > >  	[TS_FSM_WILDCARD] = _W,
> > >  	[TS_FSM_CNTRL]	  = _C,
> > > @@ -61,7 +61,7 @@
> > >  	[TS_FSM_ASCII]	  = _A,
> > >  };
> > >  
> > > -static u16 token_lookup_tbl[256] = {
> > > +static const u16 token_lookup_tbl[256] = {
> > >  _W|_A|_C,      _W|_A|_C,     _W|_A|_C,     _W|_A|_C,		/*   0-  3 */
> > >  _W|_A|_C,      _W|_A|_C,     _W|_A|_C,     _W|_A|_C,		/*   4-  7 */
> > >  _W|_A|_C,      _W|_A|_C|_S,  _W|_A|_C|_S,  _W|_A|_C|_S,		/*   8- 11 */
> > 
> > You could mark them __read_mostly at this opportunity.
> 
> Thanks for the review!
> 
> However being const already implies __read_mostly if I'm not mistaken,
> due to its const nature which doesn't allow any modification. ;)

Right, not sure what I was thinking. ;) However, gcc probably already
put it into rodata for being static and only referenced for reading
so it probably doesn't make any difference.

Re: [PATCH -mm] lib/ts_fsm.c: constify structs

[Andreas Mohr]

Hi,

On Fri, Sep 01, 2006 at 03:50:16PM +0200, Thomas Graf wrote:
> Right, not sure what I was thinking. ;) However, gcc probably already
> put it into rodata for being static and only referenced for reading
> so it probably doesn't make any difference.

I didn't verify it with this particular patch, but I'm 99.9% certain
that it isn't in rodata, since I've checked it with many other constify
patches before (objdump -x).

Andreas Mohr