* Sasha Levin (levinsasha928@gmail.com) wrote:

On Mon, Oct 29, 2012 at 2:53 PM, Mathieu Desnoyers
[off-list ref] wrote:

quoted

* Sasha Levin (levinsasha928@gmail.com) wrote:

quoted

On Mon, Oct 29, 2012 at 2:31 PM, Josh Triplett [off-list ref] wrote:

quoted

On Mon, Oct 29, 2012 at 01:29:24PM -0400, Sasha Levin wrote:

quoted

On Mon, Oct 29, 2012 at 7:35 AM, Mathieu Desnoyers
[off-list ref] wrote:

quoted

* Sasha Levin (levinsasha928@gmail.com) wrote:

quoted

Switch tracepoints to use the new hashtable implementation. This reduces the amount of
generic unrelated code in the tracepoints.

Signed-off-by: Sasha Levin <redacted>
---
 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);

[...]

quoted

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

-- 
Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>