Re: [PATCH v2 18/20] rv: Add support for per-object monitors in DA/HA

[Gabriele Monaco <gmonaco@redhat.com>]

On Tue, 2025-10-21 at 13:55 +0200, Nam Cao wrote:
> Gabriele Monaco <gmonaco@redhat.com> writes:
> > +static inline struct da_monitor *da_create_storage(da_id_type id,
> > +						   monitor_target target,
> > +						   struct da_monitor
> > *da_mon,
> > +						   gfp_t flags)
> > +{
> > +	struct da_monitor_storage *mon_storage;
> > +
> > +	if (da_mon)
> > +		return da_mon;
> 
> I think this 'if' should be moved to da_create_conditional() instead,
> because the "conditional" part should be implemented in the function
> whose name includes "conditional". I think that would make the code
> easier to follow, because one would already have a good guess what the
> function does without looking into the details.

I get this is confusing and poorly documented, also the _conditional function is
not more conditional than the other, so I'd really need to rethink the name (if
it really needs to stay).

The main purpose of the functions under da_monitor_start_hook is to return a
(possibly new) storage, it's perfectly possible, even likely, that the da_mon is
not null, in such case we don't do any further allocation.

Ignoring the vagueness of the name, the two implementations of this hook are to
call an allocation (yes, always conditionally) or just assign the target to a
pre-allocated storage.
Your suggestion of da_monitor_prepare_storage might fit both descriptions.

In fact, having something like kmalloc_nolock (if it works as I expect it would)
might save this gymnastics as we may not even care about pre-allocating, I'm
going to try it out and perhaps re-think this entire mess.

> > +static inline bool da_handle_start_event(da_id_type id, monitor_target
> > target,
> > +					 enum events event)
> > +{
> > +	struct da_monitor *da_mon;
> > +
> > +	if (!da_monitor_enabled())
> > +		return 0;
> > +	guard(rcu)();
> > +	da_mon = da_get_monitor(id, target);
> > +	da_mon = da_monitor_start_hook(id, target, da_mon);
> 
> Do you plan this da_monitor_start_hook() macro to do anything other than
> storage preparation? If not, perhaps it is better to name it
> da_monitor_prepare_storage() or something like that, so that this is
> easier to follow.

That's a good point, this appears already only in per-object code, the general
name is really just pointless.

Thanks,
Gabriele