| Message ID | 1351450948-15618-6-git-send-email-levinsasha928@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
* Sasha Levin (levinsasha928@gmail.com) wrote: > Switch tracepoints to use the new hashtable implementation. This reduces the amount of > generic unrelated code in the tracepoints. > > Signed-off-by: Sasha Levin <levinsasha928@gmail.com> > --- > kernel/tracepoint.c | 27 +++++++++++---------------- > 1 file changed, 11 insertions(+), 16 deletions(-) > > diff --git a/kernel/tracepoint.c b/kernel/tracepoint.c > index d96ba22..854df92 100644 > --- a/kernel/tracepoint.c > +++ b/kernel/tracepoint.c > @@ -26,6 +26,7 @@ > #include <linux/slab.h> > #include <linux/sched.h> > #include <linux/static_key.h> > +#include <linux/hashtable.h> > > extern struct tracepoint * const __start___tracepoints_ptrs[]; > extern struct tracepoint * const __stop___tracepoints_ptrs[]; > @@ -49,8 +50,7 @@ static LIST_HEAD(tracepoint_module_list); > * Protected by tracepoints_mutex. > */ > #define TRACEPOINT_HASH_BITS 6 > -#define TRACEPOINT_TABLE_SIZE (1 << TRACEPOINT_HASH_BITS) > -static struct hlist_head tracepoint_table[TRACEPOINT_TABLE_SIZE]; > +static DEFINE_HASHTABLE(tracepoint_table, TRACEPOINT_HASH_BITS); > [...] > > @@ -722,6 +715,8 @@ struct notifier_block tracepoint_module_nb = { > > static int init_tracepoints(void) > { > + hash_init(tracepoint_table); > + > return register_module_notifier(&tracepoint_module_nb); > } > __initcall(init_tracepoints); So we have a hash table defined in .bss (therefore entirely initialized to NULL), and you add a call to "hash_init", which iterates on the whole array and initialize it to NULL (again) ? This extra initialization is redundant. I think it should be removed from here, and hashtable.h should document that hash_init() don't need to be called on zeroed memory (which includes static/global variables, kzalloc'd memory, etc). Thanks, Mathieu
On Mon, Oct 29, 2012 at 7:35 AM, Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote: > * Sasha Levin (levinsasha928@gmail.com) wrote: >> Switch tracepoints to use the new hashtable implementation. This reduces the amount of >> generic unrelated code in the tracepoints. >> >> Signed-off-by: Sasha Levin <levinsasha928@gmail.com> >> --- >> kernel/tracepoint.c | 27 +++++++++++---------------- >> 1 file changed, 11 insertions(+), 16 deletions(-) >> >> diff --git a/kernel/tracepoint.c b/kernel/tracepoint.c >> index d96ba22..854df92 100644 >> --- a/kernel/tracepoint.c >> +++ b/kernel/tracepoint.c >> @@ -26,6 +26,7 @@ >> #include <linux/slab.h> >> #include <linux/sched.h> >> #include <linux/static_key.h> >> +#include <linux/hashtable.h> >> >> extern struct tracepoint * const __start___tracepoints_ptrs[]; >> extern struct tracepoint * const __stop___tracepoints_ptrs[]; >> @@ -49,8 +50,7 @@ static LIST_HEAD(tracepoint_module_list); >> * Protected by tracepoints_mutex. >> */ >> #define TRACEPOINT_HASH_BITS 6 >> -#define TRACEPOINT_TABLE_SIZE (1 << TRACEPOINT_HASH_BITS) >> -static struct hlist_head tracepoint_table[TRACEPOINT_TABLE_SIZE]; >> +static DEFINE_HASHTABLE(tracepoint_table, TRACEPOINT_HASH_BITS); >> > [...] >> >> @@ -722,6 +715,8 @@ struct notifier_block tracepoint_module_nb = { >> >> static int init_tracepoints(void) >> { >> + hash_init(tracepoint_table); >> + >> return register_module_notifier(&tracepoint_module_nb); >> } >> __initcall(init_tracepoints); > > So we have a hash table defined in .bss (therefore entirely initialized > to NULL), and you add a call to "hash_init", which iterates on the whole > array and initialize it to NULL (again) ? > > This extra initialization is redundant. I think it should be removed > from here, and hashtable.h should document that hash_init() don't need > to be called on zeroed memory (which includes static/global variables, > kzalloc'd memory, etc). This was discussed in the previous series, the conclusion was to call hash_init() either way to keep the encapsulation and consistency. It's cheap enough and happens only once, so why not? Thanks, Sasha -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
* Sasha Levin (levinsasha928@gmail.com) wrote: > On Mon, Oct 29, 2012 at 7:35 AM, Mathieu Desnoyers > <mathieu.desnoyers@efficios.com> wrote: > > * Sasha Levin (levinsasha928@gmail.com) wrote: > >> Switch tracepoints to use the new hashtable implementation. This reduces the amount of > >> generic unrelated code in the tracepoints. > >> > >> Signed-off-by: Sasha Levin <levinsasha928@gmail.com> > >> --- > >> kernel/tracepoint.c | 27 +++++++++++---------------- > >> 1 file changed, 11 insertions(+), 16 deletions(-) > >> > >> diff --git a/kernel/tracepoint.c b/kernel/tracepoint.c > >> index d96ba22..854df92 100644 > >> --- a/kernel/tracepoint.c > >> +++ b/kernel/tracepoint.c > >> @@ -26,6 +26,7 @@ > >> #include <linux/slab.h> > >> #include <linux/sched.h> > >> #include <linux/static_key.h> > >> +#include <linux/hashtable.h> > >> > >> extern struct tracepoint * const __start___tracepoints_ptrs[]; > >> extern struct tracepoint * const __stop___tracepoints_ptrs[]; > >> @@ -49,8 +50,7 @@ static LIST_HEAD(tracepoint_module_list); > >> * Protected by tracepoints_mutex. > >> */ > >> #define TRACEPOINT_HASH_BITS 6 > >> -#define TRACEPOINT_TABLE_SIZE (1 << TRACEPOINT_HASH_BITS) > >> -static struct hlist_head tracepoint_table[TRACEPOINT_TABLE_SIZE]; > >> +static DEFINE_HASHTABLE(tracepoint_table, TRACEPOINT_HASH_BITS); > >> > > [...] > >> > >> @@ -722,6 +715,8 @@ struct notifier_block tracepoint_module_nb = { > >> > >> static int init_tracepoints(void) > >> { > >> + hash_init(tracepoint_table); > >> + > >> return register_module_notifier(&tracepoint_module_nb); > >> } > >> __initcall(init_tracepoints); > > > > So we have a hash table defined in .bss (therefore entirely initialized > > to NULL), and you add a call to "hash_init", which iterates on the whole > > array and initialize it to NULL (again) ? > > > > This extra initialization is redundant. I think it should be removed > > from here, and hashtable.h should document that hash_init() don't need > > to be called on zeroed memory (which includes static/global variables, > > kzalloc'd memory, etc). > > This was discussed in the previous series, the conclusion was to call > hash_init() either way to keep the encapsulation and consistency. Agreed, Thanks, Mathieu > > It's cheap enough and happens only once, so why not? > > > Thanks, > Sasha
On Mon, Oct 29, 2012 at 01:29:24PM -0400, Sasha Levin wrote: > On Mon, Oct 29, 2012 at 7:35 AM, Mathieu Desnoyers > <mathieu.desnoyers@efficios.com> wrote: > > * Sasha Levin (levinsasha928@gmail.com) wrote: > >> Switch tracepoints to use the new hashtable implementation. This reduces the amount of > >> generic unrelated code in the tracepoints. > >> > >> Signed-off-by: Sasha Levin <levinsasha928@gmail.com> > >> --- > >> kernel/tracepoint.c | 27 +++++++++++---------------- > >> 1 file changed, 11 insertions(+), 16 deletions(-) > >> > >> diff --git a/kernel/tracepoint.c b/kernel/tracepoint.c > >> index d96ba22..854df92 100644 > >> --- a/kernel/tracepoint.c > >> +++ b/kernel/tracepoint.c > >> @@ -26,6 +26,7 @@ > >> #include <linux/slab.h> > >> #include <linux/sched.h> > >> #include <linux/static_key.h> > >> +#include <linux/hashtable.h> > >> > >> extern struct tracepoint * const __start___tracepoints_ptrs[]; > >> extern struct tracepoint * const __stop___tracepoints_ptrs[]; > >> @@ -49,8 +50,7 @@ static LIST_HEAD(tracepoint_module_list); > >> * Protected by tracepoints_mutex. > >> */ > >> #define TRACEPOINT_HASH_BITS 6 > >> -#define TRACEPOINT_TABLE_SIZE (1 << TRACEPOINT_HASH_BITS) > >> -static struct hlist_head tracepoint_table[TRACEPOINT_TABLE_SIZE]; > >> +static DEFINE_HASHTABLE(tracepoint_table, TRACEPOINT_HASH_BITS); > >> > > [...] > >> > >> @@ -722,6 +715,8 @@ struct notifier_block tracepoint_module_nb = { > >> > >> static int init_tracepoints(void) > >> { > >> + hash_init(tracepoint_table); > >> + > >> return register_module_notifier(&tracepoint_module_nb); > >> } > >> __initcall(init_tracepoints); > > > > So we have a hash table defined in .bss (therefore entirely initialized > > to NULL), and you add a call to "hash_init", which iterates on the whole > > array and initialize it to NULL (again) ? > > > > This extra initialization is redundant. I think it should be removed > > from here, and hashtable.h should document that hash_init() don't need > > to be called on zeroed memory (which includes static/global variables, > > kzalloc'd memory, etc). > > This was discussed in the previous series, the conclusion was to call > hash_init() either way to keep the encapsulation and consistency. > > It's cheap enough and happens only once, so why not? Unnecessary work adds up. Better not to do it unnecessarily, even if by itself it doesn't cost that much. It doesn't seem that difficult for future fields to have 0 as their initialized state. - Josh Triplett -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Oct 29, 2012 at 2:31 PM, Josh Triplett <josh@joshtriplett.org> wrote: > On Mon, Oct 29, 2012 at 01:29:24PM -0400, Sasha Levin wrote: >> On Mon, Oct 29, 2012 at 7:35 AM, Mathieu Desnoyers >> <mathieu.desnoyers@efficios.com> wrote: >> > * Sasha Levin (levinsasha928@gmail.com) wrote: >> >> Switch tracepoints to use the new hashtable implementation. This reduces the amount of >> >> generic unrelated code in the tracepoints. >> >> >> >> Signed-off-by: Sasha Levin <levinsasha928@gmail.com> >> >> --- >> >> kernel/tracepoint.c | 27 +++++++++++---------------- >> >> 1 file changed, 11 insertions(+), 16 deletions(-) >> >> >> >> diff --git a/kernel/tracepoint.c b/kernel/tracepoint.c >> >> index d96ba22..854df92 100644 >> >> --- a/kernel/tracepoint.c >> >> +++ b/kernel/tracepoint.c >> >> @@ -26,6 +26,7 @@ >> >> #include <linux/slab.h> >> >> #include <linux/sched.h> >> >> #include <linux/static_key.h> >> >> +#include <linux/hashtable.h> >> >> >> >> extern struct tracepoint * const __start___tracepoints_ptrs[]; >> >> extern struct tracepoint * const __stop___tracepoints_ptrs[]; >> >> @@ -49,8 +50,7 @@ static LIST_HEAD(tracepoint_module_list); >> >> * Protected by tracepoints_mutex. >> >> */ >> >> #define TRACEPOINT_HASH_BITS 6 >> >> -#define TRACEPOINT_TABLE_SIZE (1 << TRACEPOINT_HASH_BITS) >> >> -static struct hlist_head tracepoint_table[TRACEPOINT_TABLE_SIZE]; >> >> +static DEFINE_HASHTABLE(tracepoint_table, TRACEPOINT_HASH_BITS); >> >> >> > [...] >> >> >> >> @@ -722,6 +715,8 @@ struct notifier_block tracepoint_module_nb = { >> >> >> >> static int init_tracepoints(void) >> >> { >> >> + hash_init(tracepoint_table); >> >> + >> >> return register_module_notifier(&tracepoint_module_nb); >> >> } >> >> __initcall(init_tracepoints); >> > >> > So we have a hash table defined in .bss (therefore entirely initialized >> > to NULL), and you add a call to "hash_init", which iterates on the whole >> > array and initialize it to NULL (again) ? >> > >> > This extra initialization is redundant. I think it should be removed >> > from here, and hashtable.h should document that hash_init() don't need >> > to be called on zeroed memory (which includes static/global variables, >> > kzalloc'd memory, etc). >> >> This was discussed in the previous series, the conclusion was to call >> hash_init() either way to keep the encapsulation and consistency. >> >> It's cheap enough and happens only once, so why not? > > Unnecessary work adds up. Better not to do it unnecessarily, even if by > itself it doesn't cost that much. > > It doesn't seem that difficult for future fields to have 0 as their > initialized state. Let's put it this way: hlist requires the user to initialize hlist head before usage, therefore as a hlist user, hashtable implementation must do that. We do it automatically when the hashtable user does DEFINE_HASHTABLE(), but we can't do that if he does DECLARE_HASHTABLE(). This means that the hashtable user must call hash_init() whenever he uses DECLARE_HASHTABLE() to create his hashtable. There are two options here, either we specify that hash_init() should only be called if DECLARE_HASHTABLE() was called, which is confusing, inconsistent and prone to errors, or we can just say that it should be called whenever a hashtable is used. The only way to work around it IMO is to get hlist to not require initializing before usage, and there are good reasons that that won't happen. Thanks, Sasha -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
* Sasha Levin (levinsasha928@gmail.com) wrote: > On Mon, Oct 29, 2012 at 2:31 PM, Josh Triplett <josh@joshtriplett.org> wrote: > > On Mon, Oct 29, 2012 at 01:29:24PM -0400, Sasha Levin wrote: > >> On Mon, Oct 29, 2012 at 7:35 AM, Mathieu Desnoyers > >> <mathieu.desnoyers@efficios.com> wrote: > >> > * Sasha Levin (levinsasha928@gmail.com) wrote: > >> >> Switch tracepoints to use the new hashtable implementation. This reduces the amount of > >> >> generic unrelated code in the tracepoints. > >> >> > >> >> Signed-off-by: Sasha Levin <levinsasha928@gmail.com> > >> >> --- > >> >> kernel/tracepoint.c | 27 +++++++++++---------------- > >> >> 1 file changed, 11 insertions(+), 16 deletions(-) > >> >> > >> >> diff --git a/kernel/tracepoint.c b/kernel/tracepoint.c > >> >> index d96ba22..854df92 100644 > >> >> --- a/kernel/tracepoint.c > >> >> +++ b/kernel/tracepoint.c > >> >> @@ -26,6 +26,7 @@ > >> >> #include <linux/slab.h> > >> >> #include <linux/sched.h> > >> >> #include <linux/static_key.h> > >> >> +#include <linux/hashtable.h> > >> >> > >> >> extern struct tracepoint * const __start___tracepoints_ptrs[]; > >> >> extern struct tracepoint * const __stop___tracepoints_ptrs[]; > >> >> @@ -49,8 +50,7 @@ static LIST_HEAD(tracepoint_module_list); > >> >> * Protected by tracepoints_mutex. > >> >> */ > >> >> #define TRACEPOINT_HASH_BITS 6 > >> >> -#define TRACEPOINT_TABLE_SIZE (1 << TRACEPOINT_HASH_BITS) > >> >> -static struct hlist_head tracepoint_table[TRACEPOINT_TABLE_SIZE]; > >> >> +static DEFINE_HASHTABLE(tracepoint_table, TRACEPOINT_HASH_BITS); > >> >> > >> > [...] > >> >> > >> >> @@ -722,6 +715,8 @@ struct notifier_block tracepoint_module_nb = { > >> >> > >> >> static int init_tracepoints(void) > >> >> { > >> >> + hash_init(tracepoint_table); > >> >> + > >> >> return register_module_notifier(&tracepoint_module_nb); > >> >> } > >> >> __initcall(init_tracepoints); > >> > > >> > So we have a hash table defined in .bss (therefore entirely initialized > >> > to NULL), and you add a call to "hash_init", which iterates on the whole > >> > array and initialize it to NULL (again) ? > >> > > >> > This extra initialization is redundant. I think it should be removed > >> > from here, and hashtable.h should document that hash_init() don't need > >> > to be called on zeroed memory (which includes static/global variables, > >> > kzalloc'd memory, etc). > >> > >> This was discussed in the previous series, the conclusion was to call > >> hash_init() either way to keep the encapsulation and consistency. > >> > >> It's cheap enough and happens only once, so why not? > > > > Unnecessary work adds up. Better not to do it unnecessarily, even if by > > itself it doesn't cost that much. > > > > It doesn't seem that difficult for future fields to have 0 as their > > initialized state. > > Let's put it this way: hlist requires the user to initialize hlist > head before usage, therefore as a hlist user, hashtable implementation > must do that. > > We do it automatically when the hashtable user does > DEFINE_HASHTABLE(), but we can't do that if he does > DECLARE_HASHTABLE(). This means that the hashtable user must call > hash_init() whenever he uses DECLARE_HASHTABLE() to create his > hashtable. > > There are two options here, either we specify that hash_init() should > only be called if DECLARE_HASHTABLE() was called, which is confusing, > inconsistent and prone to errors, or we can just say that it should be > called whenever a hashtable is used. > > The only way to work around it IMO is to get hlist to not require > initializing before usage, and there are good reasons that that won't > happen. Hrm, just a second here. The argument about hash_init being useful to add magic values in the future only works for the cases where a hash table is declared with DECLARE_HASHTABLE(). It's completely pointless with DEFINE_HASHTABLE(), because we could initialize any debugging variables from within DEFINE_HASHTABLE(). So I take my "Agreed" back. I disagree with initializing the hash table twice redundantly. There should be at least "DEFINE_HASHTABLE()" or a hash_init() (for DECLARE_HASHTABLE()), but not useless execution initialization on top of an already statically initialized hash table. Thanks, Mathieu
On Mon, Oct 29, 2012 at 02:53:19PM -0400, Mathieu Desnoyers wrote: > The argument about hash_init being useful to add magic values in the > future only works for the cases where a hash table is declared with > DECLARE_HASHTABLE(). It's completely pointless with DEFINE_HASHTABLE(), > because we could initialize any debugging variables from within > DEFINE_HASHTABLE(). You can do that with [0 .. HASH_SIZE - 1] initializer. Thanks.
On Mon, Oct 29, 2012 at 11:58:14AM -0700, Tejun Heo wrote: > On Mon, Oct 29, 2012 at 02:53:19PM -0400, Mathieu Desnoyers wrote: > > The argument about hash_init being useful to add magic values in the > > future only works for the cases where a hash table is declared with > > DECLARE_HASHTABLE(). It's completely pointless with DEFINE_HASHTABLE(), > > because we could initialize any debugging variables from within > > DEFINE_HASHTABLE(). > > You can do that with [0 .. HASH_SIZE - 1] initializer. And in general, let's please try not to do optimizations which are pointless. Just stick to the usual semantics. You have an abstract data structure - invoke the initializer before using it. Sure, optimize it if it shows up somewhere. And here, if we do the initializers properly, it shouldn't cause any more actual overhead - ie. DEFINE_HASHTABLE() will basicallly boil down to all zero assignments and the compiler will put the whole thing in .bss anyway. Thanks.
On Mon, Oct 29, 2012 at 2:53 PM, Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote: > * Sasha Levin (levinsasha928@gmail.com) wrote: >> On Mon, Oct 29, 2012 at 2:31 PM, Josh Triplett <josh@joshtriplett.org> wrote: >> > On Mon, Oct 29, 2012 at 01:29:24PM -0400, Sasha Levin wrote: >> >> On Mon, Oct 29, 2012 at 7:35 AM, Mathieu Desnoyers >> >> <mathieu.desnoyers@efficios.com> wrote: >> >> > * Sasha Levin (levinsasha928@gmail.com) wrote: >> >> >> Switch tracepoints to use the new hashtable implementation. This reduces the amount of >> >> >> generic unrelated code in the tracepoints. >> >> >> >> >> >> Signed-off-by: Sasha Levin <levinsasha928@gmail.com> >> >> >> --- >> >> >> kernel/tracepoint.c | 27 +++++++++++---------------- >> >> >> 1 file changed, 11 insertions(+), 16 deletions(-) >> >> >> >> >> >> diff --git a/kernel/tracepoint.c b/kernel/tracepoint.c >> >> >> index d96ba22..854df92 100644 >> >> >> --- a/kernel/tracepoint.c >> >> >> +++ b/kernel/tracepoint.c >> >> >> @@ -26,6 +26,7 @@ >> >> >> #include <linux/slab.h> >> >> >> #include <linux/sched.h> >> >> >> #include <linux/static_key.h> >> >> >> +#include <linux/hashtable.h> >> >> >> >> >> >> extern struct tracepoint * const __start___tracepoints_ptrs[]; >> >> >> extern struct tracepoint * const __stop___tracepoints_ptrs[]; >> >> >> @@ -49,8 +50,7 @@ static LIST_HEAD(tracepoint_module_list); >> >> >> * Protected by tracepoints_mutex. >> >> >> */ >> >> >> #define TRACEPOINT_HASH_BITS 6 >> >> >> -#define TRACEPOINT_TABLE_SIZE (1 << TRACEPOINT_HASH_BITS) >> >> >> -static struct hlist_head tracepoint_table[TRACEPOINT_TABLE_SIZE]; >> >> >> +static DEFINE_HASHTABLE(tracepoint_table, TRACEPOINT_HASH_BITS); >> >> >> >> >> > [...] >> >> >> >> >> >> @@ -722,6 +715,8 @@ struct notifier_block tracepoint_module_nb = { >> >> >> >> >> >> static int init_tracepoints(void) >> >> >> { >> >> >> + hash_init(tracepoint_table); >> >> >> + >> >> >> return register_module_notifier(&tracepoint_module_nb); >> >> >> } >> >> >> __initcall(init_tracepoints); >> >> > >> >> > So we have a hash table defined in .bss (therefore entirely initialized >> >> > to NULL), and you add a call to "hash_init", which iterates on the whole >> >> > array and initialize it to NULL (again) ? >> >> > >> >> > This extra initialization is redundant. I think it should be removed >> >> > from here, and hashtable.h should document that hash_init() don't need >> >> > to be called on zeroed memory (which includes static/global variables, >> >> > kzalloc'd memory, etc). >> >> >> >> This was discussed in the previous series, the conclusion was to call >> >> hash_init() either way to keep the encapsulation and consistency. >> >> >> >> It's cheap enough and happens only once, so why not? >> > >> > Unnecessary work adds up. Better not to do it unnecessarily, even if by >> > itself it doesn't cost that much. >> > >> > It doesn't seem that difficult for future fields to have 0 as their >> > initialized state. >> >> Let's put it this way: hlist requires the user to initialize hlist >> head before usage, therefore as a hlist user, hashtable implementation >> must do that. >> >> We do it automatically when the hashtable user does >> DEFINE_HASHTABLE(), but we can't do that if he does >> DECLARE_HASHTABLE(). This means that the hashtable user must call >> hash_init() whenever he uses DECLARE_HASHTABLE() to create his >> hashtable. >> >> There are two options here, either we specify that hash_init() should >> only be called if DECLARE_HASHTABLE() was called, which is confusing, >> inconsistent and prone to errors, or we can just say that it should be >> called whenever a hashtable is used. >> >> The only way to work around it IMO is to get hlist to not require >> initializing before usage, and there are good reasons that that won't >> happen. > > Hrm, just a second here. > > The argument about hash_init being useful to add magic values in the > future only works for the cases where a hash table is declared with > DECLARE_HASHTABLE(). It's completely pointless with DEFINE_HASHTABLE(), > because we could initialize any debugging variables from within > DEFINE_HASHTABLE(). > > So I take my "Agreed" back. I disagree with initializing the hash table > twice redundantly. There should be at least "DEFINE_HASHTABLE()" or a > hash_init() (for DECLARE_HASHTABLE()), but not useless execution > initialization on top of an already statically initialized hash table. The "magic values" argument was used to point out that some sort of initialization *must* occur, either by hash_init() or by a proper initialization in DEFINE_HASHTABLE(), and we can't simply memset() it to 0. It appears that we all agree on that. The other thing is whether hash_init() should be called for hashtables that were created with DEFINE_HASHTABLE(). That point was raised by Neil Brown last time this series went around, and it seems that no one objected to the point that it should be consistent across the code. Even if we ignore hash_init() being mostly optimized out, is it really worth it taking the risk that some future patch would move a hashtable that user DEFINE_HASHTABLE() into a struct and will start using DECLARE_HASHTABLE() and forgetting to initialize it, for example? Thanks, Sasha -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
* Tejun Heo (tj@kernel.org) wrote: > On Mon, Oct 29, 2012 at 11:58:14AM -0700, Tejun Heo wrote: > > On Mon, Oct 29, 2012 at 02:53:19PM -0400, Mathieu Desnoyers wrote: > > > The argument about hash_init being useful to add magic values in the > > > future only works for the cases where a hash table is declared with > > > DECLARE_HASHTABLE(). It's completely pointless with DEFINE_HASHTABLE(), > > > because we could initialize any debugging variables from within > > > DEFINE_HASHTABLE(). > > > > You can do that with [0 .. HASH_SIZE - 1] initializer. > > And in general, let's please try not to do optimizations which are > pointless. Just stick to the usual semantics. You have an abstract > data structure - invoke the initializer before using it. Sure, > optimize it if it shows up somewhere. And here, if we do the > initializers properly, it shouldn't cause any more actual overhead - > ie. DEFINE_HASHTABLE() will basicallly boil down to all zero > assignments and the compiler will put the whole thing in .bss anyway. Yes, agreed. I was going too far in optimization land by proposing assumptions on zeroed memory. All I actually really care about is that we don't end up calling hash_init() on a statically defined (and thus already initialized) hash table. Thanks, Mathieu
On Mon, Oct 29, 2012 at 03:09:36PM -0400, Sasha Levin wrote: > The other thing is whether hash_init() should be called for hashtables > that were created with DEFINE_HASHTABLE(). That point was raised by > Neil Brown last time this series went around, and it seems that no one > objected to the point that it should be consistent across the code. Hmmm? If something is DEFINE_XXX()'d, you definitely shouldn't be calling XXX_init() on it. That's how it is with most other abstract data types and you need *VERY* strong rationale to deviate from that. Thanks.
* Sasha Levin (levinsasha928@gmail.com) wrote: > On Mon, Oct 29, 2012 at 2:53 PM, Mathieu Desnoyers > <mathieu.desnoyers@efficios.com> wrote: > > * Sasha Levin (levinsasha928@gmail.com) wrote: > >> On Mon, Oct 29, 2012 at 2:31 PM, Josh Triplett <josh@joshtriplett.org> wrote: > >> > On Mon, Oct 29, 2012 at 01:29:24PM -0400, Sasha Levin wrote: > >> >> On Mon, Oct 29, 2012 at 7:35 AM, Mathieu Desnoyers > >> >> <mathieu.desnoyers@efficios.com> wrote: > >> >> > * Sasha Levin (levinsasha928@gmail.com) wrote: > >> >> >> Switch tracepoints to use the new hashtable implementation. This reduces the amount of > >> >> >> generic unrelated code in the tracepoints. > >> >> >> > >> >> >> Signed-off-by: Sasha Levin <levinsasha928@gmail.com> > >> >> >> --- > >> >> >> kernel/tracepoint.c | 27 +++++++++++---------------- > >> >> >> 1 file changed, 11 insertions(+), 16 deletions(-) > >> >> >> > >> >> >> diff --git a/kernel/tracepoint.c b/kernel/tracepoint.c > >> >> >> index d96ba22..854df92 100644 > >> >> >> --- a/kernel/tracepoint.c > >> >> >> +++ b/kernel/tracepoint.c > >> >> >> @@ -26,6 +26,7 @@ > >> >> >> #include <linux/slab.h> > >> >> >> #include <linux/sched.h> > >> >> >> #include <linux/static_key.h> > >> >> >> +#include <linux/hashtable.h> > >> >> >> > >> >> >> extern struct tracepoint * const __start___tracepoints_ptrs[]; > >> >> >> extern struct tracepoint * const __stop___tracepoints_ptrs[]; > >> >> >> @@ -49,8 +50,7 @@ static LIST_HEAD(tracepoint_module_list); > >> >> >> * Protected by tracepoints_mutex. > >> >> >> */ > >> >> >> #define TRACEPOINT_HASH_BITS 6 > >> >> >> -#define TRACEPOINT_TABLE_SIZE (1 << TRACEPOINT_HASH_BITS) > >> >> >> -static struct hlist_head tracepoint_table[TRACEPOINT_TABLE_SIZE]; > >> >> >> +static DEFINE_HASHTABLE(tracepoint_table, TRACEPOINT_HASH_BITS); > >> >> >> > >> >> > [...] > >> >> >> > >> >> >> @@ -722,6 +715,8 @@ struct notifier_block tracepoint_module_nb = { > >> >> >> > >> >> >> static int init_tracepoints(void) > >> >> >> { > >> >> >> + hash_init(tracepoint_table); > >> >> >> + > >> >> >> return register_module_notifier(&tracepoint_module_nb); > >> >> >> } > >> >> >> __initcall(init_tracepoints); > >> >> > > >> >> > So we have a hash table defined in .bss (therefore entirely initialized > >> >> > to NULL), and you add a call to "hash_init", which iterates on the whole > >> >> > array and initialize it to NULL (again) ? > >> >> > > >> >> > This extra initialization is redundant. I think it should be removed > >> >> > from here, and hashtable.h should document that hash_init() don't need > >> >> > to be called on zeroed memory (which includes static/global variables, > >> >> > kzalloc'd memory, etc). > >> >> > >> >> This was discussed in the previous series, the conclusion was to call > >> >> hash_init() either way to keep the encapsulation and consistency. > >> >> > >> >> It's cheap enough and happens only once, so why not? > >> > > >> > Unnecessary work adds up. Better not to do it unnecessarily, even if by > >> > itself it doesn't cost that much. > >> > > >> > It doesn't seem that difficult for future fields to have 0 as their > >> > initialized state. > >> > >> Let's put it this way: hlist requires the user to initialize hlist > >> head before usage, therefore as a hlist user, hashtable implementation > >> must do that. > >> > >> We do it automatically when the hashtable user does > >> DEFINE_HASHTABLE(), but we can't do that if he does > >> DECLARE_HASHTABLE(). This means that the hashtable user must call > >> hash_init() whenever he uses DECLARE_HASHTABLE() to create his > >> hashtable. > >> > >> There are two options here, either we specify that hash_init() should > >> only be called if DECLARE_HASHTABLE() was called, which is confusing, > >> inconsistent and prone to errors, or we can just say that it should be > >> called whenever a hashtable is used. > >> > >> The only way to work around it IMO is to get hlist to not require > >> initializing before usage, and there are good reasons that that won't > >> happen. > > > > Hrm, just a second here. > > > > The argument about hash_init being useful to add magic values in the > > future only works for the cases where a hash table is declared with > > DECLARE_HASHTABLE(). It's completely pointless with DEFINE_HASHTABLE(), > > because we could initialize any debugging variables from within > > DEFINE_HASHTABLE(). > > > > So I take my "Agreed" back. I disagree with initializing the hash table > > twice redundantly. There should be at least "DEFINE_HASHTABLE()" or a > > hash_init() (for DECLARE_HASHTABLE()), but not useless execution > > initialization on top of an already statically initialized hash table. > > The "magic values" argument was used to point out that some sort of > initialization *must* occur, either by hash_init() or by a proper > initialization in DEFINE_HASHTABLE(), and we can't simply memset() it > to 0. It appears that we all agree on that. Yes. > The other thing is whether hash_init() should be called for hashtables > that were created with DEFINE_HASHTABLE(). That point was raised by > Neil Brown last time this series went around, and it seems that no one > objected to the point that it should be consistent across the code. I was probably busy in the San Diego area at that time, or preparing for it, sorry! :) > > Even if we ignore hash_init() being mostly optimized out, is it really > worth it taking the risk that some future patch would move a hashtable > that user DEFINE_HASHTABLE() into a struct and will start using > DECLARE_HASHTABLE() and forgetting to initialize it, for example? There is a saying that with "if"s, we could put Paris in a bottle. ;) Please have a look at "linux/wait.h", where if a wait queue is defined with DEFINE_*(), there is just no need to initialize it at runtime. There are plenty other kernel headers that do the same. I don't see why hashtable.h should be different. Thanks, Mathieu
On Mon, Oct 29, 2012 at 3:12 PM, Tejun Heo <tj@kernel.org> wrote: > On Mon, Oct 29, 2012 at 03:09:36PM -0400, Sasha Levin wrote: >> The other thing is whether hash_init() should be called for hashtables >> that were created with DEFINE_HASHTABLE(). That point was raised by >> Neil Brown last time this series went around, and it seems that no one >> objected to the point that it should be consistent across the code. > > Hmmm? If something is DEFINE_XXX()'d, you definitely shouldn't be > calling XXX_init() on it. That's how it is with most other abstract > data types and you need *VERY* strong rationale to deviate from that. Neil Brown raised that point last time that this series went around, and suggested that this should be consistent and hash_init() would appear everywhere, even if DEFINE_HASHTABLE() was used. Since no one objected to that I thought we're going with that. I'll chalk it up to me getting confused :) Thanks, Sasha -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/kernel/tracepoint.c b/kernel/tracepoint.c index d96ba22..854df92 100644 --- a/kernel/tracepoint.c +++ b/kernel/tracepoint.c @@ -26,6 +26,7 @@ #include <linux/slab.h> #include <linux/sched.h> #include <linux/static_key.h> +#include <linux/hashtable.h> extern struct tracepoint * const __start___tracepoints_ptrs[]; extern struct tracepoint * const __stop___tracepoints_ptrs[]; @@ -49,8 +50,7 @@ static LIST_HEAD(tracepoint_module_list); * Protected by tracepoints_mutex. */ #define TRACEPOINT_HASH_BITS 6 -#define TRACEPOINT_TABLE_SIZE (1 << TRACEPOINT_HASH_BITS) -static struct hlist_head tracepoint_table[TRACEPOINT_TABLE_SIZE]; +static DEFINE_HASHTABLE(tracepoint_table, TRACEPOINT_HASH_BITS); /* * Note about RCU : @@ -191,16 +191,15 @@ tracepoint_entry_remove_probe(struct tracepoint_entry *entry, */ static struct tracepoint_entry *get_tracepoint(const char *name) { - struct hlist_head *head; struct hlist_node *node; struct tracepoint_entry *e; u32 hash = jhash(name, strlen(name), 0); - head = &tracepoint_table[hash & (TRACEPOINT_TABLE_SIZE - 1)]; - hlist_for_each_entry(e, node, head, hlist) { + hash_for_each_possible(tracepoint_table, e, node, hlist, hash) { if (!strcmp(name, e->name)) return e; } + return NULL; } @@ -210,19 +209,13 @@ static struct tracepoint_entry *get_tracepoint(const char *name) */ static struct tracepoint_entry *add_tracepoint(const char *name) { - struct hlist_head *head; - struct hlist_node *node; struct tracepoint_entry *e; size_t name_len = strlen(name) + 1; u32 hash = jhash(name, name_len-1, 0); - head = &tracepoint_table[hash & (TRACEPOINT_TABLE_SIZE - 1)]; - hlist_for_each_entry(e, node, head, hlist) { - if (!strcmp(name, e->name)) { - printk(KERN_NOTICE - "tracepoint %s busy\n", name); - return ERR_PTR(-EEXIST); /* Already there */ - } + if (get_tracepoint(name)) { + printk(KERN_NOTICE "tracepoint %s busy\n", name); + return ERR_PTR(-EEXIST); /* Already there */ } /* * Using kmalloc here to allocate a variable length element. Could @@ -234,7 +227,7 @@ static struct tracepoint_entry *add_tracepoint(const char *name) memcpy(&e->name[0], name, name_len); e->funcs = NULL; e->refcount = 0; - hlist_add_head(&e->hlist, head); + hash_add(tracepoint_table, &e->hlist, hash); return e; } @@ -244,7 +237,7 @@ static struct tracepoint_entry *add_tracepoint(const char *name) */ static inline void remove_tracepoint(struct tracepoint_entry *e) { - hlist_del(&e->hlist); + hash_del(&e->hlist); kfree(e); } @@ -722,6 +715,8 @@ struct notifier_block tracepoint_module_nb = { static int init_tracepoints(void) { + hash_init(tracepoint_table); + return register_module_notifier(&tracepoint_module_nb); } __initcall(init_tracepoints);
Switch tracepoints to use the new hashtable implementation. This reduces the amount of generic unrelated code in the tracepoints. Signed-off-by: Sasha Levin <levinsasha928@gmail.com> --- kernel/tracepoint.c | 27 +++++++++++---------------- 1 file changed, 11 insertions(+), 16 deletions(-)