lkml.org 
[lkml]   [2007]   [Aug]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [patch 1/4] Linux Kernel Markers - Architecture Independent Code
    From
    Date
    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:

    > +/* 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.

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

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

    > +/*
    > + * 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.

    > +{
    > + 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?

    > +/*
    > + * 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.

    Cheers,
    Rusty.

    -
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

    \
     
     \ /
      Last update: 2007-08-21 02:47    [W:0.031 / U:30.468 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site