On Wed, Jan 07, 2026 at 01:19:00PM +0100, Petr Pavlu wrote:
> On 12/5/25 8:18 AM, Byungchul Park wrote:
> > struct dept_event_site and struct dept_event_site_dep have been
> > introduced to track dependencies between multi event sites for a single
> > wait, that will be loaded to data segment. Plus, a custom section,
> > '.dept.event_sites', also has been introduced to keep pointers to the
> > objects to make sure all the event sites defined exist in code.
> >
> > dept should work with the section and segment of module. Add the
> > support to handle the section and segment properly whenever modules are
> > loaded and unloaded.
> >
> > Signed-off-by: Byungchul Park <[email protected]>
>
> Below are a few comments from the module loader perspective.
Sorry about the late reply. I've been going through some major life
changes lately. :(
Thank you sooooo~ much for your helpful feedback. I will leave my
opinion below.
> > ---
> > include/linux/dept.h | 14 +++++++
> > include/linux/module.h | 5 +++
> > kernel/dependency/dept.c | 79 +++++++++++++++++++++++++++++++++++-----
> > kernel/module/main.c | 15 ++++++++
> > 4 files changed, 103 insertions(+), 10 deletions(-)
> >
> > diff --git a/include/linux/dept.h b/include/linux/dept.h
> > index 44083e6651ab..c796cdceb04e 100644
> > --- a/include/linux/dept.h
> > +++ b/include/linux/dept.h
> > @@ -166,6 +166,11 @@ struct dept_event_site {
> > struct dept_event_site *bfs_parent;
> > struct list_head bfs_node;
> >
> > + /*
> > + * for linking all dept_event_site's
> > + */
> > + struct list_head all_node;
> > +
> > /*
> > * flag indicating the event is not only declared but also
> > * actually used in code
> > @@ -182,6 +187,11 @@ struct dept_event_site_dep {
> > */
> > struct list_head dep_node;
> > struct list_head dep_rev_node;
> > +
> > + /*
> > + * for linking all dept_event_site_dep's
> > + */
> > + struct list_head all_node;
> > };
> >
> > #define DEPT_EVENT_SITE_INITIALIZER(es)
> > \
> > @@ -193,6 +203,7 @@ struct dept_event_site_dep {
> > .bfs_gen = 0, \
> > .bfs_parent = NULL, \
> > .bfs_node = LIST_HEAD_INIT((es).bfs_node), \
> > + .all_node = LIST_HEAD_INIT((es).all_node), \
> > .used = false, \
> > }
> >
> > @@ -202,6 +213,7 @@ struct dept_event_site_dep {
> > .recover_site = NULL, \
> > .dep_node = LIST_HEAD_INIT((esd).dep_node), \
> > .dep_rev_node = LIST_HEAD_INIT((esd).dep_rev_node), \
> > + .all_node = LIST_HEAD_INIT((esd).all_node), \
> > }
> >
> > struct dept_event_site_init {
> > @@ -225,6 +237,7 @@ extern void dept_init(void);
> > extern void dept_task_init(struct task_struct *t);
> > extern void dept_task_exit(struct task_struct *t);
> > extern void dept_free_range(void *start, unsigned int sz);
> > +extern void dept_mark_event_site_used(void *start, void *end);
>
> Nit: The coding style recommends not using the extern keyword with
> function declarations.
I will remove 'extern's. Thanks.
> https://www.kernel.org/doc/html/v6.19-rc4/process/coding-style.html#function-prototypes
>
> >
> > extern void dept_map_init(struct dept_map *m, struct dept_key *k, int
> > sub_u, const char *n);
> > extern void dept_map_reinit(struct dept_map *m, struct dept_key *k, int
> > sub_u, const char *n);
> > @@ -288,6 +301,7 @@ struct dept_event_site { };
> > #define dept_task_init(t) do { } while (0)
> > #define dept_task_exit(t) do { } while (0)
> > #define dept_free_range(s, sz) do { } while
> > (0)
> > +#define dept_mark_event_site_used(s, e) do { } while
> > (0)
> >
> > #define dept_map_init(m, k, su, n) do { (void)(n);
> > (void)(k); } while (0)
> > #define dept_map_reinit(m, k, su, n) do { (void)(n);
> > (void)(k); } while (0)
> > diff --git a/include/linux/module.h b/include/linux/module.h
> > index d80c3ea57472..29885ba91951 100644
> > --- a/include/linux/module.h
> > +++ b/include/linux/module.h
> > @@ -29,6 +29,7 @@
> > #include <linux/srcu.h>
> > #include <linux/static_call_types.h>
> > #include <linux/dynamic_debug.h>
> > +#include <linux/dept.h>
> >
> > #include <linux/percpu.h>
> > #include <asm/module.h>
> > @@ -588,6 +589,10 @@ struct module {
> > #ifdef CONFIG_DYNAMIC_DEBUG_CORE
> > struct _ddebug_info dyndbg_info;
> > #endif
> > +#ifdef CONFIG_DEPT
> > + struct dept_event_site **dept_event_sites;
> > + unsigned int num_dept_event_sites;
> > +#endif
> > } ____cacheline_aligned __randomize_layout;
> > #ifndef MODULE_ARCH_INIT
> > #define MODULE_ARCH_INIT {}
>
> My understanding is that entries in the .dept.event_sites section are
> added by the dept_event_site_used() macro and they are pointers to the
> dept_event_site_init struct, not dept_event_site.
>
> > diff --git a/kernel/dependency/dept.c b/kernel/dependency/dept.c
> > index b14400c4f83b..07d883579269 100644
> > --- a/kernel/dependency/dept.c
> > +++ b/kernel/dependency/dept.c
> > @@ -984,6 +984,9 @@ static void bfs(void *root, struct bfs_ops *ops, void
> > *in, void **out)
> > * event sites.
> > */
> >
> > +static LIST_HEAD(dept_event_sites);
> > +static LIST_HEAD(dept_event_site_deps);
> > +
> > /*
> > * Print all events in the circle.
> > */
> > @@ -2043,6 +2046,33 @@ static void del_dep_rcu(struct rcu_head *rh)
> > preempt_enable();
> > }
> >
> > +/*
> > + * NOTE: Must be called with dept_lock held.
> > + */
> > +static void disconnect_event_site_dep(struct dept_event_site_dep *esd)
> > +{
> > + list_del_rcu(&esd->dep_node);
> > + list_del_rcu(&esd->dep_rev_node);
> > +}
> > +
> > +/*
> > + * NOTE: Must be called with dept_lock held.
> > + */
> > +static void disconnect_event_site(struct dept_event_site *es)
> > +{
> > + struct dept_event_site_dep *esd, *next_esd;
> > +
> > + list_for_each_entry_safe(esd, next_esd, &es->dep_head, dep_node) {
> > + list_del_rcu(&esd->dep_node);
> > + list_del_rcu(&esd->dep_rev_node);
> > + }
> > +
> > + list_for_each_entry_safe(esd, next_esd, &es->dep_rev_head,
> > dep_rev_node) {
> > + list_del_rcu(&esd->dep_node);
> > + list_del_rcu(&esd->dep_rev_node);
> > + }
> > +}
> > +
> > /*
> > * NOTE: Must be called with dept_lock held.
> > */
> > @@ -2384,6 +2414,8 @@ void dept_free_range(void *start, unsigned int sz)
> > {
> > struct dept_task *dt = dept_task();
> > struct dept_class *c, *n;
> > + struct dept_event_site_dep *esd, *next_esd;
> > + struct dept_event_site *es, *next_es;
> > unsigned long flags;
> >
> > if (unlikely(!dept_working()))
> > @@ -2405,6 +2437,24 @@ void dept_free_range(void *start, unsigned int sz)
> > while (unlikely(!dept_lock()))
> > cpu_relax();
> >
> > + list_for_each_entry_safe(esd, next_esd, &dept_event_site_deps,
> > all_node) {
> > + if (!within((void *)esd, start, sz))
> > + continue;
> > +
> > + disconnect_event_site_dep(esd);
> > + list_del(&esd->all_node);
> > + }
> > +
> > + list_for_each_entry_safe(es, next_es, &dept_event_sites, all_node) {
> > + if (!within((void *)es, start, sz) &&
> > + !within(es->name, start, sz) &&
> > + !within(es->func_name, start, sz))
> > + continue;
> > +
> > + disconnect_event_site(es);
> > + list_del(&es->all_node);
> > + }
> > +
> > list_for_each_entry_safe(c, n, &dept_classes, all_node) {
> > if (!within((void *)c->key, start, sz) &&
> > !within(c->name, start, sz))
> > @@ -3337,6 +3387,7 @@ void __dept_recover_event(struct dept_event_site_dep
> > *esd,
> >
> > list_add(&esd->dep_node, &es->dep_head);
> > list_add(&esd->dep_rev_node, &rs->dep_rev_head);
> > + list_add(&esd->all_node, &dept_event_site_deps);
> > check_recover_dl_bfs(esd);
> > unlock:
> > dept_unlock();
> > @@ -3347,6 +3398,23 @@ EXPORT_SYMBOL_GPL(__dept_recover_event);
> >
> > #define B2KB(B) ((B) / 1024)
> >
> > +void dept_mark_event_site_used(void *start, void *end)
>
> Nit: I suggest that dept_mark_event_site_used() take pointers to
> dept_event_site_init, which would catch the type mismatch with
IMO, this is the easiest way to get all the pointers from start to the
end, or I can't get the number of the pointers. It's similar to the
initcalls section for device drivers.
> module::dept_event_sites.
>
> > +{
> > + struct dept_event_site_init **evtinitpp;
> > +
> > + for (evtinitpp = (struct dept_event_site_init **)start;
> > + evtinitpp < (struct dept_event_site_init **)end;
> > + evtinitpp++) {
> > + (*evtinitpp)->evt_site->used = true;
> > + (*evtinitpp)->evt_site->func_name = (*evtinitpp)->func_name;
> > + list_add(&(*evtinitpp)->evt_site->all_node,
> > &dept_event_sites);
> > +
> > + pr_info("dept_event_site %s@%s is initialized.\n",
> > + (*evtinitpp)->evt_site->name,
> > + (*evtinitpp)->evt_site->func_name);
> > + }
> > +}
> > +
> > extern char __dept_event_sites_start[], __dept_event_sites_end[];
>
> Related to the above, __dept_event_sites_start and
> __dept_event_sites_end can already be properly typed here.
How can I get the number of the pointers?
> >
> > /*
> > @@ -3356,20 +3424,11 @@ extern char __dept_event_sites_start[],
> > __dept_event_sites_end[];
> > void __init dept_init(void)
> > {
> > size_t mem_total = 0;
> > - struct dept_event_site_init **evtinitpp;
> >
> > /*
> > * dept recover dependency tracking works from now on.
> > */
> > - for (evtinitpp = (struct dept_event_site_init
> > **)__dept_event_sites_start;
> > - evtinitpp < (struct dept_event_site_init
> > **)__dept_event_sites_end;
> > - evtinitpp++) {
> > - (*evtinitpp)->evt_site->used = true;
> > - (*evtinitpp)->evt_site->func_name = (*evtinitpp)->func_name;
> > - pr_info("dept_event %s@%s is initialized.\n",
> > - (*evtinitpp)->evt_site->name,
> > - (*evtinitpp)->evt_site->func_name);
> > - }
> > + dept_mark_event_site_used(__dept_event_sites_start,
> > __dept_event_sites_end);
> > dept_recover_ready = true;
> >
> > local_irq_disable();
> > diff --git a/kernel/module/main.c b/kernel/module/main.c
> > index 03ed63f2adf0..82448cdb8ed7 100644
> > --- a/kernel/module/main.c
> > +++ b/kernel/module/main.c
> > @@ -2720,6 +2720,11 @@ static int find_module_sections(struct module *mod,
> > struct load_info *info)
> >
> > &mod->dyndbg_info.num_classes);
> > #endif
> >
> > +#ifdef CONFIG_DEPT
> > + mod->dept_event_sites = section_objs(info, ".dept.event_sites",
> > + sizeof(*mod->dept_event_sites),
> > + &mod->num_dept_event_sites);
> > +#endif
> > return 0;
> > }
> >
> > @@ -3346,6 +3351,14 @@ static int early_mod_check(struct load_info *info,
> > int flags)
> > return err;
> > }
> >
> > +static void dept_mark_event_site_used_module(struct module *mod)
> > +{
> > +#ifdef CONFIG_DEPT
> > + dept_mark_event_site_used(mod->dept_event_sites,
> > + mod->dept_event_sites +
> > mod->num_dept_event_sites);
> > +#endif
> > +}
> > +
>
> It seems to me that the .dept.event_sites section can be discarded after
> the module is initialized. In this case, the section should be prefixed
> by ".init" and its address can be obtained at the point of use in
> dept_mark_event_site_used_module(), without needing to store it inside
> the module struct.
Maybe yes. I will try. Thank you.
> Additionally, what is the reason that the dept_event_site_init data is
> not stored in the .dept.event_sites section directly and it requires
> a level of indirection?
Maybe yes. I will try to store dept_event_site_init in the
.dept.event_sites section directly.
> In general, for my own understanding, I also wonder whether the check to
> determine that a dept_event_site is used needs to be done at runtime, or
> if it could be done at build time by objtool/modpost.
You are right. I picked what I'm used to the most, among all the
candidate methods. However, I will try to use a better way if you
suggest what you think it should be.
Thanks for the thorough review, Petr.
Byungchul
> > /*
> > * Allocate and load the module: note that size of section 0 is always
> > * zero, and we rely on this for optional sections.
> > @@ -3508,6 +3521,8 @@ static int load_module(struct load_info *info, const
> > char __user *uargs,
> > /* Done! */
> > trace_module_load(mod);
> >
> > + dept_mark_event_site_used_module(mod);
> > +
> > return do_init_module(mod);
> >
> > sysfs_cleanup:
>
> --
> Thanks,
> Petr
