* [PATCH] module: struct module_ref should contains long fields
@ 2011-12-16 5:07 Eric Dumazet
2011-12-16 15:54 ` Tejun Heo
0 siblings, 1 reply; 6+ messages in thread
From: Eric Dumazet @ 2011-12-16 5:07 UTC (permalink / raw)
To: Rusty Russell; +Cc: linux-kernel, Tejun Heo, David Miller, Robin Holt
module_ref contains two "unsigned int" fields.
Thats now too small, since some machines can open more than 2^31 files.
Check commit 518de9b39e8 (fs: allow for more than 2^31 files) for
reference.
We can add an aligned(2 * sizeof(unsigned long)) attribute to force
alloc_percpu() allocating module_ref areas in single cache lines.
Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
CC: Rusty Russell <rusty@rustcorp.com.au>
CC: Tejun Heo <tj@kernel.org>
CC: Robin Holt <holt@sgi.com>
CC: David Miller <davem@davemloft.net>
---
include/linux/module.h | 12 +++++++-----
kernel/debug/kdb/kdb_main.c | 2 +-
kernel/module.c | 8 ++++----
3 files changed, 12 insertions(+), 10 deletions(-)
diff --git a/include/linux/module.h b/include/linux/module.h
index 3cb7839..2ae2bcf 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -205,6 +205,11 @@ enum module_state
MODULE_STATE_GOING,
};
+struct module_ref {
+ unsigned long incs;
+ unsigned long decs;
+} __attribute((aligned(2 * sizeof(unsigned long))));
+
struct module
{
enum module_state state;
@@ -347,10 +352,7 @@ struct module
/* Destruction function. */
void (*exit)(void);
- struct module_ref {
- unsigned int incs;
- unsigned int decs;
- } __percpu *refptr;
+ struct module_ref __percpu *refptr;
#endif
#ifdef CONFIG_CONSTRUCTORS
@@ -434,7 +436,7 @@ extern void __module_put_and_exit(struct module *mod, long code)
#define module_put_and_exit(code) __module_put_and_exit(THIS_MODULE, code);
#ifdef CONFIG_MODULE_UNLOAD
-unsigned int module_refcount(struct module *mod);
+unsigned long module_refcount(struct module *mod);
void __symbol_put(const char *symbol);
#define symbol_put(x) __symbol_put(MODULE_SYMBOL_PREFIX #x)
void symbol_put_addr(void *addr);
diff --git a/kernel/debug/kdb/kdb_main.c b/kernel/debug/kdb/kdb_main.c
index 63786e7..e2ae734 100644
--- a/kernel/debug/kdb/kdb_main.c
+++ b/kernel/debug/kdb/kdb_main.c
@@ -1982,7 +1982,7 @@ static int kdb_lsmod(int argc, const char **argv)
kdb_printf("%-20s%8u 0x%p ", mod->name,
mod->core_size, (void *)mod);
#ifdef CONFIG_MODULE_UNLOAD
- kdb_printf("%4d ", module_refcount(mod));
+ kdb_printf("%4ld ", module_refcount(mod));
#endif
if (mod->state == MODULE_STATE_GOING)
kdb_printf(" (Unloading)");
diff --git a/kernel/module.c b/kernel/module.c
index 178333c..ef05f7e 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -726,9 +726,9 @@ static int try_stop_module(struct module *mod, int flags, int *forced)
}
}
-unsigned int module_refcount(struct module *mod)
+unsigned long module_refcount(struct module *mod)
{
- unsigned int incs = 0, decs = 0;
+ unsigned long incs = 0, decs = 0;
int cpu;
for_each_possible_cpu(cpu)
@@ -854,7 +854,7 @@ static inline void print_unload_info(struct seq_file *m, struct module *mod)
struct module_use *use;
int printed_something = 0;
- seq_printf(m, " %u ", module_refcount(mod));
+ seq_printf(m, " %lu ", module_refcount(mod));
/* Always include a trailing , so userspace can differentiate
between this and the old multi-field proc format. */
@@ -904,7 +904,7 @@ EXPORT_SYMBOL_GPL(symbol_put_addr);
static ssize_t show_refcnt(struct module_attribute *mattr,
struct module_kobject *mk, char *buffer)
{
- return sprintf(buffer, "%u\n", module_refcount(mk->mod));
+ return sprintf(buffer, "%lu\n", module_refcount(mk->mod));
}
static struct module_attribute refcnt = {
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] module: struct module_ref should contains long fields
2011-12-16 5:07 [PATCH] module: struct module_ref should contains long fields Eric Dumazet
@ 2011-12-16 15:54 ` Tejun Heo
2011-12-16 16:01 ` Eric Dumazet
0 siblings, 1 reply; 6+ messages in thread
From: Tejun Heo @ 2011-12-16 15:54 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Rusty Russell, linux-kernel, David Miller, Robin Holt
Hello, Eric.
On Fri, Dec 16, 2011 at 06:07:37AM +0100, Eric Dumazet wrote:
> +struct module_ref {
> + unsigned long incs;
> + unsigned long decs;
> +} __attribute((aligned(2 * sizeof(unsigned long))));
Why not ____cacheline_aligned? Another thing is that for percpu
memory, packing could be better or at least shouldn't be worse.
Percpu area usages are likely to be local so one major benefit of
cacheline alignment - avoiding cacheline pingpong - goes away. The
constant is called SMP_CACHE_BYTES after all.
Thanks.
--
tejun
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] module: struct module_ref should contains long fields
2011-12-16 15:54 ` Tejun Heo
@ 2011-12-16 16:01 ` Eric Dumazet
2011-12-16 16:05 ` Tejun Heo
0 siblings, 1 reply; 6+ messages in thread
From: Eric Dumazet @ 2011-12-16 16:01 UTC (permalink / raw)
To: Tejun Heo; +Cc: Rusty Russell, linux-kernel, David Miller, Robin Holt
Le vendredi 16 décembre 2011 à 07:54 -0800, Tejun Heo a écrit :
> Hello, Eric.
>
> On Fri, Dec 16, 2011 at 06:07:37AM +0100, Eric Dumazet wrote:
> > +struct module_ref {
> > + unsigned long incs;
> > + unsigned long decs;
> > +} __attribute((aligned(2 * sizeof(unsigned long))));
>
> Why not ____cacheline_aligned? Another thing is that for percpu
> memory, packing could be better or at least shouldn't be worse.
> Percpu area usages are likely to be local so one major benefit of
> cacheline alignment - avoiding cacheline pingpong - goes away. The
> constant is called SMP_CACHE_BYTES after all.
Its percpu data, there is no need to waste a full cache line per cpu for
this.
This is only a hint to make sure one single cache line is touched if a
thread increments/decrements a module refcount, with no memory extra
cost : We play with the alignement of this 8 or 16 bytes block.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] module: struct module_ref should contains long fields
2011-12-16 16:01 ` Eric Dumazet
@ 2011-12-16 16:05 ` Tejun Heo
2011-12-16 16:36 ` Eric Dumazet
0 siblings, 1 reply; 6+ messages in thread
From: Tejun Heo @ 2011-12-16 16:05 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Rusty Russell, linux-kernel, David Miller, Robin Holt
Hello,
On Fri, Dec 16, 2011 at 8:01 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> Its percpu data, there is no need to waste a full cache line per cpu for
> this.
Hmmm.... okay. I just don't like seeing arbitrary alignment there.
Can you please add some comment explaining why the unusual alignment
is used there?
Thanks.
--
tejun
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] module: struct module_ref should contains long fields
2011-12-16 16:05 ` Tejun Heo
@ 2011-12-16 16:36 ` Eric Dumazet
2011-12-19 5:48 ` Rusty Russell
0 siblings, 1 reply; 6+ messages in thread
From: Eric Dumazet @ 2011-12-16 16:36 UTC (permalink / raw)
To: Tejun Heo; +Cc: Rusty Russell, linux-kernel, David Miller, Robin Holt
Le vendredi 16 décembre 2011 à 08:05 -0800, Tejun Heo a écrit :
> Hello,
>
> On Fri, Dec 16, 2011 at 8:01 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> > Its percpu data, there is no need to waste a full cache line per cpu for
> > this.
>
> Hmmm.... okay. I just don't like seeing arbitrary alignment there.
> Can you please add some comment explaining why the unusual alignment
> is used there?
Yep, thanks for the review !
[PATCH v2] module: struct module_ref should contains long fields
module_ref contains two "unsigned int" fields.
Thats now too small, since some machines can open more than 2^32 files.
Check commit 518de9b39e8 (fs: allow for more than 2^31 files) for
reference.
We can add an aligned(2 * sizeof(unsigned long)) attribute to force
alloc_percpu() allocating module_ref areas in single cache lines.
Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
CC: Rusty Russell <rusty@rustcorp.com.au>
CC: Tejun Heo <tj@kernel.org>
CC: Robin Holt <holt@sgi.com>
CC: David Miller <davem@davemloft.net>
---
V2: added kerneldoc to struct module_ref
include/linux/module.h | 21 ++++++++++++++++-----
kernel/debug/kdb/kdb_main.c | 2 +-
kernel/module.c | 8 ++++----
3 files changed, 21 insertions(+), 10 deletions(-)
diff --git a/include/linux/module.h b/include/linux/module.h
index 3cb7839..4598bf0 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -205,6 +205,20 @@ enum module_state
MODULE_STATE_GOING,
};
+/**
+ * struct module_ref - per cpu module reference counts
+ * @incs: number of module get on this cpu
+ * @decs: number of module put on this cpu
+ *
+ * We force an alignment on 8 or 16 bytes, so that alloc_percpu()
+ * put @incs/@decs in same cache line, with no extra memory cost,
+ * since alloc_percpu() is fine grained.
+ */
+struct module_ref {
+ unsigned long incs;
+ unsigned long decs;
+} __attribute((aligned(2 * sizeof(unsigned long))));
+
struct module
{
enum module_state state;
@@ -347,10 +361,7 @@ struct module
/* Destruction function. */
void (*exit)(void);
- struct module_ref {
- unsigned int incs;
- unsigned int decs;
- } __percpu *refptr;
+ struct module_ref __percpu *refptr;
#endif
#ifdef CONFIG_CONSTRUCTORS
@@ -434,7 +445,7 @@ extern void __module_put_and_exit(struct module *mod, long code)
#define module_put_and_exit(code) __module_put_and_exit(THIS_MODULE, code);
#ifdef CONFIG_MODULE_UNLOAD
-unsigned int module_refcount(struct module *mod);
+unsigned long module_refcount(struct module *mod);
void __symbol_put(const char *symbol);
#define symbol_put(x) __symbol_put(MODULE_SYMBOL_PREFIX #x)
void symbol_put_addr(void *addr);
diff --git a/kernel/debug/kdb/kdb_main.c b/kernel/debug/kdb/kdb_main.c
index 63786e7..e2ae734 100644
--- a/kernel/debug/kdb/kdb_main.c
+++ b/kernel/debug/kdb/kdb_main.c
@@ -1982,7 +1982,7 @@ static int kdb_lsmod(int argc, const char **argv)
kdb_printf("%-20s%8u 0x%p ", mod->name,
mod->core_size, (void *)mod);
#ifdef CONFIG_MODULE_UNLOAD
- kdb_printf("%4d ", module_refcount(mod));
+ kdb_printf("%4ld ", module_refcount(mod));
#endif
if (mod->state == MODULE_STATE_GOING)
kdb_printf(" (Unloading)");
diff --git a/kernel/module.c b/kernel/module.c
index 178333c..ef05f7e 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -726,9 +726,9 @@ static int try_stop_module(struct module *mod, int flags, int *forced)
}
}
-unsigned int module_refcount(struct module *mod)
+unsigned long module_refcount(struct module *mod)
{
- unsigned int incs = 0, decs = 0;
+ unsigned long incs = 0, decs = 0;
int cpu;
for_each_possible_cpu(cpu)
@@ -854,7 +854,7 @@ static inline void print_unload_info(struct seq_file *m, struct module *mod)
struct module_use *use;
int printed_something = 0;
- seq_printf(m, " %u ", module_refcount(mod));
+ seq_printf(m, " %lu ", module_refcount(mod));
/* Always include a trailing , so userspace can differentiate
between this and the old multi-field proc format. */
@@ -904,7 +904,7 @@ EXPORT_SYMBOL_GPL(symbol_put_addr);
static ssize_t show_refcnt(struct module_attribute *mattr,
struct module_kobject *mk, char *buffer)
{
- return sprintf(buffer, "%u\n", module_refcount(mk->mod));
+ return sprintf(buffer, "%lu\n", module_refcount(mk->mod));
}
static struct module_attribute refcnt = {
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] module: struct module_ref should contains long fields
2011-12-16 16:36 ` Eric Dumazet
@ 2011-12-19 5:48 ` Rusty Russell
0 siblings, 0 replies; 6+ messages in thread
From: Rusty Russell @ 2011-12-19 5:48 UTC (permalink / raw)
To: Eric Dumazet, Tejun Heo; +Cc: linux-kernel, David Miller, Robin Holt
On Fri, 16 Dec 2011 17:36:30 +0100, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> Le vendredi 16 décembre 2011 à 08:05 -0800, Tejun Heo a écrit :
> > Hello,
> >
> > On Fri, Dec 16, 2011 at 8:01 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> > > Its percpu data, there is no need to waste a full cache line per cpu for
> > > this.
> >
> > Hmmm.... okay. I just don't like seeing arbitrary alignment there.
> > Can you please add some comment explaining why the unusual alignment
> > is used there?
>
> Yep, thanks for the review !
Thanks, applied!
Cheers,
Rusty.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2011-12-19 6:08 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-12-16 5:07 [PATCH] module: struct module_ref should contains long fields Eric Dumazet
2011-12-16 15:54 ` Tejun Heo
2011-12-16 16:01 ` Eric Dumazet
2011-12-16 16:05 ` Tejun Heo
2011-12-16 16:36 ` Eric Dumazet
2011-12-19 5:48 ` Rusty Russell
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).