Re: [PATCH v18 34/42] dept: add module support for struct dept_event_site and dept_event_site_dep

[Byungchul Park]

On Wed, Feb 18, 2026 at 04:08:19PM +0100, Petr Pavlu wrote:
> On 2/13/26 6:50 AM, Byungchul Park wrote:
> > 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@xxxxxx>
> >>
> >> 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.
> >
> [...]
> >>> 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.
> 
> This was a minor suggestion.. The idea is to simply change the function
> signature to:
> 
> void dept_mark_event_site_used(struct dept_event_site_init **start,
>                                struct dept_event_site_init **end))

I got what you meant.  I will.  Thanks.

	Byungchul

> This way, the compiler can provide proper type checking to ensure that
> correct pointers are passed to dept_mark_event_site_used(). It would
> catch the type mismatch with module::dept_event_sites.
> 
> >
> >> 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?
> 
> Similarly here, changing the code to:
> 
> extern struct dept_event_site_init *__dept_event_sites_start[], *__dept_event_sites_end[];
> 
> It is the same for the initcalls you mentioned. The declarations of
> their start/end symbols are also already properly typed as
> initcall_entry_t[] in include/linux/init.h.
> 
> --
> Thanks,
> Petr