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

[Petr Pavlu <petr.pavlu@suse.com>]

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@sk.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.
> 
[...]
>>> 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))

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