Re: [PATCH v12 2/4] uacce: add uacce driver

[Jean-Philippe Brucker <jean-philippe@linaro.org>]

On Mon, Feb 24, 2020 at 10:22:02AM -0800, Raj, Ashok wrote:
> Hi Kenneth,
> 
> sorry for waking up late on this patchset.
> 
> 
> On Wed, Jan 15, 2020 at 10:12:46PM +0800, Zhangfei Gao wrote:
> [... trimmed]
> 
> > +
> > +static int uacce_fops_open(struct inode *inode, struct file *filep)
> > +{
> > +	struct uacce_mm *uacce_mm = NULL;
> > +	struct uacce_device *uacce;
> > +	struct uacce_queue *q;
> > +	int ret = 0;
> > +
> > +	uacce = xa_load(&uacce_xa, iminor(inode));
> > +	if (!uacce)
> > +		return -ENODEV;
> > +
> > +	q = kzalloc(sizeof(struct uacce_queue), GFP_KERNEL);
> > +	if (!q)
> > +		return -ENOMEM;
> > +
> > +	mutex_lock(&uacce->mm_lock);
> > +	uacce_mm = uacce_mm_get(uacce, q, current->mm);
> 
> I think having this at open time is a bit unnatural. Since when a process
> does fork, we do not inherit the PASID. Although it inherits the fd
> but cannot use the mmaped address in the child.

Both the queue and the PASID are tied to a single address space. When it
disappears, the queue is stopped (zombie state) and the PASID is freed.
The fd is not usable nor recoverable at this point, it's just waiting to
be released.

> If you move this to the mmap time, its more natural. The child could
> do a mmap() get a new PASID + mmio space to work with the hardware.

I like the idea, as it ties the lifetime of the bond to that of the queue
mapping, but I have two small concerns:

* It adds a lot of side-effect to mmap(). In addition to mapping the MMIO
  region it would now create both the bond and the queue. For userspace,
  figuring out why the mmap() fails would be more difficult.

* It forces uacce drivers to implement an mmap() interface, and have MMIO
  regions to share. I suspect it's going to be the norm but at the moment
  it's not mandatory, drivers could just implement ioctls ops.

I guess the main benefit would be reusing an fd after the original address
space dies, but is it a use-case?

I'd rather go one step further in the other direction, declare that an fd
is a queue and is exclusive to an address space, by preventing any
operation (ioctl and mmap) from an mm other than the one that opened the
fd. It's not natural but it'd keep the kernel driver simple as we wouldn't
have to reconfigure the queue during the lifetime of the fd.

Thanks,
Jean