Hello Greg,
On Tue, 8 Feb 2022 11:44:27 +0100
Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx> 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
