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

[Mete Durlu <meted@linux.ibm.com>]

On 06/11/2026 00:06, Ilya Leoshkevich wrote:

Hi Ilya,

> 
> [...]
> 
>> +static void diag204_work_fn(struct work_struct *work)
>> +{
>> +    mutex_lock(&diag204_mutex);
> 
> Consider using guard(mutex)(&diag204_mutex).

Right, that would make the work_fn much simpler;

{
         guard(mutex)(&diag204_mutex);
         if (diag204_is_cache_valid()) {
                 mod_delayed_work(system_dfl_wq, &diag204_work, 
DIAG204_WORK_DELAY);
                 return;
         }
         diag204_free_buffer(diag204_cache_info.buf);
         diag204_cache_info.expires = 0;
         diag204_cache_info.buf = NULL;
}

I'll incorporate that!
> [...]
> 
>> +/**
>> + * 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.

IMO keeping this self contained would keep the lock management
burden away from the callers while also preventing duplicate
lock release chunks and logic.
__acquires() and __releases() should help indicate any misuse
but I am not sure if it's worth it.
One objective of this patch series is to simplify the diag204
interface, so I would like to keep moving forward with the current
approach if there is no big reason against it.

Thanks!