Re: [PATCH 9/9] rv: Mandate deallocation for per-obj monitors

[Wen Yang <wen.yang@linux.dev>]

On 5/18/26 14:36, Gabriele Monaco wrote:
> On Sun, 2026-05-17 at 17:52 +0800, Wen Yang wrote:
>>
>> One gap: tools/verification/rvgen/rvgen/templates/dot2k/main.c uses
>> RV_MON_%%MONITOR_TYPE%% but generates no deallocation code, may fail
>> to build with a -Wunused-function warning.
>>
> 
> Thanks for the review!
> 
> That's technically the purpose of this patch, we don't know exactly how is the
> per-obj monitor going to deallocate, so we make sure build fails if they don't
> set up a way.
> 
> This combined with the fact per-obj monitors aren't really documented (yet),
> makes it quite confusing, doesn't it?
> 
> Would you prefer we always generate a dummy hook calling da_destroy_storage()
> and let the user decide what to do with it without forcing (obscure) compiler
> warnings?
> 

Hi Gabriele,

Thanks for the patch and the discussion.

I wonder if generating a dummy hook that calls da_destroy_storage by 
default is the best way to go.
Given that da_destroy_storage internally uses kfree_rcu(), it might 
introduce unnecessary memory allocation/free overhead and could even 
affect RCU grace periods -- especially for monitors that never actually 
need to release objects.

Perhaps a gentler approach for rvgen would be to generate a commented 
example in the template, showing how to use da_skip_deallocation to 
silence the warning.

--
Best wishes,
Wen


> 
>>
>> --
>> Best wishes,
>> Wen
>>
>> On 5/12/26 22:02, Gabriele Monaco wrote:
>>> The per-object monitors use a hash tables and dynamic allocation of the
>>> monitor storage, functions to clean a monitor that is no longer needed
>>> are provided but nothing ensures the monitor actually uses them.
>>>
>>> Remove the inline specifier on the deallocation function to let the
>>> compiler warn in case it isn't referenced. If the monitor really doesn't
>>> need one (for instance because instances will never cease to exist
>>> before disabling the monitor), the da_skip_deallocation() helper macro
>>> can be used to silence the warning.
>>>
>>> Signed-off-by: Gabriele Monaco <gmonaco@redhat.com>
>>> ---
>>>    include/rv/da_monitor.h                      | 14 +++++++++++++-
>>>    kernel/trace/rv/monitors/deadline/deadline.h |  5 ++++-
>>>    2 files changed, 17 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/include/rv/da_monitor.h b/include/rv/da_monitor.h
>>> index 402d3b935c08..378d23ab7dfb 100644
>>> --- a/include/rv/da_monitor.h
>>> +++ b/include/rv/da_monitor.h
>>> @@ -489,8 +489,11 @@ static inline monitor_target
>>> da_get_target_by_id(da_id_type id)
>>>     * locks.
>>>     * This function includes an RCU read-side critical section to synchronise
>>>     * against da_monitor_destroy().
>>> + * NOTE: inline is omitted on purpose to let the compiler warn if this
>>> function
>>> + * is never referenced. For monitors that don't require a deallocation
>>> hook,
>>> + * da_skip_deallocation() can be used.
>>>     */
>>> -static inline void da_destroy_storage(da_id_type id)
>>> +static void da_destroy_storage(da_id_type id)
>>>    {
>>>    	struct da_monitor_storage *mon_storage;
>>>    
>>> @@ -504,6 +507,15 @@ static inline void da_destroy_storage(da_id_type id)
>>>    	kfree_rcu(mon_storage, rcu);
>>>    }
>>>    
>>> +/*
>>> + * da_skip_deallocation - explicitly mark a deallocation function as not
>>> required
>>> + *
>>> + * Only use when you are absolutely sure the monitor doesn't require a
>>> + * deallocation hook (i.e. it's not possible for an object to finish
>>> existing
>>> + * when the monitor is still running).
>>> + */
>>> +#define da_skip_deallocation(hook) ((void)hook)
>>> +
>>>    static void da_monitor_reset_all(void)
>>>    {
>>>    	struct da_monitor_storage *mon_storage;
>>> diff --git a/kernel/trace/rv/monitors/deadline/deadline.h
>>> b/kernel/trace/rv/monitors/deadline/deadline.h
>>> index 78fca873d61e..c39fd79148c2 100644
>>> --- a/kernel/trace/rv/monitors/deadline/deadline.h
>>> +++ b/kernel/trace/rv/monitors/deadline/deadline.h
>>> @@ -194,7 +194,10 @@ static void __maybe_unused handle_newtask(void *data,
>>> struct task_struct *task,
>>>    		da_create_storage(EXPAND_ID_TASK(task), NULL);
>>>    }
>>>    
>>> -static void __maybe_unused handle_exit(void *data, struct task_struct *p,
>>> bool group_dead)
>>> +/*
>>> + * Deallocation hook, use da_skip_deallocation() when not necessary
>>> + */
>>> +static void handle_exit(void *data, struct task_struct *p, bool group_dead)
>>>    {
>>>    	if (p->policy == SCHED_DEADLINE)
>>>    		da_destroy_storage(get_entity_id(&p->dl, DL_TASK,
>>> DL_TASK));
>