Re: [PATCH] xen/gntdev: fix up blockable calls to mn_invl_range_start
From: Juergen Gross
Date: Fri Aug 24 2018 - 01:08:29 EST
On 23/08/18 21:09, Michal Hocko wrote:
> On Thu 23-08-18 10:06:53, Boris Ostrovsky wrote:
>> On 08/23/2018 09:51 AM, Michal Hocko wrote:
>>> On Thu 23-08-18 22:44:07, Tetsuo Handa wrote:
>>>> On 2018/08/23 21:07, Michal Hocko wrote:
>>>>> 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
>
> Yeah. I was half way out and posted a complete garbage. Sorry about
> that!
>
> Michal repeat after me
> Never post patches when in hurry! Never post patches when in hurry!
> Never post patches when in hurry! Never post patches when in hurry!
> Never post patches when in hurry! Never post patches when in hurry!
> Never post patches when in hurry! Never post patches when in hurry!
> Never post patches when in hurry! Never post patches when in hurry!
>
> What I really meant was this
>
> diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c
> index e7d8bb1bee2a..6fcc5a44f29d 100644
> --- a/drivers/xen/gntdev.c
> +++ b/drivers/xen/gntdev.c
> @@ -525,17 +525,25 @@ 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))
> + continue;
> +
> + if (!blockable) {
> ret = -EAGAIN;
> goto out_unlock;
> }
> +
> unmap_if_in_range(map, start, end);
> }
> list_for_each_entry(map, &priv->freeable_maps, next) {
> - if (!blockable && in_range(map, start, end)) {
> + if (!in_range(map, start, end))
> + continue;
> +
> + if (!blockable) {
> ret = -EAGAIN;
> goto out_unlock;
> }
> +
> unmap_if_in_range(map, start, end);
> }
>
>
I liked the general structure before 93065ac753e4 better.
Why don't you return to that, add blockable parameter to
unmap_if_in_range() and let unmap_if_in_range() return a value (0 or
-EAGAIN)? This will avoid repeating the very same code.
So:
--- a/drivers/xen/gntdev.c
+++ b/drivers/xen/gntdev.c
@@ -479,25 +479,21 @@ 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 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 (!map->vma)
+ return 0;
+ if (map->vma->vm_start >= end)
+ return 0;
+ if (map->vma->vm_end <= start)
+ 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 +504,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 +517,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:
Juergen