Message-ID: <f8335a71-f9af-4911-96fa-46e7088acaf8@suse.com>
Date: Fri, 4 Jul 2025 08:05:55 +0200
From: Juergen Gross <jgross@...e.com>
To: Christophe JAILLET <christophe.jaillet@...adoo.fr>,
 Tu Dinh <ngoc-tu.dinh@...es.tech>, Abinash <abinashlalotra@...il.com>
Cc: sstabellini@...nel.org, oleksandr_tyshchenko@...m.com,
 xen-devel@...ts.xenproject.org, linux-kernel@...r.kernel.org,
 Abinash Singh <abinashsinghlalotra@...il.com>
Subject: Re: [RFC PATCH] xen/gntdev: reduce stack usage by dynamically
 allocating gntdev_copy_batch

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

Download attachment "OpenPGP_0xB0DE9DD628BF132F.asc" of type "application/pgp-keys" (3684 bytes)

Download attachment "OpenPGP_signature.asc" of type "application/pgp-signature" (496 bytes)

Powered by blists - more mailing lists