Re: [PATCH] xen/gntdev: remove struct gntdev_copy_batch from stack

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Wed, 9 Jul 2025, Jürgen Groß wrote:
> On 08.07.25 21:01, Stefano Stabellini wrote:
> > On Thu, 3 Jul 2025, Juergen Gross wrote:
> > > When compiling the kernel with LLVM, the following warning was issued:
> > > 
> > >    drivers/xen/gntdev.c:991: warning: stack frame size (1160) exceeds
> > >    limit (1024) in function 'gntdev_ioctl'
> > > 
> > > The main reason is struct gntdev_copy_batch which is located on the
> > > stack and has a size of nearly 1kb.
> > > 
> > > For performance reasons it shouldn't by just dynamically allocated
> > > instead, so allocate a new instance when needed and instead of freeing
> > > it put it into a list of free structs anchored in struct gntdev_priv.
> > > 
> > > Fixes: a4cdb556cae0 ("xen/gntdev: add ioctl for grant copy")
> > > Reported-by: Abinash Singh <abinashsinghlalotra@xxxxxxxxx>
> > > Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
> > > ---
> > >   drivers/xen/gntdev-common.h |  4 +++
> > >   drivers/xen/gntdev.c        | 71 ++++++++++++++++++++++++++-----------
> > >   2 files changed, 54 insertions(+), 21 deletions(-)
> > > 
> > > diff --git a/drivers/xen/gntdev-common.h b/drivers/xen/gntdev-common.h
> > > index 9c286b2a1900..ac8ce3179ba2 100644
> > > --- a/drivers/xen/gntdev-common.h
> > > +++ b/drivers/xen/gntdev-common.h
> > > @@ -26,6 +26,10 @@ struct gntdev_priv {
> > >           /* lock protects maps and freeable_maps. */
> > >           struct mutex lock;
> > >   +       /* Free instances of struct gntdev_copy_batch. */
> > > + struct gntdev_copy_batch *batch;
> > > + struct mutex batch_lock;
> > > +
> > >   #ifdef CONFIG_XEN_GRANT_DMA_ALLOC
> > >           /* Device for which DMA memory is allocated. */
> > >           struct device *dma_dev;
> > > diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c
> > > index 61faea1f0663..1f2160765618 100644
> > > --- a/drivers/xen/gntdev.c
> > > +++ b/drivers/xen/gntdev.c
> > > @@ -56,6 +56,18 @@ MODULE_AUTHOR("Derek G. Murray
> > > <Derek.Murray@xxxxxxxxxxxx>, "
> > >                 "Gerd Hoffmann <kraxel@xxxxxxxxxx>");
> > >   MODULE_DESCRIPTION("User-space granted page access driver");
> > >   +#define GNTDEV_COPY_BATCH 16
> > > +
> > > +struct gntdev_copy_batch {
> > > + struct gnttab_copy ops[GNTDEV_COPY_BATCH];
> > > + struct page *pages[GNTDEV_COPY_BATCH];
> > > + s16 __user *status[GNTDEV_COPY_BATCH];
> > > + unsigned int nr_ops;
> > > + unsigned int nr_pages;
> > > + bool writeable;
> > > + struct gntdev_copy_batch *next;
> > > +};
> > > +
> > >   static unsigned int limit = 64*1024;
> > >   module_param(limit, uint, 0644);
> > >   MODULE_PARM_DESC(limit,
> > > @@ -584,6 +596,8 @@ static int gntdev_open(struct inode *inode, struct
> > > file *flip)
> > >           INIT_LIST_HEAD(&priv->maps);
> > >           mutex_init(&priv->lock);
> > >   +       mutex_init(&priv->batch_lock);
> > > +
> > >   #ifdef CONFIG_XEN_GNTDEV_DMABUF
> > >           priv->dmabuf_priv = gntdev_dmabuf_init(flip);
> > >           if (IS_ERR(priv->dmabuf_priv)) {
> > > @@ -608,6 +622,7 @@ static int gntdev_release(struct inode *inode, struct
> > > file *flip)
> > >   {
> > >           struct gntdev_priv *priv = flip->private_data;
> > >           struct gntdev_grant_map *map;
> > > + struct gntdev_copy_batch *batch;
> > >           pr_debug("priv %p\n", priv);
> > >   @@ -620,6 +635,14 @@ static int gntdev_release(struct inode *inode,
> > > struct file *flip)
> > >           }
> > >           mutex_unlock(&priv->lock);
> > >   +       mutex_lock(&priv->batch_lock);
> > > + while (priv->batch) {
> > > +         batch = priv->batch;
> > > +         priv->batch = batch->next;
> > > +         kfree(batch);
> > > + }
> > > + mutex_unlock(&priv->batch_lock);
> > > +
> > >   #ifdef CONFIG_XEN_GNTDEV_DMABUF
> > >           gntdev_dmabuf_fini(priv->dmabuf_priv);
> > >   #endif
> > > @@ -785,17 +808,6 @@ static long gntdev_ioctl_notify(struct gntdev_priv
> > > *priv, void __user *u)
> > >           return rc;
> > >   }
> > >   -#define GNTDEV_COPY_BATCH 16
> > > -
> > > -struct gntdev_copy_batch {
> > > - struct gnttab_copy ops[GNTDEV_COPY_BATCH];
> > > - struct page *pages[GNTDEV_COPY_BATCH];
> > > - s16 __user *status[GNTDEV_COPY_BATCH];
> > > - unsigned int nr_ops;
> > > - unsigned int nr_pages;
> > > - bool writeable;
> > > -};
> > > -
> > >   static int gntdev_get_page(struct gntdev_copy_batch *batch, void __user
> > > *virt,
> > >                                   unsigned long *gfn)
> > >   {
> > > @@ -953,36 +965,53 @@ static int gntdev_grant_copy_seg(struct
> > > gntdev_copy_batch *batch,
> > >   static long gntdev_ioctl_grant_copy(struct gntdev_priv *priv, void
> > > __user *u)
> > >   {
> > >           struct ioctl_gntdev_grant_copy copy;
> > > - struct gntdev_copy_batch batch;
> > > + struct gntdev_copy_batch *batch;
> > >           unsigned int i;
> > >           int ret = 0;
> > >           if (copy_from_user(&copy, u, sizeof(copy)))
> > >                   return -EFAULT;
> > >   -       batch.nr_ops = 0;
> > > - batch.nr_pages = 0;
> > > + mutex_lock(&priv->batch_lock);
> > > + if (!priv->batch) {
> > > +         batch = kmalloc(sizeof(*batch), GFP_KERNEL);
> > > + } else {
> > > +         batch = priv->batch;
> > > +         priv->batch = batch->next;
> > > + }
> > > + mutex_unlock(&priv->batch_lock);
> > 
> > I am concerned about the potentially unbounded amount of memory that
> > could be allocated this way.
> 
> Unbounded? It can be at most the number of threads using the interface
> concurrently.

That's what I meant



> > The mutex is already a potentially very slow operation. Could we instead
> > allocate a single batch, and if it is currently in use, use the mutex to
> > wait until it becomes available?
> 
> As this interface is e.g. used by the qemu based qdisk backend, the chances
> are very high that there are concurrent users. This would hurt multi-ring
> qdisk quite badly!
> 
> It would be possible to replace the mutex with a spinlock and do the kmalloc()
> outside the locked region.
> 
> > 
> > I am also OK with the current approach but I thought I would ask.
> > 
> > 
> > 
> > 
> > > + if (!batch)
> > > +         return -ENOMEM;
> > > +
> > > + batch->nr_ops = 0;
> > > + batch->nr_pages = 0;
> > >           for (i = 0; i < copy.count; i++) {
> > >                   struct gntdev_grant_copy_segment seg;
> > >                   if (copy_from_user(&seg, &copy.segments[i],
> > > sizeof(seg))) {
> > >                           ret = -EFAULT;
> > > +                 gntdev_put_pages(batch);
> > >                           goto out;
> > >                   }
> > >   -               ret = gntdev_grant_copy_seg(&batch, &seg,
> > > &copy.segments[i].status);
> > > -         if (ret < 0)
> > > +         ret = gntdev_grant_copy_seg(batch, &seg,
> > > &copy.segments[i].status);
> > > +         if (ret < 0) {
> > > +                 gntdev_put_pages(batch);
> > >                           goto out;
> > > +         }
> > >                   cond_resched();
> > >           }
> > > - if (batch.nr_ops)
> > > -         ret = gntdev_copy(&batch);
> > > - return ret;
> > > + if (batch->nr_ops)
> > > +         ret = gntdev_copy(batch);
> > > +
> > > + out:
> > > + mutex_lock(&priv->batch_lock);
> > > + batch->next = priv->batch;
> > > + priv->batch = batch;
> > > + mutex_unlock(&priv->batch_lock);
> > >   -  out:
> > > - gntdev_put_pages(&batch);
> > 
> > One change from before is that in case of no errors, gntdev_put_pages is
> > not called anymore. Do we want that? Specifically, we are missing the
> > call to unpin_user_pages_dirty_lock
> 
> I don't think you are right. There was a "return ret" before the "out:"
> label before my patch.

You are right, I missed it.

Reviewed-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>