Re: [PATCH] xen/gntdev: fix up blockable calls to mn_invl_range_start
From: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Date: 2018-08-23 14:05:49
Also in:
lkml
On 08/23/2018 09:51 AM, Michal Hocko wrote:
quoted hunk ↗ jump to hunk
On Thu 23-08-18 22:44:07, Tetsuo Handa wrote:quoted
On 2018/08/23 21:07, Michal Hocko wrote:quoted
diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c index 57390c7666e5..e7d8bb1bee2a 100644 --- a/drivers/xen/gntdev.c +++ b/drivers/xen/gntdev.c@@ -519,21 +519,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)) { + if (!blockable && in_range(map, start, end)) {This still looks strange. Prior to 93065ac753e4, in_range() test was inside unmap_if_in_range(). But this patch removes in_range() test if blockable == true. That is, unmap_if_in_range() will unconditionally unmap if blockable == true, which seems to be an unexpected change.You are right. I completely forgot I've removed in_range there. Does this look any better?diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c index e7d8bb1bee2a..30f81004ea63 100644 --- a/drivers/xen/gntdev.c +++ b/drivers/xen/gntdev.c@@ -525,14 +525,20 @@ static int mn_invl_range_start(struct mmu_notifier *mn, return -EAGAIN; list_for_each_entry(map, &priv->maps, next) { - if (!blockable && in_range(map, start, end)) { + if (in_range(map, start, end)) { + if (blockable) + continue; + ret = -EAGAIN; goto out_unlock; } unmap_if_in_range(map, start, end);
(I obviously missed that too with my R-b).
This will never get anything done either. How about
A A A if (in_range()) {
A A A A A A if (!blockable) {
A A A A A A A A A ret = -EGAIN;
A A A A A A A A A goto out_unlock;
A A A A A A }
A A A A A A unmap_range(); // new name since unmap_if_in_range() doesn't
perform any checks now
A A A }
-boris
}
list_for_each_entry(map, &priv->freeable_maps, next) {
- if (!blockable && in_range(map, start, end)) {
+ if (in_range(map, start, end)) {
+ if (blockable)
+ continue;
+
ret = -EAGAIN;
goto out_unlock;
}