Re: [RFC PATCH] xen/gntdev: reduce stack usage by dynamically allocating gntdev_copy_batch

[Juergen Gross <jgross@suse.com>]

[-- Attachment #1.1.1: Type: text/plain, Size: 3497 bytes --]

On 03.07.25 19:35, Christophe JAILLET wrote:
> Le 03/07/2025 à 07:22, Jürgen Groß a écrit :
>> On 03.07.25 00:42, Tu Dinh wrote:
>>> On 01/07/2025 23:53, Abinash wrote:
>>>> Hi ,
>>>>
>>>> Thanks for pointing that out.
>>>>
>>>> I haven’t measured the performance impact yet — my main focus was on
>>>> getting rid of the stack usage warning triggered by LLVM due to
>>>> inlining. But you're right, gntdev_ioctl_grant_copy() is on a hot
>>>> path, and calling kmalloc() there could definitely slow things down,
>>>> especially under memory pressure.
>>>>
>>>> I’ll run some benchmarks to compare the current approach with the
>>>> dynamic allocation, and also look into alternatives — maybe
>>>> pre-allocating the struct or limiting inlining instead. If you have
>>>> any ideas or suggestions on how best to approach this, I’d be happy to
>>>> hear them.
>>>>
>>>> Do you have any suggestions on how to test the performance?
>>>>
>>>> Best,
>>>> Abinash
>>>>
>>>>
>>>
>>> Preallocating may work but I'd be wary of synchronization if the
>>> preallocated struct is shared.
>>>
>>> I'd look at optimizing status[] which should save quite a few bytes.
>>>
>>> Reducing GNTDEV_COPY_BATCH could be a last resort, but that may also
>>> impact performance.
>>
>> IMHO the most promising way would be to dynamically allocate the struct, but
>> don't free it at the end of the ioctl. Instead it could be put into a list
>> anchored in struct gntdev_priv, so freeing would be done only at close() time.
>>
>> Synchronization would be minimal (just for taking a free struct from the list
>> or putting it back again), while memory usage would be basically just as needed,
>> depending on the number of concurrent threads using the same file descriptor
>> for the ioctl.
>>
>> This approach would even allow to raise GNTDEV_COPY_BATCH, maybe resulting even
>> in a gain of performance.
>>
>> I'll write a patch implementing the allocation scheme.
>>
>>
>> Juergen
> 
> It may be an overkill, but sometimes we see pattern that try to keep the best of 
> the 2 worlds. Something like:
> 
> 
> static struct gntdev_copy_batch static_batch;
> static struct mutex my_mutex;
> 
> static long gntdev_ioctl_grant_copy(...)
> {
>      struct gntdev_copy_batch *dynamic_batch = NULL;
>      struct gntdev_copy_batch *batch;
> 
>      ...
> 
>      if (mutex_trylock(&my_mutex)) {
>          /*
>           * No concurrent access?
>           * Use a shared static variable to avoid an allocation
>           */
>          batch = &static_batch;
>      else {
>          /* otherwise, we need some fresh memory */
>          dynamic_batch = kmalloc(sizeof(*batch), GFP_KERNEL);
>          if (!batch)
>              return -ENOMEM;
> 
>          batch = dynamic_batch;
>      }
> 
>      /* do stuff with 'batch' */
>      ...
> 
> free_batch:
>      if (!dynamic_batch)
>          mutex_unlock(&my_mutex);
>      else
>          kfree(dynamic_batch);
>       return ret;
>   }
> 
> 
> Just my 2c.

Thanks for the remark, but this won't help much. gntdev_ioctl_grant_copy()
is e.g. used by qemu for doing disk I/O on behalf of a Xen guest. This
means that it is very likely to be used concurrently, so just optimizing
for a single thread won't be enough.


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3743 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]