[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH] xen/gntdev: remove struct gntdev_copy_batch from stack
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(©, 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, ©.segments[i], > > > sizeof(seg))) { > > > ret = -EFAULT; > > > + gntdev_put_pages(batch); > > > goto out; > > > } > > > - ret = gntdev_grant_copy_seg(&batch, &seg, > > > ©.segments[i].status); > > > - if (ret < 0) > > > + ret = gntdev_grant_copy_seg(batch, &seg, > > > ©.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>
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |