| Message ID | 20180827112623.8992-2-mhocko@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mmu_notifiers follow ups | expand |
On 08/27/2018 07:26 AM, Michal Hocko wrote: > From: Michal Hocko <mhocko@suse.com> > > 93065ac753e4 ("mm, oom: distinguish blockable mode for mmu notifiers") > has introduced blockable parameter to all mmu_notifiers and the notifier > has to back off when called in !blockable case and it could block down > the road. > > The above commit implemented that for mn_invl_range_start but both > in_range checks are done unconditionally regardless of the blockable > mode and as such they would fail all the time for regular calls. > Fix this by checking blockable parameter as well. > > Once we are there we can remove the stale TODO. The lock has to be > sleepable because we wait for completion down in gnttab_unmap_refs_sync. > > Changes since v1 > - pull in_range check into mn_invl_range_start - Juergen > > Fixes: 93065ac753e4 ("mm, oom: distinguish blockable mode for mmu notifiers") > Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com> > Cc: Juergen Gross <jgross@suse.com> > Signed-off-by: Michal Hocko <mhocko@suse.com> LGTM, although in_range() has a single call site so we really don't need it. I'll wait for Juergen to bless this since he asked for this approach. -boris > --- > drivers/xen/gntdev.c | 26 +++++++++++++++----------- > 1 file changed, 15 insertions(+), 11 deletions(-) > > diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c > index 57390c7666e5..b0b02a501167 100644 > --- a/drivers/xen/gntdev.c > +++ b/drivers/xen/gntdev.c > @@ -492,12 +492,19 @@ static bool in_range(struct gntdev_grant_map *map, > return true; > } > > -static void unmap_if_in_range(struct gntdev_grant_map *map, > - unsigned long start, unsigned long end) > +static int unmap_if_in_range(struct gntdev_grant_map *map, > + unsigned long start, unsigned long end, > + bool blockable) > { > unsigned long mstart, mend; > int err; > > + if (!in_range(map, start, end)) > + return 0; > + > + if (!blockable) > + return -EAGAIN; > + > mstart = max(start, map->vma->vm_start); > mend = min(end, map->vma->vm_end); > pr_debug("map %d+%d (%lx %lx), range %lx %lx, mrange %lx %lx\n", > @@ -508,6 +515,8 @@ static void unmap_if_in_range(struct gntdev_grant_map *map, > (mstart - map->vma->vm_start) >> PAGE_SHIFT, > (mend - mstart) >> PAGE_SHIFT); > WARN_ON(err); > + > + return 0; > } > > static int mn_invl_range_start(struct mmu_notifier *mn, > @@ -519,25 +528,20 @@ static int mn_invl_range_start(struct mmu_notifier *mn, > struct gntdev_grant_map *map; > int ret = 0; > > - /* TODO do we really need a mutex here? */ > if (blockable) > mutex_lock(&priv->lock); > else if (!mutex_trylock(&priv->lock)) > return -EAGAIN; > > list_for_each_entry(map, &priv->maps, next) { > - if (in_range(map, start, end)) { > - ret = -EAGAIN; > + ret = unmap_if_in_range(map, start, end, blockable); > + if (ret) > goto out_unlock; > - } > - unmap_if_in_range(map, start, end); > } > list_for_each_entry(map, &priv->freeable_maps, next) { > - if (in_range(map, start, end)) { > - ret = -EAGAIN; > + ret = unmap_if_in_range(map, start, end, blockable); > + if (ret) > goto out_unlock; > - } > - unmap_if_in_range(map, start, end); > } > > out_unlock:
On 27/08/18 13:26, Michal Hocko wrote: > From: Michal Hocko <mhocko@suse.com> > > 93065ac753e4 ("mm, oom: distinguish blockable mode for mmu notifiers") > has introduced blockable parameter to all mmu_notifiers and the notifier > has to back off when called in !blockable case and it could block down > the road. > > The above commit implemented that for mn_invl_range_start but both > in_range checks are done unconditionally regardless of the blockable > mode and as such they would fail all the time for regular calls. > Fix this by checking blockable parameter as well. > > Once we are there we can remove the stale TODO. The lock has to be > sleepable because we wait for completion down in gnttab_unmap_refs_sync. > > Changes since v1 > - pull in_range check into mn_invl_range_start - Juergen > > Fixes: 93065ac753e4 ("mm, oom: distinguish blockable mode for mmu notifiers") > Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com> > Cc: Juergen Gross <jgross@suse.com> > Signed-off-by: Michal Hocko <mhocko@suse.com> Reviewed-by: Juergen Gross <jgross@suse.com> Juergen
diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c index 57390c7666e5..b0b02a501167 100644 --- a/drivers/xen/gntdev.c +++ b/drivers/xen/gntdev.c @@ -492,12 +492,19 @@ static bool in_range(struct gntdev_grant_map *map, return true; } -static void unmap_if_in_range(struct gntdev_grant_map *map, - unsigned long start, unsigned long end) +static int unmap_if_in_range(struct gntdev_grant_map *map, + unsigned long start, unsigned long end, + bool blockable) { unsigned long mstart, mend; int err; + if (!in_range(map, start, end)) + return 0; + + if (!blockable) + return -EAGAIN; + mstart = max(start, map->vma->vm_start); mend = min(end, map->vma->vm_end); pr_debug("map %d+%d (%lx %lx), range %lx %lx, mrange %lx %lx\n", @@ -508,6 +515,8 @@ static void unmap_if_in_range(struct gntdev_grant_map *map, (mstart - map->vma->vm_start) >> PAGE_SHIFT, (mend - mstart) >> PAGE_SHIFT); WARN_ON(err); + + return 0; } static int mn_invl_range_start(struct mmu_notifier *mn, @@ -519,25 +528,20 @@ static int mn_invl_range_start(struct mmu_notifier *mn, struct gntdev_grant_map *map; int ret = 0; - /* TODO do we really need a mutex here? */ if (blockable) mutex_lock(&priv->lock); else if (!mutex_trylock(&priv->lock)) return -EAGAIN; list_for_each_entry(map, &priv->maps, next) { - if (in_range(map, start, end)) { - ret = -EAGAIN; + ret = unmap_if_in_range(map, start, end, blockable); + if (ret) goto out_unlock; - } - unmap_if_in_range(map, start, end); } list_for_each_entry(map, &priv->freeable_maps, next) { - if (in_range(map, start, end)) { - ret = -EAGAIN; + ret = unmap_if_in_range(map, start, end, blockable); + if (ret) goto out_unlock; - } - unmap_if_in_range(map, start, end); } out_unlock: