Re: [PATCH 1/1] USB: usbfs: Use a spinlock instead of atomic accesses to tally used memory.

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, Feb 08, 2022 at 09:48:58PM +0100, Ingo Rohloff wrote:
> While the fromer code imposes a limit on the used memory, it might be over

By "fromer", you probably mean "former".  But that's an uncommon usage which 
might confuse people (I was confused when I first read it).  It would be 
better to say "existing code".

> pessimistic (even if this is unlikely).
> 
> Example scenario:
> 8 threads running in parallel, all entering
> "usbfs_increase_memory_usage()" at the same time.
> The atomic accesses in "usbfs_increase_memory_usage()" could be
> serialized like this:
>   8 x "atomic64_add"
>   8 x "atomic64_read"
> If the 8 x "atomic64_add" raise "usbfs_memory_usage" above the limit,
> then all 8 calls of "usbfs_increase_memory_usage()" will return with
> -ENOMEM.  If you instead serialize over the whole access to
> "usbfs_memory_usage" by using a spinlock, some of these calls will
> succeed.
> 
> Signed-off-by: Ingo Rohloff <ingo.rohloff@xxxxxxxxxxxxxx>
> ---
>  drivers/usb/core/devio.c | 35 ++++++++++++++++++++++++-----------
>  1 file changed, 24 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
> index fa66e6e58792..522b170c42de 100644
> --- a/drivers/usb/core/devio.c
> +++ b/drivers/usb/core/devio.c
> @@ -139,30 +139,43 @@ MODULE_PARM_DESC(usbfs_memory_mb,
>  /* Hard limit, necessary to avoid arithmetic overflow */
>  #define USBFS_XFER_MAX         (UINT_MAX / 2 - 1000000)
>  
> -static atomic64_t usbfs_memory_usage;	/* Total memory currently allocated */
> +static DEFINE_SPINLOCK(usbfs_memory_usage_lock);
> +static u64 usbfs_memory_usage;	/* Total memory currently allocated */
>  
>  /* Check whether it's okay to allocate more memory for a transfer */
>  static int usbfs_increase_memory_usage(u64 amount)
>  {
> -	u64 lim;
> +	u64 lim, total_mem;
> +	unsigned long flags;
> +	int ret;
>  
> -	lim = READ_ONCE(usbfs_memory_mb);
> +	lim = usbfs_memory_mb;

Don't eliminate this READ_ONCE().  It is needed because usbfs_memory_mb is a 
writable module parameter.  If the value is changed by another process at 
the same time you read it, you could get a nonsense value unless you use 
READ_ONCE().

>  	lim <<= 20;
>  
> -	atomic64_add(amount, &usbfs_memory_usage);
> -
> -	if (lim > 0 && atomic64_read(&usbfs_memory_usage) > lim) {
> -		atomic64_sub(amount, &usbfs_memory_usage);
> -		return -ENOMEM;
> -	}
> +	ret = 0;
> +	spin_lock_irqsave(&usbfs_memory_usage_lock, flags);
> +	total_mem = usbfs_memory_usage + amount;
> +	if (lim > 0 && total_mem > lim)
> +		ret = -ENOMEM;
> +	else
> +		usbfs_memory_usage = total_mem;
> +	spin_unlock_irqrestore(&usbfs_memory_usage_lock, flags);
>  
> -	return 0;
> +	return ret;
>  }
>  
>  /* Memory for a transfer is being deallocated */
>  static void usbfs_decrease_memory_usage(u64 amount)
>  {
> -	atomic64_sub(amount, &usbfs_memory_usage);
> +	u64 total_mem;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&usbfs_memory_usage_lock, flags);
> +	total_mem = usbfs_memory_usage;
> +	if (amount > total_mem)
> +		amount = total_mem;
> +	usbfs_memory_usage = total_mem - amount;

As a matter of personal taste, I prefer:

	if (amount > usbfs_memory_usage)
		usbfs_memory_usage = 0;
	else
		usbfs_memory_usage -= amount;

> +	spin_unlock_irqrestore(&usbfs_memory_usage_lock, flags);
>  }
>  
>  static int connected(struct usb_dev_state *ps)

Alan Stern



[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux