Re: [PATCH RFC 1/3] mm: Add f_ops->populate()

[Jarkko Sakkinen <jarkko@kernel.org>]

On Sun, Mar 06, 2022 at 11:01:36AM +0100, Greg Kroah-Hartman wrote:
> On Sun, Mar 06, 2022 at 07:32:05AM +0200, Jarkko Sakkinen wrote:
> > Sometimes you might want to use MAP_POPULATE to ask a device driver to
> > initialize the device memory in some specific manner. SGX driver can use
> > this to request more memory by issuing ENCLS[EAUG] x86 opcode for each
> > page in the address range.
> > 
> > Add f_ops->populate() with the same parameters as f_ops->mmap() and make
> > it conditionally called inside call_mmap(). Update call sites
> > accodingly.
> > ---
> > Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
> > v3:
> > -       if (!ret && do_populate && file->f_op->populate)
> > +       if (!ret && do_populate && file->f_op->populate &&
> > +           !!(vma->vm_flags & (VM_IO | VM_PFNMAP)))
> > (reported by Matthew Wilcox)
> > v2:
> > -       if (!ret && do_populate)
> > +       if (!ret && do_populate && file->f_op->populate)
> > (reported by Jan Harkes)
> > ---
> >  arch/mips/kernel/vdso.c                    |  2 +-
> >  drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c |  2 +-
> >  fs/coda/file.c                             |  2 +-
> >  fs/overlayfs/file.c                        |  2 +-
> >  include/linux/fs.h                         | 12 ++++++++++--
> >  include/linux/mm.h                         |  2 +-
> >  ipc/shm.c                                  |  2 +-
> >  mm/mmap.c                                  | 10 +++++-----
> >  mm/nommu.c                                 |  4 ++--
> >  9 files changed, 23 insertions(+), 15 deletions(-)
> > 
> > diff --git a/arch/mips/kernel/vdso.c b/arch/mips/kernel/vdso.c
> > index 3d0cf471f2fe..89f3f3da9abd 100644
> > --- a/arch/mips/kernel/vdso.c
> > +++ b/arch/mips/kernel/vdso.c
> > @@ -102,7 +102,7 @@ int arch_setup_additional_pages(struct linux_binprm *bprm, int uses_interp)
> >  		base = mmap_region(NULL, STACK_TOP, PAGE_SIZE,
> >  				VM_READ | VM_EXEC |
> >  				VM_MAYREAD | VM_MAYWRITE | VM_MAYEXEC,
> > -				0, NULL);
> > +				0, NULL, false);
> >  		if (IS_ERR_VALUE(base)) {
> >  			ret = base;
> >  			goto out;
> > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
> > index 1b526039a60d..4c71f64d6a79 100644
> > --- a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
> > +++ b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
> > @@ -107,7 +107,7 @@ static int i915_gem_dmabuf_mmap(struct dma_buf *dma_buf, struct vm_area_struct *
> >  	if (!obj->base.filp)
> >  		return -ENODEV;
> >  
> > -	ret = call_mmap(obj->base.filp, vma);
> > +	ret = call_mmap(obj->base.filp, vma, false);
> >  	if (ret)
> >  		return ret;
> >  
> > diff --git a/fs/coda/file.c b/fs/coda/file.c
> > index 29dd87be2fb8..e14f312fdbf8 100644
> > --- a/fs/coda/file.c
> > +++ b/fs/coda/file.c
> > @@ -173,7 +173,7 @@ coda_file_mmap(struct file *coda_file, struct vm_area_struct *vma)
> >  	spin_unlock(&cii->c_lock);
> >  
> >  	vma->vm_file = get_file(host_file);
> > -	ret = call_mmap(vma->vm_file, vma);
> > +	ret = call_mmap(vma->vm_file, vma, false);
> >  
> >  	if (ret) {
> >  		/* if call_mmap fails, our caller will put host_file so we
> > diff --git a/fs/overlayfs/file.c b/fs/overlayfs/file.c
> > index fa125feed0ff..b963a9397e80 100644
> > --- a/fs/overlayfs/file.c
> > +++ b/fs/overlayfs/file.c
> > @@ -503,7 +503,7 @@ static int ovl_mmap(struct file *file, struct vm_area_struct *vma)
> >  	vma_set_file(vma, realfile);
> >  
> >  	old_cred = ovl_override_creds(file_inode(file)->i_sb);
> > -	ret = call_mmap(vma->vm_file, vma);
> > +	ret = call_mmap(vma->vm_file, vma, false);
> >  	revert_creds(old_cred);
> >  	ovl_file_accessed(file);
> >  
> > diff --git a/include/linux/fs.h b/include/linux/fs.h
> > index e2d892b201b0..2909e2d14af8 100644
> > --- a/include/linux/fs.h
> > +++ b/include/linux/fs.h
> > @@ -42,6 +42,7 @@
> >  #include <linux/mount.h>
> >  #include <linux/cred.h>
> >  #include <linux/mnt_idmapping.h>
> > +#include <linux/mm.h>
> >  
> >  #include <asm/byteorder.h>
> >  #include <uapi/linux/fs.h>
> > @@ -1993,6 +1994,7 @@ struct file_operations {
> >  	long (*unlocked_ioctl) (struct file *, unsigned int, unsigned long);
> >  	long (*compat_ioctl) (struct file *, unsigned int, unsigned long);
> >  	int (*mmap) (struct file *, struct vm_area_struct *);
> > +	int (*populate)(struct file *, struct vm_area_struct *);
> >  	unsigned long mmap_supported_flags;
> >  	int (*open) (struct inode *, struct file *);
> >  	int (*flush) (struct file *, fl_owner_t id);
> > @@ -2074,9 +2076,15 @@ static inline ssize_t call_write_iter(struct file *file, struct kiocb *kio,
> >  	return file->f_op->write_iter(kio, iter);
> >  }
> >  
> > -static inline int call_mmap(struct file *file, struct vm_area_struct *vma)
> > +static inline int call_mmap(struct file *file, struct vm_area_struct *vma, bool do_populate)
> >  {
> > -	return file->f_op->mmap(file, vma);
> > +	int ret = file->f_op->mmap(file, vma);
> > +
> > +	if (!ret && do_populate && file->f_op->populate &&
> > +	    !!(vma->vm_flags & (VM_IO | VM_PFNMAP)))
> > +		ret = file->f_op->populate(file, vma);
> > +
> > +	return ret;
> >  }
> >  
> >  extern ssize_t vfs_read(struct file *, char __user *, size_t, loff_t *);
> > diff --git a/include/linux/mm.h b/include/linux/mm.h
> > index 213cc569b192..6c8c036f423b 100644
> > --- a/include/linux/mm.h
> > +++ b/include/linux/mm.h
> > @@ -2683,7 +2683,7 @@ extern unsigned long get_unmapped_area(struct file *, unsigned long, unsigned lo
> >  
> >  extern unsigned long mmap_region(struct file *file, unsigned long addr,
> >  	unsigned long len, vm_flags_t vm_flags, unsigned long pgoff,
> > -	struct list_head *uf);
> > +	struct list_head *uf, bool do_populate);
> 
> As I have said many times before, don't add random boolean flags to
> function arguments, as they provide no hint as to what they do at all.
> When you read the code, you then have to go back and look up the
> function definition here and see what exactly it means and the flow is
> broken.
> 
> Make function names mean something obvious, for this, if it really is a
> good idea to have this new flag (and I doubt it, but that's not my
> call), then make this a mmap_region_populate() call to make it obvious
> it is something different than the notmal mmap_region() call.

I can create:

* mmap_region_populate()
* call_mmap_populate()

This would localize the changes and leave out those boolean parameters.

> But as is, this is pretty horrid, don't you agree?

So can I conclude from this that in general having populate available for
device memory is something horrid, or just the implementation path?

That's the main reason why I made this RFC patch set, to get clear answer
to that question. I.e. if it is in general sense a bad idea, I'll just
create ioctl. If it is the implementation, I'll try to improve it.

Otherwise, I don't know whether or not it is good idea to include such
patch into the main SGX2 patch set. No means enforcibl tryy to push support
IO memory populate.

> thanks,
> 
> greg k-h

BR, Jarkko