Re: [PATCH v6 1/9] rv: Introduce DA_MON_ALLOCATION_STRATEGY

[Gabriele Monaco <gmonaco@redhat.com>]

On Fri, 2026-08-21 at 00:45 +0800, wen.yang@linux.dev wrote:
> From: Wen Yang <wen.yang@linux.dev>
>
> +#define DA_ALLOC_AUTO   0
> +#define DA_ALLOC_POOL   1
> +#define DA_ALLOC_MANUAL 2
> +
> +#ifdef DA_MON_POOL_SIZE
> +#ifdef DA_MON_ALLOCATION_STRATEGY

That's correct, but technically it wouldn't be wrong to also define
DA_ALLOC_POOL. We could do:

  #if defined(DA_MON_ALLOCATION_STRATEGY) && DA_MON_ALLOCATION_STRATEGY != DA_ALLOC_POOL
  #error "DA_MON_POOL_SIZE implies DA_ALLOC_POOL"
  #endif

Then the next define shouldn't be an issue because the preprocessor
doesn't complain on multiple /equivalent/ definitions.

No big deal if you prefer it like this though.

> +#error "Define only one of DA_MON_POOL_SIZE or DA_MON_ALLOCATION_STRATEGY"
> +#endif
> +#if DA_MON_POOL_SIZE == 0
> +#error "DA_MON_POOL_SIZE must be non-zero"
> +#endif
> +#define DA_MON_ALLOCATION_STRATEGY DA_ALLOC_POOL
> +#endif /* DA_MON_POOL_SIZE */
> +
> +#ifndef DA_MON_ALLOCATION_STRATEGY
> +#ifdef DA_SKIP_AUTO_ALLOC

Right, I told you not to touch nomiss, but there's no need to maintain
DA_SKIP_AUTO_ALLOC, we can have the monitor do

  #define DA_MON_ALLOCATION_STRATEGY DA_ALLOC_MANUAL

(without any other change), so we can simplify the logic.

...
> +/*
> + * Per-object teardown hook, called after da_monitor_reset_all() +
> + * da_monitor_sync_hook() and before hash_del_rcu() for each entry.
> + * All HA timer callbacks have completed at this point.
> + * Define before including this header.  Default: no-op.
> + */
> +#ifndef da_extra_cleanup
> +#define da_extra_cleanup(da_mon)
> +#endif

da_extra_cleanup() doesn't belong in this patch, does it?
You could have a separate patch for this.

...
> +/*
> + * da_create_pool_storage - pop a free pool slot and insert it into the hash.
> + *
> + * Returns the new da_monitor, or NULL if the pool is exhausted.  Finding
> + * an existing entry for the same id fires WARN_ON_ONCE (double-start bug).
> + *
> + * Caller must hold an RCU read-side CS and the monitor's serialisation lock.
> + */
> +static inline struct da_monitor *
> +da_create_pool_storage(da_id_type id, monitor_target target,
> +		       struct da_monitor *da_mon)
> +{
> +	struct da_monitor_storage *mon_storage, *existing;
> +
> +	if (da_mon)
> +		return da_mon;
> +
> +	mon_storage = mempool_alloc_preallocated(&da_monitor_pool);
> +	if (!mon_storage)
> +		return NULL;
> +	memset(mon_storage, 0, sizeof(*mon_storage));
> +
> +	mon_storage->id = id;
> +	mon_storage->target = target;
> +
> +	/* Single consumer under the caller's lock; duplicate is a double-
> start bug. */
> +	existing = __da_get_mon_storage(id);
> +	if (WARN_ON_ONCE(existing)) {
> +		mempool_free(mon_storage, &da_monitor_pool);
> +		return NULL;
> +	}

Isn't this check for existing redundant? da_prepare_storage() is always
called after a da_get_monitor(), so the first check for da_mon is in
fact validating that we do not already have this entry.

It doesn't return NULL because the entire machine doesn't expect two
different targets with the same id. This doesn't seem to be expected in
your case either (you WARN), so I think you can easily drop this check.

> +	hash_add_rcu(da_monitor_ht, &mon_storage->node, id);
> +	return &mon_storage->rv.da_mon;
> +}
> +
...
> @@ -607,21 +723,37 @@ static inline void da_monitor_destroy(void)
>  	 * pending, we can safely assume no concurrent user.
>  	 */
>  	hash_for_each_safe(da_monitor_ht, bkt, tmp, mon_storage, node) {
> +		da_extra_cleanup(&mon_storage->rv.da_mon);

This one line belongs to another patch, see the comment about
da_extra_cleanup() above.

>  		hash_del_rcu(&mon_storage->node);
> -		kfree(mon_storage);
> +		if (DA_MON_ALLOCATION_STRATEGY == DA_ALLOC_POOL)
> +			mempool_free(mon_storage, &da_monitor_pool);
> +		else
> +			kfree(mon_storage);
> +	}
> +
> +	if (DA_MON_ALLOCATION_STRATEGY == DA_ALLOC_POOL) {
> +		rcu_barrier();
> +		mempool_exit(&da_monitor_pool);
>  	}
>  }
>  
>  /*
> - * Allow the per-object monitors to run allocation manually, necessary if the
> - * start condition is in a context problematic for allocation (e.g.
> scheduling).
> - * In such case, if the storage was pre-allocated without a target, set it
> now.
> + * da_prepare_storage - allocate or link per-object monitor storage.
> + *
> + * Called only from da_handle_start_run_event(); must run in task context

Also from da_handle_start_event(), you could use
da_handle_start*_event() in the comment or even better say "only when the
monitor is started for the first time", because we still call
da_handle_start_event() and friends when the monitor is running on
models where the start event can occur again, but obviously do not
reallocate (that's what the check for da_mon was for).

> + * for DA_ALLOC_AUTO and DA_ALLOC_POOL (both take a spinlock_t internally).
> + * Subsequent event handlers use da_handle_event() and never allocate.
>   */
> -#ifdef DA_SKIP_AUTO_ALLOC
> -#define da_prepare_storage da_fill_empty_storage
> -#else
> -#define da_prepare_storage da_create_storage
> -#endif /* DA_SKIP_AUTO_ALLOC */
> +static inline struct da_monitor *
> +da_prepare_storage(da_id_type id, monitor_target target,
> +		   struct da_monitor *da_mon)
> +{
> +	if (DA_MON_ALLOCATION_STRATEGY == DA_ALLOC_POOL)
> +		return da_create_pool_storage(id, target, da_mon);
> +	if (DA_MON_ALLOCATION_STRATEGY == DA_ALLOC_MANUAL)
> +		return da_fill_empty_storage(id, target, da_mon);
> +	return da_create_storage(id, target, da_mon);
> +}

The rest of the implementation looks good.

Thanks,
Gabriele