[PATCH v7 2/3] uacce: add uacce driver

zhangfei zhangfei.gao at linaro.org
Fri Nov 8 00:23:50 AEDT 2019



On 2019/11/6 下午11:32, Jean-Philippe Brucker wrote:
> On Wed, Nov 06, 2019 at 04:17:40PM +0800, zhangfei wrote:
>>> But I still believe it would be better to create an uacce_mm structure
>>> that tracks all queues bound to this mm, and pass that to uacce_sva_exit
>>> instead of the uacce_device.
>> I am afraid this method may not work.
>> Since currently iommu_sva_bind_device only accept the same drvdata for the
>> same dev,
>> that's the reason we can not directly use "queue" as drvdata.
>> Each time create an uacce_mm structure should be same problem as queue, and
>> fail for same dev.
>> So we use uacce and pick up the right queue inside.
> What I had in mind is keep one uacce_mm per mm and per device, and we can
> pass that to iommu_sva_bind_device(). It requires some structure changes,
> see the attached patch.
Cool, thanks Jean
How about merge them together.
>
>>> The queue isn't bound to a task, but its address space. With clone() the
>>> address space can be shared between tasks. In addition, whoever has a
>>> queue fd also gets access to this address space. So after a fork() the
>>> child may be able to program the queue to DMA into the parent's address
>>> space, even without CLONE_VM. Users must be aware of this and I think it's
>>> important to explain it very clearly in the UAPI.
>>> [...]
>>>> +void uacce_unregister(struct uacce_device *uacce)
>>>> +{
>>>> +	if (!uacce)
>>>> +		return;
>>>> +
>>>> +	mutex_lock(&uacce->q_lock);
>>>> +	if (!list_empty(&uacce->qs)) {
>>>> +		struct uacce_queue *q;
>>>> +
>>>> +		list_for_each_entry(q, &uacce->qs, list) {
>>>> +			uacce_put_queue(q);
>>> The open file descriptor will still exist after this function returns.
>>> Can all fops can be called with a stale queue?
>> To more clear:.
>> Do you mean rmmod without fops_release.
> Yes I think so. What happens when userspace starts some queues, and
> the device driver suddenly calls uacce_unregister(). We call
> cdev_device_del() later in this function, but quoting the documentation:
> "any cdevs already open will remain and their fops will still be callable
> even after this function returns." So we need to make sure that any of the
> fops is safe to run after the uacce device disappears.
We can protect stale queue via q->state, since q is released later in 
fops_release.
And uacce_unregister: put_queue will set q->state = UACCE_Q_ZOMBIE.
Will add state check in mmap too.
>
> I noticed a lock dependency inversion on uacce->q_lock: uacce_unregister()
> calls iommu_sva_unbind_device() while holding the uacce->q_lock, but
> uacce_sva_exit() takes the uacce->q_lock with the SVA lock held. In theory
> we could simply avoid calling iommu_sva_unbind_device() here since it will
> be done by fops_release(), but then disabling the SVA feature in
> uacce_unregister() won't work (because there still are bonds). The
> attached patch should fix it, but I haven't tried running uacce_register()
> yet.
Have tested, it is OK.

Thanks



More information about the Linux-accelerators mailing list