Skip to contentSkip to conversation

libibverbs: Undo changes in memory range tree when madvise() fails

PATCH In review Alex Vainman 2 participants 3 messages Mar 28, 2010 – Apr 21, 2010 1 message not in the archive InfiniBand

lore
Share & tools
lore
#0 lore raw

PATCH v3 4/4 libibverbs: Undo changes in memory range tree when madvise() fails

ibv_madvise_range() doesn't cleanup if madvise() fails. This patch comes to roll back the changes, made in memory tree, which preceded the madvise() failure:

When madvise() fails on a memory range portion out of the whole range which user requested to modify and ibv_madvise_range() successfully modified a few tree nodes up to the problematical portion sub-ranges (this can happen if there is an overlap between user's range and range's which where previously added to the memory tree) then it is not enough to undo the split and merge operation performed on the current node, which caused the failure, but the functions needed to undo all the changes made on all the previous ranges from start pointer to current location.
The patch revertes all the changes by re-running it self from start pointer to current location with toggled inc value.

---
 src/memory.c |   21 ++++++++++++++++++---
 1 files changed, 18 insertions(+), 3 deletions(-)
src/memory.c +18 -3
index 03f49c8..14f5bc5 100644
@@ -527,18 +527,19 @@ static int ibv_madvise_range(void *base, size_t size, int advice)
 	uintptr_t start, end;
 	struct ibv_mem_node *node, *tmp;
 	int inc;
+	int rolling_back = 0;
 	int ret = 0;
 
 	if (!size)
 		return 0;
 
-	inc = advice == MADV_DONTFORK ? 1 : -1;
-
 	start = (uintptr_t) base & ~(page_size - 1);
 	end   = ((uintptr_t) (base + size + page_size - 1) &
 		 ~(page_size - 1)) - 1;
 
 	pthread_mutex_lock(&mm_mutex);
+again:
+	inc = advice == MADV_DONTFORK ? 1 : -1;
 
 	node = get_start_node(start, end, inc);
 	if (!node) {
@@ -576,7 +577,19 @@ static int ibv_madvise_range(void *base, size_t size, int advice)
 					      advice);
 			if (ret) {
 				node = undo_node(node, start, inc);
-				goto out;
+
+				if (rolling_back || !node)
+					goto out;
+
+				/* madvise failed, roll back previous changes */
+				rolling_back = 1;
+				advice = advice == MADV_DONTFORK ? MADV_DOFORK :
+								   MADV_DONTFORK;
+				tmp = __mm_prev(node);
+				if (!tmp || start > tmp->end)
+					goto out;
+				end = tmp->end;
+				goto again;
 			}
 		}
 
@@ -591,6 +604,8 @@ static int ibv_madvise_range(void *base, size_t size, int advice)
 	}
 
 out:
+	if (rolling_back)
+		ret = -1;
 	pthread_mutex_unlock(&mm_mutex);
 
 	return ret;
Signature · 6 lines
1.6.5.3

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
#2 lore raw
In reply to a message not in the archive

Hi Roland,

I'v sent you in my previous mail version 3 of the part 4 of the patch: "Undo changes in memory range tree when madvise() fails", updated according to yours recommedations. There are just two exceptions:

1.The lines:
589- if (!tmp || start)
590- goto out;-

I've changed to:
589+ if (!tmp || start > tmp->end)
590+ goto out;

Since, if the first sub range causes to madvice failure (so: start > tmp->end), there is no need to start the roll back mechanism, which can incorrectly modify, in this case, the memory tree.

2. Added

607+ if (rolling_back)
608+ ret = -1;

Since the rollback proccess can change the value of the ret varibale, can happen that madvice fails but the function returns "0"! This change was done in order to avoid that scenario.

-Alex

Alex Vainman Wrote:

Quoted 68 lines

ibv_madvise_range() doesn't cleanup if madvise() fails. This patch comes to roll back the changes, made in memory tree, which preceded the madvise() failure:

When madvise() fails on a memory range portion out of the whole range which user requested to modify and ibv_madvise_range() successfully modified a few tree nodes up to the problematical portion sub-ranges (this can happen if there is an overlap between user's range and range's which where previously added to the memory tree) then it is not enough to undo the split and merge operation performed on the current node, which caused the failure, but the functions needed to undo all the changes made on all the previous ranges from start pointer to current location.
The patch revertes all the changes by re-running it self from start pointer to current location with toggled inc value.

---
 src/memory.c |   21 ++++++++++++++++++---
 1 files changed, 18 insertions(+), 3 deletions(-)
src/memory.c +16 -3
index 03f49c8..14f5bc5 100644
@@ -527,18 +527,19 @@ static int ibv_madvise_range(void *base, size_t size, int advice)
 	uintptr_t start, end;
 	struct ibv_mem_node *node, *tmp;
 	int inc;
+	int rolling_back = 0;
 	int ret = 0;
 
 	if (!size)
 		return 0;
 
-	inc = advice == MADV_DONTFORK ? 1 : -1;
-
 	start = (uintptr_t) base & ~(page_size - 1);
 	end   = ((uintptr_t) (base + size + page_size - 1) &
 		 ~(page_size - 1)) - 1;
 
 	pthread_mutex_lock(&mm_mutex);
+again:
+	inc = advice == MADV_DONTFORK ? 1 : -1;
 
 	node = get_start_node(start, end, inc);
 	if (!node) {
@@ -576,7 +577,19 @@ static int ibv_madvise_range(void *base, size_t size, int advice)
 					      advice);
 			if (ret) {
 				node = undo_node(node, start, inc);
-				goto out;
+
+				if (rolling_back || !node)
+					goto out;
+
+				/* madvise failed, roll back previous changes */
+				rolling_back = 1;
+				advice = advice == MADV_DONTFORK ? MADV_DOFORK :
+								   MADV_DONTFORK;
+				tmp = __mm_prev(node);
+				if (!tmp || start > tmp->end)
+					goto out;
+				end = tmp->end;
+				goto again;
 			}
 		}
 

src/memory.c static int ibv_madvise_range(void *base, size_t size, int advice) · line 611

Quoted 8 lines
src/memory.c +2 -0
@@ -591,6 +604,8 @@ static int ibv_madvise_range(void *base, size_t size, int advice)
 	}
 
 out:
+	if (rolling_back)
+		ret = -1;
 	pthread_mutex_unlock(&mm_mutex);
 
 	return ret;

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html