Re: linux-next: manual merge of the akpm-current tree with the xen-tip tree

[Boris Ostrovsky]

On 07/30/2018 01:02 PM, Boris Ostrovsky wrote:
> On 07/30/2018 05:02 AM, Stephen Rothwell wrote:
>> Hi all,
>>
>> Today's linux-next merge of the akpm-current tree got a conflict in:
>>
>>   drivers/xen/gntdev.c
>>
>> between commit:
>>
>>   1d3145675538 ("xen/gntdev: Make private routines/structures accessible")
>>
>> from the xen-tip tree and commit:
>>
>>   aaefcabe9c25 ("mm, oom: distinguish blockable mode for mmu notifiers")
>>
>> from the akpm-current tree.
>>
>> I fixed it up (see below) and can carry the fix as necessary. This
>> is now fixed as far as linux-next is concerned, but any non trivial
>> conflicts should be mentioned to your upstream maintainer when your tree
>> is submitted for merging.  You may also want to consider cooperating
>> with the maintainer of the conflicting tree to minimise any particularly
>> complex conflicts.
>>
>> -- Cheers, Stephen Rothwell diff --cc drivers/xen/gntdev.c index
>> c866a62f766d,55b4f0e3f4d6..000000000000 --- a/drivers/xen/gntdev.c +++
>> b/drivers/xen/gntdev.c @@@ -479,7 -441,20 +479,20 @@@ static const
>> struct vm_operations_struc /*
>> ------------------------------------------------------------------ */
>> -static bool in_range(struct grant_map *map, ++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 grant_map *map, +static void
>> unmap_if_in_range(struct gntdev_grant_map *map, unsigned long start,
>> unsigned long end) { unsigned long mstart, mend; @@@ -503,15 -472,26
>> +510,26 @@@ WARN_ON(err); } - static void mn_invl_range_start(struct
>> mmu_notifier *mn, + static int mn_invl_range_start(struct mmu_notifier
>> *mn, struct mm_struct *mm, - unsigned long start, unsigned long end) +
>> unsigned long start, unsigned long end, + bool blockable) { struct
>> gntdev_priv *priv = container_of(mn, struct gntdev_priv, mn); - struct
>> grant_map *map; + 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; - mutex_lock(&priv->lock); list_for_each_entry(map,
>> &priv->maps, next) { + if (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) {


Ugh... That's some interesting whitespace optimization on part of
thundebird. Let me paste the relevant patch hunk here.


diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c
index bd56653b9bbc..55b4f0e3f4d6 100644
--- a/drivers/xen/gntdev.c
+++ b/drivers/xen/gntdev.c
@@ -441,18 +441,25 @@ static const struct vm_operations_struct gntdev_vmops = {
 
 /* ------------------------------------------------------------------ */
 
+static bool in_range(struct 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 grant_map *map,
 			      unsigned long start, unsigned long end)
 {
 	unsigned long mstart, mend;
 	int err;
 
-	if (!map->vma)
-		return;
-	if (map->vma->vm_start >= end)
-		return;
-	if (map->vma->vm_end <= start)
-		return;
 	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",
@@ -465,21 +472,40 @@ static void unmap_if_in_range(struct grant_map *map,
 	WARN_ON(err);
 }
 
-static void mn_invl_range_start(struct mmu_notifier *mn,
+static int mn_invl_range_start(struct mmu_notifier *mn,
 				struct mm_struct *mm,
-				unsigned long start, unsigned long end)
+				unsigned long start, unsigned long end,
+				bool blockable)
 {
 	struct gntdev_priv *priv = container_of(mn, struct gntdev_priv, mn);
 	struct 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;
 
-	mutex_lock(&priv->lock);
 	list_for_each_entry(map, &priv->maps, next) {
+		if (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)) {
+			ret = -EAGAIN;
+			goto out_unlock;
+		}
 		unmap_if_in_range(map, start, end);
 	}
+
+out_unlock:
 	mutex_unlock(&priv->lock);
+
+	return ret;
 }


-boris



>
> I clearly missed this (aaefcabe9c25) patch but now that I am looking at
> it I don't think I understand the logic for changes in
> list_for_each_entry() loops.
>
> Aren't we ending up never unmapping grant pages? Michal, can you explain
> what you are trying to do here?
>
>
> -boris
>
>
>

Attachment: signature.asc
Description: OpenPGP digital signature