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

["Jürgen Groß" <jgross@suse.com>]

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

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@gmail.com>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> ---
>>   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@cl.cam.ac.uk>, "
>>   	      "Gerd Hoffmann <kraxel@redhat.com>");
>>   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.

> 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.


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 --]