Re: [PATCH v5] s390/diag204: Add common diag204 interface with caching

[Ilya Leoshkevich <iii@linux.ibm.com>]

On 11/4/26 10:24, Mete Durlu wrote:
> diag204 is currently used by hypfs and sthyi, each with its own probe,
> buffer allocation, and store logic. This duplicates the subcode and
> data format selection and lets any caller issue a diag204 whenever it
> likes, which is costly on the hypervisor side.
> 
> Add a common implementation in arch/s390/kernel/diag/diag204.c and
> convert hypfs and sthyi to it.
> 
> The supported subcode (7, 6 or 4) and data format are probed once in an
> arch_initcall. As a consequence, all kernel configurations, including
> those without hypfs, now probe diag204 at boot.
> 
> diag204_store() serves data from a cache that is valid for one second.
> When the cache is stale, only one caller at a time refreshes it by
> issuing diag204. Other callers wait on the mutex and then use the
> fresh data. Within the validity period no new diag204 is issued, which
> limits the rate of diag204 calls. An idle cache is dropped by a delayed
> work after 10 seconds to not hold on to the buffer indefinitely.
> 
> If allow_busy is set, the diag204 busy-indication bit is passed to the
> hypervisor, the updater may return -EBUSY, and concurrent callers
> return -EBUSY immediately instead of waiting. Otherwise callers wait
> on a killable mutex and get -EINTR if interrupted.
> 
> Signed-off-by: Mete Durlu <meted@linux.ibm.com>

[...]

>   arch/s390/hypfs/hypfs_diag.c    | 131 +++---------------
>   arch/s390/hypfs/hypfs_diag.h    |   5 +-
>   arch/s390/hypfs/hypfs_diag_fs.c |   8 +-
>   arch/s390/include/asm/diag.h    |   7 +
>   arch/s390/kernel/diag/Makefile  |   2 +-
>   arch/s390/kernel/diag/diag204.c | 284 ++++++++++++++++++++++++++++++++++++++++
>   arch/s390/kernel/sthyi.c        |  30 ++---
>   7 files changed, 324 insertions(+), 143 deletions(-)

[...]

> +static void diag204_work_fn(struct work_struct *work)
> +{
> +	mutex_lock(&diag204_mutex);

Consider using guard(mutex)(&diag204_mutex).

> +	if (!diag204_cache_info.buf)
> +		goto unlock;
> +	if (diag204_is_cache_valid())
> +		goto rearm;
> +	diag204_free_buffer(diag204_cache_info.buf);
> +	diag204_cache_info.expires = 0;
> +	diag204_cache_info.buf = NULL;
> +	goto unlock;
> +rearm:
> +	mod_delayed_work(system_dfl_wq, &diag204_work, DIAG204_WORK_DELAY);
> +unlock:
> +	mutex_unlock(&diag204_mutex);
> +}

[...]

> +/**
> + * diag204_store() - Store diag204 data
> + * @buf:             Diag204 data buffer allocated using
> + *                   diag204_allocate_buffer() or a page aligned, zeroed buffer
> + *                   with a size acquired from diag204_get_buf_size().
> + * @allow_busy:      Allow diag204 to return -EBUSY instead of blocking and
> + *                   return early when there is a concurrent updater.
> + *
> + * Diag204 queries CPU utilization data from the hypervisor. Per CPU
> + * utilization information is returned either only for the current guest
> + * or, with authorization for all guests configured.
> + *
> + * Context: diag204_store() can sleep and holds diag204_mutex to serialize
> + *          diag204 cache access.
> + *
> + * Return:
> + * * 0 on success
> + * * -EOPNOTSUPP diag204 is not supported
> + * * -EINVAL if @buf is NULL
> + * * -ENOMEM if the temporary buffer could not be allocated
> + * * -EBUSY if @allow_busy is set and diag204 or a concurrent cache update
> + *    is busy
> + * * -EINTR if interrupted by a signal
> + */
> +int diag204_store(void *buf, bool allow_busy)
> +{
> +	int rc;
> +
> +	if (!diag204_is_available())
> +		return -EOPNOTSUPP;
> +	if (!buf)
> +		return -EINVAL;
> +	rc = 0;
> +	if (allow_busy) {
> +		if (!mutex_trylock(&diag204_mutex))
> +			return -EBUSY;
> +	} else {
> +		if (mutex_lock_killable(&diag204_mutex))
> +			return -EINTR;
> +	}
> +	if (diag204_is_cache_valid())
> +		goto copy;
> +	rc = diag204_update_cache(allow_busy);
> +	if (rc)
> +		goto out;
> +copy:
> +	memcpy(buf, diag204_cache_info.buf, diag204_get_buf_size());

I wonder if it would make sense to return the buffer address and keep
the lock. Most callers should not take more time than memcpy() and would
benefit from simplified buffer management. Others can do memcpy()
themselves. __acquires() and __releases() annotations should help with
keeping such solution maintainable.

[...]