[PATCH] xen/gntdev: fix up blockable calls to mn_invl_range_start

[PATCH] xen/gntdev: fix up blockable calls to mn_invl_range_start

[Michal Hocko]

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.

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>
---
 drivers/xen/gntdev.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

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)) {
 			ret = -EAGAIN;
 			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)) {
+		if (!blockable && in_range(map, start, end)) {
 			ret = -EAGAIN;
 			goto out_unlock;
 		}
-- 
2.18.0

[PATCH] xen/gntdev: fix up blockable calls to mn_invl_range_start

[Michal Hocko]

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.

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>
---
 drivers/xen/gntdev.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

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)) {
 			ret = -EAGAIN;
 			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)) {
+		if (!blockable && in_range(map, start, end)) {
 			ret = -EAGAIN;
 			goto out_unlock;
 		}
-- 
2.18.0

Re: [PATCH] xen/gntdev: fix up blockable calls to mn_invl_range_start

[Tetsuo Handa]

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.

>  			ret = -EAGAIN;
>  			goto out_unlock;
>  		}
>  		unmap_if_in_range(map, start, end);
>  	}

Re: [PATCH] xen/gntdev: fix up blockable calls to mn_invl_range_start

[Tetsuo Handa]

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.

>  			ret = -EAGAIN;
>  			goto out_unlock;
>  		}
>  		unmap_if_in_range(map, start, end);
>  	}

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] xen/gntdev: fix up blockable calls to mn_invl_range_start

[Boris Ostrovsky]

On 08/23/2018 08:07 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.
>
> 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: Boris Ostrovsky <boris.ostrovsky@oracel.com>

Re: [PATCH] xen/gntdev: fix up blockable calls to mn_invl_range_start

[Boris Ostrovsky]

On 08/23/2018 08:07 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.
>
> 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: Boris Ostrovsky <boris.ostrovsky@oracel.com>



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] xen/gntdev: fix up blockable calls to mn_invl_range_start

[Michal Hocko]

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);
 	}
 	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;
 		}
-- 
Michal Hocko
SUSE Labs

Re: [PATCH] xen/gntdev: fix up blockable calls to mn_invl_range_start

[Michal Hocko]

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);
 	}
 	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;
 		}
-- 
Michal Hocko
SUSE Labs

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] xen/gntdev: fix up blockable calls to mn_invl_range_start

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

    if (in_range()) {
        if (!blockable) {
            ret = -EGAIN;
            goto out_unlock;
        }
        unmap_range(); // new name since unmap_if_in_range() doesn't
perform any checks now
    }



-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;
>  		}

Re: [PATCH] xen/gntdev: fix up blockable calls to mn_invl_range_start

[Boris Ostrovsky]

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

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;
>  		}

Re: [PATCH] xen/gntdev: fix up blockable calls to mn_invl_range_start

[Boris Ostrovsky]

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

    if (in_range()) {
        if (!blockable) {
            ret = -EGAIN;
            goto out_unlock;
        }
        unmap_range(); // new name since unmap_if_in_range() doesn't
perform any checks now
    }



-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;
>  		}


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] xen/gntdev: fix up blockable calls to mn_invl_range_start

[Tetsuo Handa]

On 2018/08/23 22:51, Michal Hocko wrote:
> 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;
>  		}

This still looks strange. in_range() returns false if map->vma == NULL.
But unmap_if_in_range() unconditionally dereferences map->vma->vm_[start|end] .
Suggestion from Boris looks better.

>  		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)) {
> +			if (blockable)
> +				continue;
> +			
>  			ret = -EAGAIN;
>  			goto out_unlock;
>  		}
> 

Re: [PATCH] xen/gntdev: fix up blockable calls to mn_invl_range_start

[Tetsuo Handa]

On 2018/08/23 22:51, Michal Hocko wrote:
> 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;
>  		}

This still looks strange. in_range() returns false if map->vma == NULL.
But unmap_if_in_range() unconditionally dereferences map->vma->vm_[start|end] .
Suggestion from Boris looks better.

>  		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)) {
> +			if (blockable)
> +				continue;
> +			
>  			ret = -EAGAIN;
>  			goto out_unlock;
>  		}
> 


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] xen/gntdev: fix up blockable calls to mn_invl_range_start

[Michal Hocko]

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);
 	}
 
-- 
Michal Hocko
SUSE Labs

Re: [PATCH] xen/gntdev: fix up blockable calls to mn_invl_range_start

[Michal Hocko]

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);
 	}
 
-- 
Michal Hocko
SUSE Labs

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] xen/gntdev: fix up blockable calls to mn_invl_range_start

[Juergen Gross]

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

Re: [PATCH] xen/gntdev: fix up blockable calls to mn_invl_range_start

[Juergen Gross]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] xen/gntdev: fix up blockable calls to mn_invl_range_start

[Michal Hocko]

On Fri 24-08-18 07:03:28, Juergen Gross wrote:
> 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.

I can do that if that is your preference of course. I have even
considered that but then I have got to this...

> @@ -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;
>  }

and I really didn't know what to do about that. On one hand the error
has been ignored already, on the other hand it is just too ugly to
ignore it when we do provide a return value. Moreover I can see how
somebody would like to clean that up later and I am not sure what are we
going to do about it in callers.

But by all means, I will go the way you maintainers prefer.
-- 
Michal Hocko
SUSE Labs

Re: [PATCH] xen/gntdev: fix up blockable calls to mn_invl_range_start

[Michal Hocko]

On Fri 24-08-18 07:03:28, Juergen Gross wrote:
> 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.

I can do that if that is your preference of course. I have even
considered that but then I have got to this...

> @@ -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;
>  }

and I really didn't know what to do about that. On one hand the error
has been ignored already, on the other hand it is just too ugly to
ignore it when we do provide a return value. Moreover I can see how
somebody would like to clean that up later and I am not sure what are we
going to do about it in callers.

But by all means, I will go the way you maintainers prefer.
-- 
Michal Hocko
SUSE Labs

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH] xen/gntdev: fix up blockable calls to mn_invl_range_start

[Michal Hocko]

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.

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>
---
 drivers/xen/gntdev.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

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)) {
 			ret = -EAGAIN;
 			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)) {
+		if (!blockable && in_range(map, start, end)) {
 			ret = -EAGAIN;
 			goto out_unlock;
 		}
-- 
2.18.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel