Re: [PATCH v2 10/59] dyndbg: replace classmap list with a vector

[jim.cromie@gmail.com]

On Mon, Mar 24, 2025 at 9:08 AM Louis Chauvet <louis.chauvet@bootlin.com> wrote:
>
>
>
> Le 20/03/2025 à 19:51, Jim Cromie a écrit :

> Thanks for your explanation of __outvar! It makes sense. I never seen
> this pattern anywhere in the kernel, maybe a simple doc comment is
> enough to carry the information:


Im gonna pull it - Ive decided its misleading,
since it looks like it does something that it cant do.

a plain old comment would be enough, if its needed at all.
__outvar was at least brief ;-)

In fact I did, but in a later patch. cleaning now.



>         /**
>          * ddebug_find_valid_class - Find a valid class for a
>          * given string
>          * @dt: debug table to inspect
>          * @class_string: string to match on
>          * @class_id: output pointer for the class_id value
>          *
>          * Returns: Pointer to the ddebug_class_map instance, if found.
>          * @class_id will be set to the id of this class. If no class
>          * matching @class_string is found, returns NULL and class_id is
>          * set to -ENOENT.
>          */
>
> Or at maybe change the documentation of __outvar to be a bit more
> explicit about how it works (I had difficulties to understand because a
> lot of "magic macro" exist in the kernel and sometimes carry information
> to the compiler, but this one is always empty):
>
>         /**
>          * __outvar - Denotes that an argument is used as an output
>          * value
>          * This macro does nothing apart clarify  for the reader that
>          * the parameter will be filled by the callee
>          */
>
> With any of the modifications (feel free to reword them as you want):
>
> Reviewed-by: Louis Chauvet <louis.chauvet@bootlin.com>
>
> >   {
> >       struct ddebug_class_map *map;
> > -     int idx;
> > +     int i, idx;
> >
> > -     list_for_each_entry(map, &dt->maps, link) {
> > +     for (map = dt->classes, i = 0; i < dt->num_classes; i++, map++) {
> >               idx = match_string(map->class_names, map->length, class_string);
> >               if (idx >= 0) {
> >                       *class_id = idx + map->base;
> > @@ -164,7 +167,6 @@ static struct ddebug_class_map *ddebug_find_valid_class(struct ddebug_table cons
> >       return NULL;
> >   }
> >
> > -#define __outvar /* filled by callee */
> >   /*
> >    * Search the tables for _ddebug's which match the given `query' and
> >    * apply the `flags' and `mask' to them.  Returns number of matching
> > @@ -1122,9 +1124,10 @@ static void *ddebug_proc_next(struct seq_file *m, void *p, loff_t *pos)
> >
> >   static const char *ddebug_class_name(struct ddebug_iter *iter, struct _ddebug *dp)
> >   {
> > -     struct ddebug_class_map *map;
> > +     struct ddebug_class_map *map = iter->table->classes;
> > +     int i, nc = iter->table->num_classes;
> >
> > -     list_for_each_entry(map, &iter->table->maps, link)
> > +     for (i = 0; i < nc; i++, map++)
> >               if (class_in_range(dp->class_id, map))
> >                       return map->class_names[dp->class_id - map->base];
> >
> > @@ -1208,30 +1211,31 @@ static const struct proc_ops proc_fops = {
> >       .proc_write = ddebug_proc_write
> >   };
> >
> > -static void ddebug_attach_module_classes(struct ddebug_table *dt,
> > -                                      struct ddebug_class_map *classes,
> > -                                      int num_classes)
> > +static void ddebug_attach_module_classes(struct ddebug_table *dt, struct _ddebug_info *di)
> >   {
> >       struct ddebug_class_map *cm;
> > -     int i, j, ct = 0;
> > +     int i, nc = 0;
> >
> > -     for (cm = classes, i = 0; i < num_classes; i++, cm++) {
> > +     /*
> > +      * Find this module's classmaps in a subrange/wholerange of
> > +      * the builtin/modular classmap vector/section.  Save the start
> > +      * and length of the subrange at its edges.
> > +      */
> > +     for (cm = di->classes, i = 0; i < di->num_classes; i++, cm++) {
> >
> >               if (!strcmp(cm->mod_name, dt->mod_name)) {
> > -
> > -                     v2pr_info("class[%d]: module:%s base:%d len:%d ty:%d\n", i,
> > -                               cm->mod_name, cm->base, cm->length, cm->map_type);
> > -
> > -                     for (j = 0; j < cm->length; j++)
> > -                             v3pr_info(" %d: %d %s\n", j + cm->base, j,
> > -                                       cm->class_names[j]);
> > -
> > -                     list_add(&cm->link, &dt->maps);
> > -                     ct++;
> > +                     if (!nc) {
> > +                             v2pr_info("start subrange, class[%d]: module:%s base:%d len:%d ty:%d\n",
> > +                                       i, cm->mod_name, cm->base, cm->length, cm->map_type);
> > +                             dt->classes = cm;
> > +                     }
> > +                     nc++;
> >               }
> >       }
> > -     if (ct)
> > -             vpr_info("module:%s attached %d classes\n", dt->mod_name, ct);
> > +     if (nc) {
> > +             dt->num_classes = nc;
> > +             vpr_info("module:%s attached %d classes\n", dt->mod_name, nc);
> > +     }
> >   }
> >
> >   /*
> > @@ -1263,10 +1267,9 @@ static int ddebug_add_module(struct _ddebug_info *di, const char *modname)
> >       dt->num_ddebugs = di->num_descs;
> >
> >       INIT_LIST_HEAD(&dt->link);
> > -     INIT_LIST_HEAD(&dt->maps);
> >
> >       if (di->classes && di->num_classes)
> > -             ddebug_attach_module_classes(dt, di->classes, di->num_classes);
> > +             ddebug_attach_module_classes(dt, di);
> >
> >       mutex_lock(&ddebug_lock);
> >       list_add_tail(&dt->link, &ddebug_tables);
> > @@ -1379,8 +1382,8 @@ static void ddebug_remove_all_tables(void)
> >       mutex_lock(&ddebug_lock);
> >       while (!list_empty(&ddebug_tables)) {
> >               struct ddebug_table *dt = list_entry(ddebug_tables.next,
> > -                                                   struct ddebug_table,
> > -                                                   link);
> > +                                                  struct ddebug_table,
> > +                                                  link);
> >               ddebug_table_free(dt);
> >       }
> >       mutex_unlock(&ddebug_lock);
>
> --
> Louis Chauvet, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
>
>