Message-ID: <20260213055006.GA55430@system.software.com>
Date: Fri, 13 Feb 2026 14:50:06 +0900
From: Byungchul Park <byungchul@...com>
To: Petr Pavlu <petr.pavlu@...e.com>
Cc: kernel_team@...ynix.com, torvalds@...ux-foundation.org,
	damien.lemoal@...nsource.wdc.com, linux-ide@...r.kernel.org,
	adilger.kernel@...ger.ca, linux-ext4@...r.kernel.org,
	mingo@...hat.com, peterz@...radead.org, will@...nel.org,
	tglx@...utronix.de, rostedt@...dmis.org, joel@...lfernandes.org,
	sashal@...nel.org, daniel.vetter@...ll.ch, duyuyang@...il.com,
	johannes.berg@...el.com, tj@...nel.org, tytso@....edu,
	willy@...radead.org, david@...morbit.com, amir73il@...il.com,
	gregkh@...uxfoundation.org, kernel-team@....com, linux-mm@...ck.org,
	akpm@...ux-foundation.org, mhocko@...nel.org, minchan@...nel.org,
	hannes@...xchg.org, vdavydov.dev@...il.com, sj@...nel.org,
	jglisse@...hat.com, dennis@...nel.org, cl@...ux.com,
	penberg@...nel.org, rientjes@...gle.com, vbabka@...e.cz,
	ngupta@...are.org, linux-block@...r.kernel.org,
	josef@...icpanda.com, linux-fsdevel@...r.kernel.org, jack@...e.cz,
	jlayton@...nel.org, dan.j.williams@...el.com, hch@...radead.org,
	djwong@...nel.org, dri-devel@...ts.freedesktop.org,
	rodrigosiqueiramelo@...il.com, melissa.srw@...il.com,
	hamohammed.sa@...il.com, harry.yoo@...cle.com,
	chris.p.wilson@...el.com, gwan-gyeong.mun@...el.com,
	max.byungchul.park@...il.com, boqun.feng@...il.com,
	longman@...hat.com, yunseong.kim@...csson.com, ysk@...lloc.com,
	yeoreum.yun@....com, netdev@...r.kernel.org,
	matthew.brost@...el.com, her0gyugyu@...il.com, corbet@....net,
	catalin.marinas@....com, bp@...en8.de, x86@...nel.org,
	hpa@...or.com, luto@...nel.org, sumit.semwal@...aro.org,
	gustavo@...ovan.org, christian.koenig@....com,
	andi.shyti@...nel.org, arnd@...db.de, lorenzo.stoakes@...cle.com,
	Liam.Howlett@...cle.com, rppt@...nel.org, surenb@...gle.com,
	mcgrof@...nel.org, da.gomez@...nel.org, samitolvanen@...gle.com,
	paulmck@...nel.org, frederic@...nel.org, neeraj.upadhyay@...nel.org,
	joelagnelf@...dia.com, josh@...htriplett.org, urezki@...il.com,
	mathieu.desnoyers@...icios.com, jiangshanlai@...il.com,
	qiang.zhang@...ux.dev, juri.lelli@...hat.com,
	vincent.guittot@...aro.org, dietmar.eggemann@....com,
	bsegall@...gle.com, mgorman@...e.de, vschneid@...hat.com,
	chuck.lever@...cle.com, neil@...wn.name, okorniev@...hat.com,
	Dai.Ngo@...cle.com, tom@...pey.com, trondmy@...nel.org,
	anna@...nel.org, kees@...nel.org, bigeasy@...utronix.de,
	clrkwllms@...nel.org, mark.rutland@....com, ada.coupriediaz@....com,
	kristina.martsenko@....com, wangkefeng.wang@...wei.com,
	broonie@...nel.org, kevin.brodsky@....com, dwmw@...zon.co.uk,
	shakeel.butt@...ux.dev, ast@...nel.org, ziy@...dia.com,
	yuzhao@...gle.com, baolin.wang@...ux.alibaba.com,
	usamaarif642@...il.com, joel.granados@...nel.org,
	richard.weiyang@...il.com, geert+renesas@...der.be,
	tim.c.chen@...ux.intel.com, linux@...blig.org,
	alexander.shishkin@...ux.intel.com, lillian@...r-ark.net,
	chenhuacai@...nel.org, francesco@...la.it,
	guoweikang.kernel@...il.com, link@...o.com, jpoimboe@...nel.org,
	masahiroy@...nel.org, brauner@...nel.org,
	thomas.weissschuh@...utronix.de, oleg@...hat.com, mjguzik@...il.com,
	andrii@...nel.org, wangfushuai@...du.com, linux-doc@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org, linux-media@...r.kernel.org,
	linaro-mm-sig@...ts.linaro.org, linux-i2c@...r.kernel.org,
	linux-arch@...r.kernel.org, linux-modules@...r.kernel.org,
	rcu@...r.kernel.org, linux-nfs@...r.kernel.org,
	linux-rt-devel@...ts.linux.dev, 2407018371@...com, dakr@...nel.org,
	miguel.ojeda.sandonis@...il.com, neilb@...mail.net,
	bagasdotme@...il.com, wsa+renesas@...g-engineering.com,
	dave.hansen@...el.com, geert@...ux-m68k.org, ojeda@...nel.org,
	alex.gaynor@...il.com, gary@...yguo.net, bjorn3_gh@...tonmail.com,
	lossin@...nel.org, a.hindborg@...nel.org, aliceryhl@...gle.com,
	tmgross@...ch.edu, rust-for-linux@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v18 34/42] dept: add module support for struct
 dept_event_site and dept_event_site_dep

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 <byungchul@...com>
> 
> 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

Powered by blists - more mailing lists