| Message ID | 1450904784-17139-1-git-send-email-Julia.Lawall@lip6.fr (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Dec 23, 2015 at 10:06:24PM +0100, Julia Lawall wrote: > The cleancache_ops structure is never modified, so declare it as const. > > This also removes the __read_mostly declaration on the cleancache_ops > variable declaration, since it seems redundant with const. > > Done with the help of Coccinelle. > > Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr> > > --- > > Not sure that the __read_mostly change is correct. Does it apply to the > variable, or to what the variable points to? It should just put the structure in the right section (.rodata). Thanks for the patch! > > drivers/xen/tmem.c | 2 +- > include/linux/cleancache.h | 2 +- > mm/cleancache.c | 4 ++-- > 3 files changed, 4 insertions(+), 4 deletions(-) > > diff --git a/include/linux/cleancache.h b/include/linux/cleancache.h > index bda5ec0b4..cb3e142 100644 > --- a/include/linux/cleancache.h > +++ b/include/linux/cleancache.h > @@ -37,7 +37,7 @@ struct cleancache_ops { > void (*invalidate_fs)(int); > }; > > -extern int cleancache_register_ops(struct cleancache_ops *ops); > +extern int cleancache_register_ops(const struct cleancache_ops *ops); > extern void __cleancache_init_fs(struct super_block *); > extern void __cleancache_init_shared_fs(struct super_block *); > extern int __cleancache_get_page(struct page *); > diff --git a/drivers/xen/tmem.c b/drivers/xen/tmem.c > index 945fc43..4ac2ca8 100644 > --- a/drivers/xen/tmem.c > +++ b/drivers/xen/tmem.c > @@ -242,7 +242,7 @@ static int tmem_cleancache_init_shared_fs(char *uuid, size_t pagesize) > return xen_tmem_new_pool(shared_uuid, TMEM_POOL_SHARED, pagesize); > } > > -static struct cleancache_ops tmem_cleancache_ops = { > +static const struct cleancache_ops tmem_cleancache_ops = { > .put_page = tmem_cleancache_put_page, > .get_page = tmem_cleancache_get_page, > .invalidate_page = tmem_cleancache_flush_page, > diff --git a/mm/cleancache.c b/mm/cleancache.c > index 8fc5081..c6356d6 100644 > --- a/mm/cleancache.c > +++ b/mm/cleancache.c > @@ -22,7 +22,7 @@ > * cleancache_ops is set by cleancache_register_ops to contain the pointers > * to the cleancache "backend" implementation functions. > */ > -static struct cleancache_ops *cleancache_ops __read_mostly; > +static const struct cleancache_ops *cleancache_ops; > > /* > * Counters available via /sys/kernel/debug/cleancache (if debugfs is > @@ -49,7 +49,7 @@ static void cleancache_register_ops_sb(struct super_block *sb, void *unused) > /* > * Register operations for cleancache. Returns 0 on success. > */ > -int cleancache_register_ops(struct cleancache_ops *ops) > +int cleancache_register_ops(const struct cleancache_ops *ops) > { > if (cmpxchg(&cleancache_ops, NULL, ops)) > return -EBUSY; >
On 23/12/15 21:06, Julia Lawall wrote: > The cleancache_ops structure is never modified, so declare it as const. > > This also removes the __read_mostly declaration on the cleancache_ops > variable declaration, since it seems redundant with const. > > Done with the help of Coccinelle. > > Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr> > > --- > > Not sure that the __read_mostly change is correct. Does it apply to the > variable, or to what the variable points to? The variable, so... > --- a/mm/cleancache.c > +++ b/mm/cleancache.c > @@ -22,7 +22,7 @@ > * cleancache_ops is set by cleancache_register_ops to contain the pointers > * to the cleancache "backend" implementation functions. > */ > -static struct cleancache_ops *cleancache_ops __read_mostly; > +static const struct cleancache_ops *cleancache_ops; ...you want to retain the __read_mostly here. David
On Thu, 21 Jan 2016, David Vrabel wrote: > On 23/12/15 21:06, Julia Lawall wrote: > > The cleancache_ops structure is never modified, so declare it as const. > > > > This also removes the __read_mostly declaration on the cleancache_ops > > variable declaration, since it seems redundant with const. > > > > Done with the help of Coccinelle. > > > > Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr> > > > > --- > > > > Not sure that the __read_mostly change is correct. Does it apply to the > > variable, or to what the variable points to? > > The variable, so... Thanks. I'll update the patch, unless you have already fixed it. julia > > --- a/mm/cleancache.c > > +++ b/mm/cleancache.c > > @@ -22,7 +22,7 @@ > > * cleancache_ops is set by cleancache_register_ops to contain the pointers > > * to the cleancache "backend" implementation functions. > > */ > > -static struct cleancache_ops *cleancache_ops __read_mostly; > > +static const struct cleancache_ops *cleancache_ops; > > ...you want to retain the __read_mostly here. > > David > -- > To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >
On Wed, Jan 20 2016, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote: > On Wed, Dec 23, 2015 at 10:06:24PM +0100, Julia Lawall wrote: >> The cleancache_ops structure is never modified, so declare it as const. >> >> This also removes the __read_mostly declaration on the cleancache_ops >> variable declaration, since it seems redundant with const. >> >> Done with the help of Coccinelle. >> >> Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr> >> >> --- >> >> Not sure that the __read_mostly change is correct. Does it apply to the >> variable, or to what the variable points to? > > It should just put the structure in the right section (.rodata). > > Thanks for the patch! The __read_mostly marker should probably be left there... >> */ >> -static struct cleancache_ops *cleancache_ops __read_mostly; >> +static const struct cleancache_ops *cleancache_ops; >> >> /* >> * Counters available via /sys/kernel/debug/cleancache (if debugfs is >> @@ -49,7 +49,7 @@ static void cleancache_register_ops_sb(struct super_block *sb, void *unused) >> /* >> * Register operations for cleancache. Returns 0 on success. >> */ >> -int cleancache_register_ops(struct cleancache_ops *ops) >> +int cleancache_register_ops(const struct cleancache_ops *ops) >> { >> if (cmpxchg(&cleancache_ops, NULL, ops)) >> return -EBUSY; >> I don't know this code, but I assume that this is mostly a one-time thing, so once cleancache_ops gets its value assigned, it doesn't change, and that's what the __read_mostly is about (it applies to the object declared, not whatever it happens to point to). (Also, the commit message is slightly inaccurate: it is tmem_cleancache_ops which is never changed and hence declared const; changing the various pointers to it to const is just a necessary followup). Rasmus
On Thu, 21 Jan 2016, Rasmus Villemoes wrote: > On Wed, Jan 20 2016, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote: > > > On Wed, Dec 23, 2015 at 10:06:24PM +0100, Julia Lawall wrote: > >> The cleancache_ops structure is never modified, so declare it as const. > >> > >> This also removes the __read_mostly declaration on the cleancache_ops > >> variable declaration, since it seems redundant with const. > >> > >> Done with the help of Coccinelle. > >> > >> Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr> > >> > >> --- > >> > >> Not sure that the __read_mostly change is correct. Does it apply to the > >> variable, or to what the variable points to? > > > > It should just put the structure in the right section (.rodata). > > > > Thanks for the patch! > > The __read_mostly marker should probably be left there... I sent a corrected version this afternoon. > > >> */ > >> -static struct cleancache_ops *cleancache_ops __read_mostly; > >> +static const struct cleancache_ops *cleancache_ops; > >> > >> /* > >> * Counters available via /sys/kernel/debug/cleancache (if debugfs is > >> @@ -49,7 +49,7 @@ static void cleancache_register_ops_sb(struct super_block *sb, void *unused) > >> /* > >> * Register operations for cleancache. Returns 0 on success. > >> */ > >> -int cleancache_register_ops(struct cleancache_ops *ops) > >> +int cleancache_register_ops(const struct cleancache_ops *ops) > >> { > >> if (cmpxchg(&cleancache_ops, NULL, ops)) > >> return -EBUSY; > >> > > I don't know this code, but I assume that this is mostly a one-time > thing, so once cleancache_ops gets its value assigned, it doesn't > change, and that's what the __read_mostly is about (it applies to the > object declared, not whatever it happens to point to). > > (Also, the commit message is slightly inaccurate: it is > tmem_cleancache_ops which is never changed and hence declared const; > changing the various pointers to it to const is just a necessary followup). OK, in general, I have referred to the type rather than the structure name in these patches, since there can be more than one structure. julia
diff --git a/include/linux/cleancache.h b/include/linux/cleancache.h index bda5ec0b4..cb3e142 100644 --- a/include/linux/cleancache.h +++ b/include/linux/cleancache.h @@ -37,7 +37,7 @@ struct cleancache_ops { void (*invalidate_fs)(int); }; -extern int cleancache_register_ops(struct cleancache_ops *ops); +extern int cleancache_register_ops(const struct cleancache_ops *ops); extern void __cleancache_init_fs(struct super_block *); extern void __cleancache_init_shared_fs(struct super_block *); extern int __cleancache_get_page(struct page *); diff --git a/drivers/xen/tmem.c b/drivers/xen/tmem.c index 945fc43..4ac2ca8 100644 --- a/drivers/xen/tmem.c +++ b/drivers/xen/tmem.c @@ -242,7 +242,7 @@ static int tmem_cleancache_init_shared_fs(char *uuid, size_t pagesize) return xen_tmem_new_pool(shared_uuid, TMEM_POOL_SHARED, pagesize); } -static struct cleancache_ops tmem_cleancache_ops = { +static const struct cleancache_ops tmem_cleancache_ops = { .put_page = tmem_cleancache_put_page, .get_page = tmem_cleancache_get_page, .invalidate_page = tmem_cleancache_flush_page, diff --git a/mm/cleancache.c b/mm/cleancache.c index 8fc5081..c6356d6 100644 --- a/mm/cleancache.c +++ b/mm/cleancache.c @@ -22,7 +22,7 @@ * cleancache_ops is set by cleancache_register_ops to contain the pointers * to the cleancache "backend" implementation functions. */ -static struct cleancache_ops *cleancache_ops __read_mostly; +static const struct cleancache_ops *cleancache_ops; /* * Counters available via /sys/kernel/debug/cleancache (if debugfs is @@ -49,7 +49,7 @@ static void cleancache_register_ops_sb(struct super_block *sb, void *unused) /* * Register operations for cleancache. Returns 0 on success. */ -int cleancache_register_ops(struct cleancache_ops *ops) +int cleancache_register_ops(const struct cleancache_ops *ops) { if (cmpxchg(&cleancache_ops, NULL, ops)) return -EBUSY;
The cleancache_ops structure is never modified, so declare it as const. This also removes the __read_mostly declaration on the cleancache_ops variable declaration, since it seems redundant with const. Done with the help of Coccinelle. Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr> --- Not sure that the __read_mostly change is correct. Does it apply to the variable, or to what the variable points to? drivers/xen/tmem.c | 2 +- include/linux/cleancache.h | 2 +- mm/cleancache.c | 4 ++-- 3 files changed, 4 insertions(+), 4 deletions(-)