Re: [PATCH v2 09/15] xen/gntdev: use mmu_range_notifier_insert

2019-11-08 Thread Jason Gunthorpe
On Tue, Nov 05, 2019 at 10:16:46AM -0500, Boris Ostrovsky wrote:

> > So, I suppose it can be relaxed to a null test and a WARN_ON that it
> > hasn't changed?
> 
> You mean
> 
> if (use_ptemod) {
>     WARN_ON(map->vma != vma);
>     ...
> 
> 
> Yes, that sounds good.

I amended my copy of the patch with the above, has this rework shown
signs of working?

@@ -436,7 +436,8 @@ static void gntdev_vma_close(struct vm_area_struct *vma)
struct gntdev_priv *priv = file->private_data;
 
pr_debug("gntdev_vma_close %p\n", vma);
-   if (use_ptemod && map->vma == vma) {
+   if (use_ptemod) {
+   WARN_ON(map->vma != vma);
mmu_range_notifier_remove(>notifier);
map->vma = NULL;
}

Jason
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 09/15] xen/gntdev: use mmu_range_notifier_insert

2019-11-07 Thread Boris Ostrovsky
On 11/7/19 3:36 PM, Jason Gunthorpe wrote:
> On Tue, Nov 05, 2019 at 10:16:46AM -0500, Boris Ostrovsky wrote:
>
>>> So, I suppose it can be relaxed to a null test and a WARN_ON that it
>>> hasn't changed?
>> You mean
>>
>> if (use_ptemod) {
>>     WARN_ON(map->vma != vma);
>>     ...
>>
>>
>> Yes, that sounds good.
> I amended my copy of the patch with the above, has this rework shown
> signs of working?

Yes, it works fine.

But please don't forget notifier ops initialization.

With those two changes,

Reviewed-by: Boris Ostrovsky 

>
> @@ -436,7 +436,8 @@ static void gntdev_vma_close(struct vm_area_struct *vma)
> struct gntdev_priv *priv = file->private_data;
>  
> pr_debug("gntdev_vma_close %p\n", vma);
> -   if (use_ptemod && map->vma == vma) {
> +   if (use_ptemod) {
> +   WARN_ON(map->vma != vma);
> mmu_range_notifier_remove(>notifier);
> map->vma = NULL;
> }
>
> Jason

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 09/15] xen/gntdev: use mmu_range_notifier_insert

2019-11-05 Thread Boris Ostrovsky
On 11/4/19 9:31 PM, Jason Gunthorpe wrote:
> On Mon, Nov 04, 2019 at 05:03:31PM -0500, Boris Ostrovsky wrote:
>> On 10/28/19 4:10 PM, Jason Gunthorpe wrote:
>>> @@ -445,17 +438,9 @@ static void gntdev_vma_close(struct vm_area_struct 
>>> *vma)
>>> struct gntdev_priv *priv = file->private_data;
>>>  
>>> pr_debug("gntdev_vma_close %p\n", vma);
>>> -   if (use_ptemod) {
>>> -   /* It is possible that an mmu notifier could be running
>>> -* concurrently, so take priv->lock to ensure that the vma won't
>>> -* vanishing during the unmap_grant_pages call, since we will
>>> -* spin here until that completes. Such a concurrent call will
>>> -* not do any unmapping, since that has been done prior to
>>> -* closing the vma, but it may still iterate the unmap_ops list.
>>> -*/
>>> -   mutex_lock(>lock);
>>> +   if (use_ptemod && map->vma == vma) {
>>
>> Is it possible for map->vma not to be equal to vma?
> It could be NULL at least if use_ptemod is not set.
>
> Otherwise, I'm not sure, the confusing bit is that the map comes from
> here:
>
> map = gntdev_find_map_index(priv, index, count);
>
> It looks like the intent is that the map->vma is always set to the
> only vma that has the map as private_data.

I am not sure how this can work otherwise. We stash map pointer in vm's
vm_private_data and vice versa (for use_ptemod) gntdev_mmap() so if they
have to match.

That's why I was asking you to see if you had something particular in
mind when you added this test.

> So, I suppose it can be relaxed to a null test and a WARN_ON that it
> hasn't changed?

You mean

if (use_ptemod) {
    WARN_ON(map->vma != vma);
    ...


Yes, that sounds good.


-boris
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 09/15] xen/gntdev: use mmu_range_notifier_insert

2019-10-31 Thread Boris Ostrovsky
On 10/28/19 4:10 PM, Jason Gunthorpe wrote:
> From: Jason Gunthorpe 
>
> gntdev simply wants to monitor a specific VMA for any notifier events,
> this can be done straightforwardly using mmu_range_notifier_insert() over
> the VMA's VA range.
>
> The notifier should be attached until the original VMA is destroyed.
>
> It is unclear if any of this is even sane, but at least a lot of duplicate
> code is removed.

I didn't have a chance to look at the patch itself yet but as a heads-up
--- it crashes dom0.

-boris


>
> Cc: Oleksandr Andrushchenko 
> Cc: Boris Ostrovsky 
> Cc: xen-de...@lists.xenproject.org
> Cc: Juergen Gross 
> Cc: Stefano Stabellini 
> Signed-off-by: Jason Gunthorpe 
> ---
>  drivers/xen/gntdev-common.h |   8 +-
>  drivers/xen/gntdev.c| 180 ++--
>  2 files changed, 49 insertions(+), 139 deletions(-)
>
> diff --git a/drivers/xen/gntdev-common.h b/drivers/xen/gntdev-common.h
> index 2f8b949c3eeb14..b201fdd20b667b 100644
> --- a/drivers/xen/gntdev-common.h
> +++ b/drivers/xen/gntdev-common.h
> @@ -21,15 +21,8 @@ struct gntdev_dmabuf_priv;
>  struct gntdev_priv {
>   /* Maps with visible offsets in the file descriptor. */
>   struct list_head maps;
> - /*
> -  * Maps that are not visible; will be freed on munmap.
> -  * Only populated if populate_freeable_maps == 1
> -  */
> - struct list_head freeable_maps;
>   /* lock protects maps and freeable_maps. */
>   struct mutex lock;
> - struct mm_struct *mm;
> - struct mmu_notifier mn;
>  
>  #ifdef CONFIG_XEN_GRANT_DMA_ALLOC
>   /* Device for which DMA memory is allocated. */
> @@ -49,6 +42,7 @@ struct gntdev_unmap_notify {
>  };
>  
>  struct gntdev_grant_map {
> + struct mmu_range_notifier notifier;
>   struct list_head next;
>   struct vm_area_struct *vma;
>   int index;
> diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c
> index a446a7221e13e9..12d626670bebbc 100644
> --- a/drivers/xen/gntdev.c
> +++ b/drivers/xen/gntdev.c
> @@ -65,7 +65,6 @@ MODULE_PARM_DESC(limit, "Maximum number of grants that may 
> be mapped by "
>  static atomic_t pages_mapped = ATOMIC_INIT(0);
>  
>  static int use_ptemod;
> -#define populate_freeable_maps use_ptemod
>  
>  static int unmap_grant_pages(struct gntdev_grant_map *map,
>int offset, int pages);
> @@ -251,12 +250,6 @@ void gntdev_put_map(struct gntdev_priv *priv, struct 
> gntdev_grant_map *map)
>   evtchn_put(map->notify.event);
>   }
>  
> - if (populate_freeable_maps && priv) {
> - mutex_lock(>lock);
> - list_del(>next);
> - mutex_unlock(>lock);
> - }
> -
>   if (map->pages && !use_ptemod)
>   unmap_grant_pages(map, 0, map->count);
>   gntdev_free_map(map);
> @@ -445,17 +438,9 @@ static void gntdev_vma_close(struct vm_area_struct *vma)
>   struct gntdev_priv *priv = file->private_data;
>  
>   pr_debug("gntdev_vma_close %p\n", vma);
> - if (use_ptemod) {
> - /* It is possible that an mmu notifier could be running
> -  * concurrently, so take priv->lock to ensure that the vma won't
> -  * vanishing during the unmap_grant_pages call, since we will
> -  * spin here until that completes. Such a concurrent call will
> -  * not do any unmapping, since that has been done prior to
> -  * closing the vma, but it may still iterate the unmap_ops list.
> -  */
> - mutex_lock(>lock);
> + if (use_ptemod && map->vma == vma) {
> + mmu_range_notifier_remove(>notifier);
>   map->vma = NULL;
> - mutex_unlock(>lock);
>   }
>   vma->vm_private_data = NULL;
>   gntdev_put_map(priv, map);
> @@ -477,109 +462,44 @@ static const struct vm_operations_struct gntdev_vmops 
> = {
>  
>  /* -- */
>  
> -static bool in_range(struct gntdev_grant_map *map,
> -   unsigned long start, unsigned long end)
> -{
> - if (!map->vma)
> - return false;
> - if (map->vma->vm_start >= end)
> - return false;
> - if (map->vma->vm_end <= start)
> - return false;
> -
> - return true;
> -}
> -
> -static int unmap_if_in_range(struct gntdev_grant_map *map,
> -   unsigned long start, unsigned long end,
> -   bool blockable)
> +static bool gntdev_invalidate(struct mmu_range_notifier *mn,
> +   const struct mmu_notifier_range *range,
> +   unsigned long cur_seq)
>  {
> + struct gntdev_grant_map *map =
> + container_of(mn, struct gntdev_grant_map, notifier);
>   unsigned long mstart, mend;
>   int err;
>  
> - if (!in_range(map, start, end))
> - return 0;
> + if (!mmu_notifier_range_blockable(range))
> +