[PATCH 3/3] drm/xe: Get page on user fence creation

[Matthew Brost]

On Tue, Feb 27, 2024 at 01:38:27PM +0100, Thomas Hellström wrote:
> On Tue, 2024-02-27 at 12:01 +0100, Thomas Hellström wrote:
> > On Mon, 2024-02-26 at 18:43 -0800, Matthew Brost wrote:
> > > Attempt to get page on user fence creation and kmap_local_page on
> > > signaling. Should reduce latency and can ensure 64 bit atomicity
> > > compared to copy_to_user.
> > > 
> > > Signed-off-by: Matthew Brost <matthew.brost at intel.com>
> > > ---
> > >  drivers/gpu/drm/xe/xe_sync.c | 45 ++++++++++++++++++++++++++++----
> > > --
> > > --
> > >  1 file changed, 36 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/xe/xe_sync.c
> > > b/drivers/gpu/drm/xe/xe_sync.c
> > > index 022e158d28d9..2f3e062c0101 100644
> > > --- a/drivers/gpu/drm/xe/xe_sync.c
> > > +++ b/drivers/gpu/drm/xe/xe_sync.c
> > > @@ -6,6 +6,7 @@
> > >  #include "xe_sync.h"
> > >  
> > >  #include <linux/dma-fence-array.h>
> > > +#include <linux/highmem.h>
> > >  #include <linux/kthread.h>
> > >  #include <linux/sched/mm.h>
> > >  #include <linux/uaccess.h>
> > > @@ -25,6 +26,7 @@ struct user_fence {
> > >  	struct dma_fence_cb cb;
> > >  	struct work_struct worker;
> > >  	struct mm_struct *mm;
> > > +	struct page *page;
> > >  	u64 __user *addr;
> > >  	u64 value;
> > >  };
> > > @@ -34,7 +36,10 @@ static void user_fence_destroy(struct kref
> > > *kref)
> > >  	struct user_fence *ufence = container_of(kref, struct
> > > user_fence,
> > >  						 refcount);
> > >  
> > > -	mmdrop(ufence->mm);
> > > +	if (ufence->page)
> > > +		put_page(ufence->page);
> > > +	else if (ufence->mm)
> > > +		mmdrop(ufence->mm);
> > >  	kfree(ufence);
> > >  }
> > >  
> > > @@ -53,6 +58,7 @@ static struct user_fence
> > > *user_fence_create(struct
> > > xe_device *xe, u64 addr,
> > >  {
> > >  	struct user_fence *ufence;
> > >  	u64 __user *ptr = u64_to_user_ptr(addr);
> > > +	int ret;
> > >  
> > >  	if (!access_ok(ptr, sizeof(ptr)))
> > >  		return ERR_PTR(-EFAULT);
> > > @@ -66,7 +72,11 @@ static struct user_fence
> > > *user_fence_create(struct
> > > xe_device *xe, u64 addr,
> > >  	ufence->addr = ptr;
> > >  	ufence->value = value;
> > >  	ufence->mm = current->mm;
> > > -	mmgrab(ufence->mm);
> > > +	ret = get_user_pages_fast(addr, 1, FOLL_WRITE, &ufence-
> > > > page);
> > 
> > Hmm. This is mid-term pinning a page-cache page. We shouldn't really
> > do
> > that since it interferes with huge pages and numa migration.
> > 
> > What about just prefaulting and dropping the refcount and then we
> > do this again when signalling?
> > 
> > > +	if (ret != 1) {
> > > +		mmgrab(ufence->mm);
> > > +		ufence->page = NULL;
> > > +	}
> > >  
> > >  	return ufence;
> > >  }
> > > @@ -74,13 +84,30 @@ static struct user_fence
> > > *user_fence_create(struct xe_device *xe, u64 addr,
> > >  static void user_fence_worker(struct work_struct *w)
> > >  {
> > >  	struct user_fence *ufence = container_of(w, struct
> > > user_fence, worker);
> > > -
> > > -	if (mmget_not_zero(ufence->mm)) {
> > > -		kthread_use_mm(ufence->mm);
> > > -		if (copy_to_user(ufence->addr, &ufence->value,
> > > sizeof(ufence->value)))
> > > -			XE_WARN_ON("Copy to user failed");
> > > -		kthread_unuse_mm(ufence->mm);
> > > -		mmput(ufence->mm);
> > > +	struct mm_struct *mm = ufence->mm;
> > > +
> > > +	if (mmget_not_zero(mm)) {
> > > +		if (ufence->page) {
> > > +			u64 *ptr;
> > > +			void *va;
> > 
> > 
> > > +
> > > +			va = kmap_local_page(ufence->page);
> > > +			ptr = va + offset_in_page(ufence->addr);
> > > +			xchg(ptr, ufence->value);
> > 
> > Does this compile on 32-bit?
> 
> It doesn't. Would probably need a try_cmpxchg64() loop.
> 

Thanks, will fix.

Matt

> /Thomas
>