LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Mathieu Desnoyers <mathieu.desnoyers@polymtl.ca>
To: Rusty Russell <rusty@rustcorp.com.au>
Cc: akpm@linux-foundation.org, linux-kernel@vger.kernel.org
Subject: Re: [patch 1/4] Linux Kernel Markers - Architecture Independent Code
Date: Fri, 24 Aug 2007 12:26:56 -0400	[thread overview]
Message-ID: <20070824162656.GC21226@Krystal> (raw)
In-Reply-To: <1187656987.19435.163.camel@localhost.localdomain>

* Rusty Russell (rusty@rustcorp.com.au) wrote:
> On Mon, 2007-08-20 at 16:27 -0400, Mathieu Desnoyers wrote:
> > The marker activation functions sits in kernel/marker.c. A hash table is used
> > to keep track of the registered probes and armed markers, so the markers within
> > a newly loaded module that should be active can be activated at module load
> > time.
> 
> Hi Mathieu!
> 
> 	Just reading through this patch, a couple of comments:
> 

Hi Rusty,

> > +/* To be used for string format validity checking with gcc */
> > +static inline void __mark_check_format(const char *fmt, ...)
> > +		__attribute__ ((format (printf, 1, 2)));
> > +static inline void __mark_check_format(const char *fmt, ...) { }
> 
> If you place the __attribute__() before the function name, you can do
> this in the definition.
> 

Ok, will fix.

> > ===================================================================
> > --- linux-2.6-lttng.orig/kernel/module.c	2007-08-10 19:44:18.000000000 -0400
> > +++ linux-2.6-lttng/kernel/module.c	2007-08-10 23:54:38.000000000 -0400
> > @@ -1980,6 +1986,10 @@ static struct module *load_module(void _
> >  			sechdrs[immediateindex].sh_size / sizeof(*mod->immediate);
> >  	}
> >  #endif
> > +	if (markersindex)
> > +		sechdrs[markersindex].sh_flags |= SHF_ALLOC;
> > +	if (markersstringsindex)
> > +		sechdrs[markersstringsindex].sh_flags |= SHF_ALLOC;
> >  
> 
> Perhaps I'm missing something, but I don't see why these sections
> wouldn't be SHF_ALLOC already.
> 

Declaring variables with __attribute__((section("__markers_strings")))
will likely put them in an allocated section, you are right. Will fix.

The same applies to immediate values with .section __immediate, \"a\",
@progbits, which is explicitely allocated.

> >  	mod->unused_syms = (void *)sechdrs[unusedindex].sh_addr;
> >  	if (unusedcrcindex)
> > @@ -2021,6 +2031,13 @@ static struct module *load_module(void _
> >  		if (err < 0)
> >  			goto cleanup;
> >  	}
> > +#ifdef CONFIG_MARKERS
> > +	if (markersindex) {
> > +		mod->markers = (void *)sechdrs[markersindex].sh_addr;
> > +		mod->num_markers =
> > +			sechdrs[markersindex].sh_size / sizeof(*mod->markers);
> > +	}
> > +#endif
> 
> Because of the wonders of ELF, section 0 has sh_addr and sh_size 0.  So
> the if (markersindex) is unnecessary here.
> 

Ok. Will apply to immediate values too.

> > +/*
> > + * Get marker if the marker is present in the marker hash table.
> > + * Must be called with markers_mutex held.
> > + * Returns NULL if not present.
> > + */
> > +static struct marker_entry *_get_marker(const char *name)
> 
> You seem to really enjoy underscores, yet I'm having trouble
> understanding why this would have an underscore in it.
> 

Just they are internal functiona meant to be called with markers_mutex
held. But I guess having a static prefix and not being exported is
enough. Will remove. I'll just keep
_marker_update_probes/marker_update_probes to differentiate between
locked/non-locked version.

> > +{
> > +	struct hlist_head *head;
> > +	struct hlist_node *node;
> > +	struct marker_entry *e;
> > +	size_t len = strlen(name) + 1;
> > +	u32 hash = jhash(name, len-1, 0);
> > +
> > +	head = &marker_table[hash & ((1 << MARKER_HASH_BITS)-1)];
> > +	hlist_for_each_entry(e, node, head, hlist) {
> > +		if (!strcmp(name, e->name))
> > +			return e;
> > +	}
> > +	return NULL;
> > +}
> 
> OK, don't understand the strlen, len, len-1 dance here?
> 

Let's say we have abc\0 for marker name as name input.

len = 3 + 1 = 4 (including \0)
hash is done only on the 3 first chars, excluding the \0 (therefore the
                                                          len-1 there)

Actually, it's like this only for a matter of consistency between
add_marker and remove_marker, which are quite similar, but add_marker
needs name_len to include the \0 value. It would be odd to change the
logic between the two functions to one including the \0 and the other
excluding it.

> > +/*
> > + * Updates the probe callback corresponding to a range of markers.
> > + * Must be called with markers_mutex held.
> > + */
> > +static void _marker_update_probe_range(
> 
> And yet:
> 
> > +void module_marker_update(struct module *mod)
> > +{
> > +	if (!mod->taints)
> > +		_marker_update_probe_range(mod->markers,
> > +			mod->markers+mod->num_markers, NULL, NULL);
> > +}
> 
> This doesn't hold the markers_mutex.
> 

Since this function is only meant to be called upon module load, before
the module is added to the module list, this marker range cannot be seen
by other calls to marker_update_probe_range. But you are right. It also
protects the hash table, so I should take the mutex there.  Fixing.

Thanks for the review,

Mathieu

> Cheers,
> Rusty.
> 

-- 
Mathieu Desnoyers
Computer Engineering Ph.D. Student, Ecole Polytechnique de Montreal
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68

  reply	other threads:[~2007-08-24 16:27 UTC|newest]

Thread overview: 32+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2007-08-20 20:27 [patch 0/4] Linux Kernel Markers Mathieu Desnoyers
2007-08-20 20:27 ` [patch 1/4] Linux Kernel Markers - Architecture Independent Code Mathieu Desnoyers
2007-08-21  0:43   ` Rusty Russell
2007-08-24 16:26     ` Mathieu Desnoyers [this message]
2007-08-25 20:49       ` Rusty Russell
2007-08-25 21:26         ` Mathieu Desnoyers
2007-08-20 20:27 ` [patch 2/4] Linux Kernel Markers - Add kconfig menus for the marker code Mathieu Desnoyers
2007-08-20 20:27 ` [patch 3/4] Linux Kernel Markers - Documentation Mathieu Desnoyers
2007-08-28 20:53   ` Christoph Hellwig
2007-08-28 21:24     ` Mathieu Desnoyers
2007-08-20 20:27 ` [patch 4/4] Port of blktrace to the Linux Kernel Markers Mathieu Desnoyers
  -- strict thread matches above, loose matches on Subject: below --
2007-09-18 21:13 [patch 0/4] Linux Kernel Markers for 2.6.23-rc6-mm1 Mathieu Desnoyers
2007-09-18 21:13 ` [patch 1/4] Linux Kernel Markers - Architecture Independent Code Mathieu Desnoyers
2007-09-19 11:37   ` Mathieu Desnoyers
2007-09-19 13:53     ` Frank Ch. Eigler
2007-09-19 20:32       ` Denys Vlasenko
2007-09-21 12:58         ` Mathieu Desnoyers
2007-09-21 13:07           ` Christoph Hellwig
2007-09-21 13:30           ` Frank Ch. Eigler
2007-09-21 13:38             ` Mathieu Desnoyers
2007-10-15 19:41               ` Frank Ch. Eigler
2007-10-15 23:12                 ` Mathieu Desnoyers
2007-10-15 23:50                   ` Roland McGrath
2007-10-25 19:17                     ` Mathieu Desnoyers
2007-10-26 14:28                       ` Frank Ch. Eigler
2007-09-19 17:32     ` Denys Vlasenko
2007-09-19 18:46       ` Mathieu Desnoyers
2007-09-19 18:50         ` Mathieu Desnoyers
2007-09-21  0:58   ` Steven Rostedt
2007-09-21 13:45     ` Mathieu Desnoyers
2007-09-17 18:46 [patch 0/4] Linux Kernel Markers Mathieu Desnoyers
2007-09-17 18:46 ` [patch 1/4] Linux Kernel Markers - Architecture Independent Code Mathieu Desnoyers
2007-08-27 16:05 [patch 0/4] Linux Kernel Markers Mathieu Desnoyers
2007-08-27 16:05 ` [patch 1/4] Linux Kernel Markers - Architecture Independent Code Mathieu Desnoyers
2007-08-12 15:10 [patch 0/4] Linux Kernel Markers Mathieu Desnoyers
2007-08-12 15:10 ` [patch 1/4] Linux Kernel Markers - Architecture Independent Code Mathieu Desnoyers

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20070824162656.GC21226@Krystal \
    --to=mathieu.desnoyers@polymtl.ca \
    --cc=akpm@linux-foundation.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=rusty@rustcorp.com.au \
    --subject='Re: [patch 1/4] Linux Kernel Markers - Architecture Independent Code' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

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).