drivers, vmw_balloon.c: Increment alloc and sleep_alloc only when page allocation succeeds.

While doing allocation statistics in vmballoon_reserve_page function,
alloc and sleep_alloc has been incremented even if allocation fails. But,
b->stats.alloc and b->stats.sleep_alloc supposed to increment only when they succeed. This patch makes sure that, alloc and sleep_alloc gets incremented when page allocation succeeds.

Signed-off-by Rakib Mullick <[email protected]>

---

drivers/misc/vmw_balloon.c +5 -5

index 053d36c..d2c25c9 100644
@@ -414,11 +414,6 @@ static int vmballoon_reserve_page(struct vmballoon *b, bool can_sleep)
 	bool locked = false;
 
 	do {
-		if (!can_sleep)
-			STATS_INC(b->stats.alloc);
-		else
-			STATS_INC(b->stats.sleep_alloc);
-
 		flags = can_sleep ? VMW_PAGE_ALLOC_CANSLEEP : VMW_PAGE_ALLOC_NOSLEEP;
 		page = alloc_page(flags);
 		if (!page) {

@@ -427,6 +422,11 @@ static int vmballoon_reserve_page(struct vmballoon *b, bool can_sleep)
 			else
 				STATS_INC(b->stats.sleep_alloc_fail);
 			return -ENOMEM;
+		} else {
+			if (!can_sleep)
+				STATS_INC(b->stats.alloc);
+			else
+				STATS_INC(b->stats.sleep_alloc);
 		}
 
 		/* inform monitor */

Quoted 7 lines · Rakib Mullick

While doing allocation statistics in vmballoon_reserve_page function,

alloc and sleep_alloc has been incremented even if allocation fails. But,
b->stats.alloc and b->stats.sleep_alloc supposed to increment only when they succeed. This patch makes sure that, alloc and sleep_alloc gets incremented when page allocation succeeds.

Dmitry could say for sure, but this seems to actually change the semantics. If the allocations fail, it increments alloc_fail and sleep_alloc_fail accordingly so you could easily see 10 alloc and 5 alloc_fail. With your patch, it would be 5 alloc and 5 alloc_fail.

I don't know which one is best, but I would opt to stay with the semantics that alloc and sleep_alloc have already had rather than changing them.

Quoted 9 lines · David Rientjes

Quoted 7 lines · Rakib Mullick

 While doing allocation statistics in vmballoon_reserve_page function,

alloc and sleep_alloc has been incremented even if allocation fails. But,
b->stats.alloc and b->stats.sleep_alloc supposed to increment only when they succeed. This patch makes sure that, alloc and sleep_alloc gets incremented when page allocation succeeds.

Quoted 8 lines

Dmitry could say for sure, but this seems to actually change the semantics.  If the allocations fail, it increments alloc_fail and sleep_alloc_fail accordingly so you could easily see 10 alloc and 5 alloc_fail.  With your patch, it would be 5 alloc and 5 alloc_fail.

I don't know which one is best, but I would opt to stay with the semantics that alloc and sleep_alloc have already had rather than changing them.

Right, b->stats.alloc and b->stats.sleep_alloc show number of allocation attempts and alloc_fail and sleep_alloc_fail show how many of these attempts failed. This behavior matches behavior of the driver we have been shipping out of the tree for many years and I would prefer to keep it as is.

Quoted 20 lines · Dmitry Torokhov

Quoted 10 lines · David Rientjes

Quoted 8 lines · Rakib Mullick

While doing allocation statistics in vmballoon_reserve_page
function,

alloc and sleep_alloc has been incremented even if allocation fails. But,
b->stats.alloc and b->stats.sleep_alloc supposed to increment only when they succeed. This patch makes sure that, alloc and sleep_alloc gets incremented when page allocation succeeds.

Quoted 8 lines

Dmitry could say for sure, but this seems to actually change the semantics. If the allocations fail, it increments alloc_fail and sleep_alloc_fail accordingly so you could easily see 10 alloc and 5 alloc_fail. With your patch, it would be 5 alloc and 5 alloc_fail.

I don't know which one is best, but I would opt to stay with the semantics that alloc and sleep_alloc have already had rather than changing them.

Quoted 5 lines

Right, b->stats.alloc and b->stats.sleep_alloc show number of allocation attempts and alloc_fail and sleep_alloc_fail show how many of these attempts failed. This behavior matches behavior of the driver we have been shipping out of the tree for many years and I would prefer to keep it as is.

Then why b->stats.alloc and b->stats.sleep_alloc are not renamed as b->stats.alloc_attempted and b->stats.sleep_alloc_attempted respectively. Current naming is confusing, anyone looking at the alloc and sleep_alloc stat would easily think that, if 10 alloc and 5 alloc_fail then 10 has been allocated and 5 times has been allocation fails. Isn't it?