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 <[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.
>
[...]
>>> 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
