Memory allocation and copy from user space with vmemdup_user() is relatively slow and can sleep. Move it outside the lock to minimize the time the mutex is held, reducing contention for concurrent accesses.

Signed-off-by: Josh Law <objecting@objecting.org>
---
 drivers/staging/axis-fifo/axis-fifo.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/drivers/staging/axis-fifo/axis-fifo.c b/drivers/staging/axis-fifo/axis-fifo.c
index aad2206b481a..d5533235cefc 100644
--- a/drivers/staging/axis-fifo/axis-fifo.c
+++ b/drivers/staging/axis-fifo/axis-fifo.c
@@ -233,9 +233,15 @@ static ssize_t axis_fifo_write(struct file *f, const char __user *buf,
 	    (words_to_write > (fifo->tx_fifo_depth - 4)))
 		return -EINVAL;
 
+	txbuf = vmemdup_user(buf, len);
+	if (IS_ERR(txbuf))
+		return PTR_ERR(txbuf);
+
 	if (f->f_flags & O_NONBLOCK) {
-		if (!mutex_trylock(&fifo->write_lock))
-			return -EAGAIN;
+		if (!mutex_trylock(&fifo->write_lock)) {
+			ret = -EAGAIN;
+			goto err_free;
+		}
 
 		if (words_to_write > ioread32(fifo->base_addr +
 					      XLLF_TDFV_OFFSET)) {
@@ -252,21 +258,17 @@ static ssize_t axis_fifo_write(struct file *f, const char __user *buf,
 			goto end_unlock;
 	}
 
-	txbuf = vmemdup_user(buf, len);
-	if (IS_ERR(txbuf)) {
-		ret = PTR_ERR(txbuf);
-		goto end_unlock;
-	}
-
 	for (int i = 0; i < words_to_write; ++i)
 		iowrite32(txbuf[i], fifo->base_addr + XLLF_TDFD_OFFSET);
 
 	iowrite32(len, fifo->base_addr + XLLF_TLR_OFFSET);
 
 	ret = len;
-	kvfree(txbuf);
+
 end_unlock:
 	mutex_unlock(&fifo->write_lock);
+err_free:
+	kvfree(txbuf);
 
 	return ret;
 }
-- 
2.43.0

On Sun, Mar 01, 2026 at 12:58:36AM +0000, Josh Law wrote:
> Memory allocation and copy from user space with vmemdup_user() is relatively slow and can sleep. Move it outside the lock to minimize the time the mutex is held, reducing contention for concurrent accesses.

Something happened with your line wrapping :(

And are you sure that your change works?  What is wrong with grabbing
the lock this way?

thanks,

greg k-h

9 Mar 2026 16:07:50 Greg Kroah-Hartman <gregkh@linuxfoundation.org>:

> On Sun, Mar 01, 2026 at 12:58:36AM +0000, Josh Law wrote:
>> Memory allocation and copy from user space with vmemdup_user() is relatively slow and can sleep. Move it outside the lock to minimize the time the mutex is held, reducing contention for concurrent accesses.
>
> Something happened with your line wrapping :(
>
> And are you sure that your change works?  What is wrong with grabbing
> the lock this way?
>
> thanks,
>
> greg k-h

The issue with grabbing the lock before vmemdup_user() is that the allocation and user-space copy can take a significant amount of time and can sleep.

Also, it's just to be more efficient haha, and yes I'm sure this change works, I own the hardware, and it operates better...


V/R




Josh law

On Mon, Mar 09, 2026 at 04:11:25PM +0000, Josh Law wrote:
> 9 Mar 2026 16:07:50 Greg Kroah-Hartman <gregkh@linuxfoundation.org>:
> 
> > On Sun, Mar 01, 2026 at 12:58:36AM +0000, Josh Law wrote:
> >> Memory allocation and copy from user space with vmemdup_user() is relatively slow and can sleep. Move it outside the lock to minimize the time the mutex is held, reducing contention for concurrent accesses.
> >
> > Something happened with your line wrapping :(
> >
> > And are you sure that your change works?  What is wrong with grabbing
> > the lock this way?
> >
> > thanks,
> >
> > greg k-h
> 
> The issue with grabbing the lock before vmemdup_user() is that the allocation and user-space copy can take a significant amount of time and can sleep.

But what is wrong with that?

> Also, it's just to be more efficient haha, and yes I'm sure this change works, I own the hardware, and it operates better...

Great, please put the performance numbers in the patch, that will show
the need for this change.

thanks,

greg k-h

9 Mar 2026 16:56:37 Greg Kroah-Hartman <gregkh@linuxfoundation.org>:

> On Mon, Mar 09, 2026 at 04:11:25PM +0000, Josh Law wrote:
>> 9 Mar 2026 16:07:50 Greg Kroah-Hartman <gregkh@linuxfoundation.org>:
>>
>>> On Sun, Mar 01, 2026 at 12:58:36AM +0000, Josh Law wrote:
>>>> Memory allocation and copy from user space with vmemdup_user() is relatively slow and can sleep. Move it outside the lock to minimize the time the mutex is held, reducing contention for concurrent accesses.
>>>
>>> Something happened with your line wrapping :(
>>>
>>> And are you sure that your change works?  What is wrong with grabbing
>>> the lock this way?
>>>
>>> thanks,
>>>
>>> greg k-h
>>
>> The issue with grabbing the lock before vmemdup_user() is that the allocation and user-space copy can take a significant amount of time and can sleep.
>
> But what is wrong with that?
>
>> Also, it's just to be more efficient haha, and yes I'm sure this change works, I own the hardware, and it operates better...
>
> Great, please put the performance numbers in the patch, that will show
> the need for this change.
>
> thanks,
>
> greg k-h

Hi Greg,
Fair point. Because mutexes are allowed to sleep, holding it during vmemdup_user() isn't inherently a bug. The issue is that while it sleeps, it blocks other processes from acquiring the lock, which bottlenecks CPU usage times slightly



Perf numbers:

sys before: about 18 seconds
sys after: 14 seconds

V/R

Josh law

On Sat Feb 28, 2026 at 6:58 PM CST, Josh Law wrote:
> Memory allocation and copy from user space with vmemdup_user() is relatively slow and can sleep. Move it outside the lock to minimize the time the mutex is held, reducing contention for concurrent accesses.
>
> Signed-off-by: Josh Law <objecting@objecting.org>
> ---

Hi,

It looks like you've sent a few patches editing the same file with 3?
chained together but the rest just own their own. This is a little
confusing trying to review. Typically you'd make a cover letter and then
chain them all together using git send-email.

Also, keep commit body lines under 75 chars. You should use this script
to catch things like this:

"$ ./scripts/checkpatch.pl --strict <patch>"

Thanks,

ET