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

[Juergen Gross]

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

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.

It may be an overkill, but sometimes we see pattern that try to keep the best of the 2 worlds. Something like:

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

OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

OpenPGP_signature.asc
Description: OpenPGP digital signature