Re: [PATCH v2 1/1] usb: gadget: f_fs: Support zerocopy transfers via mmap.

[Ingo Rohloff <ingo.rohloff@lauterbach.com>]

Hello Greg,

On Tue, 8 Feb 2022 11:44:27 +0100
Greg KH <gregkh@linuxfoundation.org> wrote:

> On Wed, Feb 02, 2022 at 06:20:56PM +0100, Ingo Rohloff wrote:
> > This patch implements the same functionality for FunctionFS as
> > commit f7d34b445abc00e979b7 ("USB: Add support for usbfs zerocopy.")
> > did for USB host devio.c

> ...

> > +/* Check whether it's okay to allocate more memory for mmap */
> > +static int ffsm_increase_mmap_mem_usage(struct ffs_data *ffs, u64 amount)
> > +{
> > +	u64 lim;
> > +
> > +	lim = READ_ONCE(ffs->mmap_memory_mb);
> > +	lim <<= 20;
> > +
> > +	atomic64_add(amount, &ffs->mmap_mem_usage);
> > +
> > +	if (lim > 0 && atomic64_read(&ffs->mmap_mem_usage) > lim) {  
> 
> What prevents it from changing right after you read this?

Nothing.

First of all: I just used the same code as in "drivers/usb/core/devio.c"
functions "usbfs_increase_memory_usage()" and "usbfs_decrease_memory_usage()".

As far as I understand the code of these two functions, this code to imposes a
limit on the amount of kernel space memory, a user might allocate via "mmap"
calls.

The construction makes sure that when "ffsm_increase_mmap_mem_usage()" returns
successfully, it is guaranteed, that "mmap_mem_usage" *was* smaller/equal than 
"lim" at the time the atomic64_read() call was done.

Of course "mmap_mem_usage" might change immediately after the atomic64_read().
But as far as I analyzed, this still limits the total amount of memory
allocated, as long as you make sure that you do the allocation of memory
*after* "ffsm_increase_mmap_mem_usage()" returned successfully and you
deallocate the memory *before* you call "ffsm_decrease_mmap_mem_usage()".


> > +		atomic64_sub(amount, &ffs->mmap_mem_usage);  
> 
> Why not use a real lock instead of trying to do a fake one with this
> atomic variable?

I don't think there is a good reason using the "atomic" stuff:
I think this code path anyway is not hit that often (only when you mmap or
munmap buffers), so this should not have any noticeable impact on performance.

I just took the code from "drivers/usb/core/devio.c",
"usbfs_increase_memory_usage()".
I am still convinced it is correct.

You are of course right: You can easily use a lock here and this makes the
intention of the code a lot clearer I guess.

I will modify the patch accordingly.

so long
  Ingo