On Tue 2021-02-16 17:27:08, Chris Down wrote: > Petr Mladek writes: > > > +/* > > > + * Stores .printk_fmt section boundaries for vmlinux and all loaded > > > modules. > > > + * Add entries with store_printk_fmt_sec, remove entries with > > > + * remove_printk_fmt_sec. > > > + */ > > > +static DEFINE_HASHTABLE(printk_fmts_mod_sections, 8); > > > The hash table looks like an overkill. This is slow path. There are > > typically only tens of loaded modules. Even the module loader > > uses plain list for iterating the list of modules. > > I don't think it's overkill -- we have prod systems with hundreds. Hell, > even my laptop has over 150 loaded. If someone needs to walk all of the > files in debugfs, it seems unreasonable to do an O(n^2) walk when an O(n) > one would suffice. > > Unless you have a practical concern, I think this is a distinct case from > the module loader with its own unique requirements, so I'd prefer to use the > hash table.
OK, it is true that the module API is either called with a particular struct module pointer. Or it has to iterate over all modules anyway, for example, when looking for a symbol. Well, do we need access to struct module at all? What about storing the pointer to struct pf_object into struct printk_fmt_sec *ps into the s->file->f_inode->i_private? Then we would not need any global list/table at all. > > > + > > > +/* Protects printk_fmts_mod_sections */ > > > +static DEFINE_MUTEX(printk_fmts_mutex); > > > > What is the rule for using "printk_fmts" and "printk_fmt", please? > > I can't find the system here ;-) > > > > Anyway, I would prefer to use "printk_fmt" everywhere. > > Or maybe even "pf_". > > pf_ sounds fine. :-) > > > > + > > > +static const char *ps_get_module_name(const struct printk_fmt_sec *ps); > > > +static int debugfs_pf_open(struct inode *inode, struct file *file); > > > > There are used many different: > > > > + shortcuts: fmt, fmts, ps, fmt_sec, dfs > > > > + styles/ordering: ps_get_module_name() vs. > > find_printk_fmt_sec() vs. > > printk_fmt_size() vs. > > debugfs_pf_open() > > > > It might be bearable because there is not much code. But it would > > still help a lot when we make it more consistent. Many subsystems > > prefer using a feature-specific prefix. > > > > It might be "printk_fmt_" or "pf_" [*] in this case. And we could use > > names like: > > > > + struct pf_object [**] > > + pf_get_object_name() > > + pf_find_object() > > + pf_fops, > > + pf_open() > > + pf_release() > > + pf_mutex, > > + pf_add_object() > > + pf_remove_object() > > + pf_module_notify > > Oh, I meant to change the name for v4 but neglected to do so. Sounds good, > will do. Thanks a lot. I am sorry that I ask you to do so many changes. I talked about the style early enough to make the review easy. Also I think that it is not ideal and annoing to do these mass changes and refactoring when the code is already reviewed, tested, and functional. Best Regards, Petr
