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

[Greg KH <gregkh@linuxfoundation.org>]

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
> 
> For FunctionFS, each "struct file *" for an endpoint (except EP0), keeps
> a list of mmapped buffers. User space might use these buffers to avoid
> copying of data by the kernel, by employing Linux native AsyncIO via
> libaio.
> 
> Standard read() and write() operations will NOT be zerocopy.
> Especially for reads, the expected USB transfer length is unclear;
> whereas an AIO request clearly specifies the maximum transfer length.
> 
> Signed-off-by: Ingo Rohloff <ingo.rohloff@lauterbach.com>
> ---
>  drivers/usb/gadget/function/f_fs.c | 269 ++++++++++++++++++++++++++++-
>  drivers/usb/gadget/function/u_fs.h |   4 +
>  2 files changed, 267 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
> index 7461d27e9604..a2e46704426f 100644
> --- a/drivers/usb/gadget/function/f_fs.c
> +++ b/drivers/usb/gadget/function/f_fs.c
> @@ -201,6 +201,23 @@ struct ffs_epfile {
>  	unsigned char			_pad;
>  };
>  
> +struct ffs_ep_listener {
> +	struct ffs_epfile *epfile;
> +	struct list_head ffsm_list;
> +	spinlock_t       ffsm_lock;
> +};
> +
> +struct ffs_memory {
> +	struct list_head memlist;
> +	struct ffs_ep_listener *listener;
> +	char *kmem;
> +	unsigned long vm_start;
> +	u32 size;
> +
> +	int vma_use_count;
> +	int aio_use_count;
> +};
> +
>  struct ffs_buffer {
>  	size_t length;
>  	char *data;
> @@ -227,6 +244,7 @@ struct ffs_io_data {
>  	bool use_sg;
>  
>  	struct ffs_data *ffs;
> +	struct ffs_memory *ffsm;
>  };
>  
>  struct ffs_desc_helper {
> @@ -702,6 +720,200 @@ static const struct file_operations ffs_ep0_operations = {
>  	.poll =		ffs_ep0_poll,
>  };
>  
> +/* Handling of mmapped transfers *******************************/
> +
> +/* 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?

> +		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?  You are not really protecting anything here correctly
from what I can tell.

thanks,

greg k-h