mempool: drop unnecessary and incorrect BUG_ON() from mempool_destroy()

mempool_destroy() is a thin wrapper around free_pool(). The only thing it adds is BUG_ON(pool->curr_nr != pool->min_nr). The intention seems to be to enforce that all allocated elements are freed; however, the BUG_ON() can't achieve that (it doesn't know anything about objects above min_nr) and incorrect as mempool_resize() is allowed to leave the pool extended but not filled. Furthermore, panicking is way worse than any memory leak and there are better debug tools to track memory leaks.

Drop the BUG_ON() from mempool_destory() and as that leaves the function identical to free_pool(), replace it.

Signed-off-by Tejun Heo <[email protected]>

Cc Andrew Morton <[email protected]>

Cc [email protected]

---
These patches are on top of "mempool: fix and document synchronization and memory barrier usage" patch[1]. Both are fixes and it probably is a good idea to forward to -stable.

For the initial allocation, mempool passes modified gfp mask to the backing allocator so that it doesn't try too hard when there are reserved elements waiting in the pool; however, when that allocation fails and pool is empty too, it either waits for the pool to be replenished before retrying or fails if !__GFP_WAIT.

* If the caller was calling in with GFP_ATOMIC, it never gets to try emergency reserve. Allocations which would have succeeded without mempool may fail, which is just wrong.

* Allocation which could have succeeded after a bit of reclaim now has to wait on the reserved items and it's not like mempool doesn't retry with the original gfp mask. It just does that *after* someone returns an element, pointlessly delaying things.

This is a significant change in behaviour.  Previously the mempool code would preserve emergency pools while waiting for someone to return an item.  Now, it will permit many more items to be allocated, chewing into the emergency pools.

We *know* that items will soon become available, so why not wait for that to happen rather than consuming memory which less robust callers could have utilised?

IOW, this change appears to make the kernel more vulnerable to memory exhaustion failures?

Quoted 3 lines

Signed-off-by Tejun Heo <[email protected]>

Cc Andrew Morton <[email protected]>

Cc [email protected]

(that's [email protected])

Quoted 4 lines

---
These patches are on top of "mempool: fix and document synchronization and memory barrier usage" patch[1]. Both are fixes and it probably is a good idea to forward to -stable.

I'm not sure that either of these are suitable for -stable. There's no demonstrated problem, nor even a likely theoretical one, is there?

If we do decide to backport, I don't think the -stable guys will want the large-but-nice comment-adding patch so both these patches would need to be reworked for -stable usage. The first patch does apply successfully to mainline. The second does not.

Hello,

Quoted 4 lines · Andrew Morton

Quoted 3 lines

Signed-off-by Tejun Heo <[email protected]>

Cc Andrew Morton <[email protected]>

Cc [email protected]

Quoted 1 line

(that's [email protected])

(cc'ing Greg)

It has been [email protected] for quite a while and Greg scans for that Cc. Even MAINTAINERS has that as the official mail address. I heard that the mailing alias is broken at the moment but wouldn't it be better to fix that?

mempool modifies gfp_mask so that the backing allocator doesn't try too hard or trigger warning message when there's pool to fall back on. In addition, for the first try, it removes __GFP_WAIT and IO, so that it doesn't trigger reclaim or wait when allocation can be fulfilled from pool; however, when that allocation fails and pool is empty too, it waits for the pool to be replenished before retrying.

Allocation which could have succeeded after a bit of reclaim has to wait on the reserved items and it's not like mempool doesn't retry with __GFP_WAIT and IO. It just does that *after* someone returns an element, pointlessly delaying things.

Fix it by retrying immediately if the first round of allocation attempts w/o __GFP_WAIT and IO fails.

That said, I still find it a bit unsettling that a GFP_ATOMIC allocation which would otherwise succeed may fail when issued through mempool.

Maybe the RTTD is clearing __GFP_NOMEMALLOC on retry if the gfp requsted by the caller is !__GFP_WAIT && !__GFP_NOMEMALLOC?

If the pool is empty and the memory allocator is down into its emergency reserves then we have:

Old behaviour: Wait for someone to return an item, then retry

New behaviour: enable page reclaim in gfp_mask, retry a
single time then wait for someone to return an item.

So what we can expect to see is that in this low-memory situation, mempool_alloc() will perform a lot more page reclaim, and more mempool items will be let loose into the kernel.

I'm not sure what the effects of this will be. I can't immediately point at any bad ones. Probably not much, as the mempool_alloc() caller will probably be doing other allocations, using the reclaim-permitting gfp_mask.

But I have painful memories of us (me and Jens, iirc) churning this code over and over again until it stopped causing problems. Some were subtle and nasty. Much dumpster diving into the pre-git changelogs should be done before changing it, lest we rediscover long-fixed problems :(

Quoted 3 lines

That said, I still find it a bit unsettling that a GFP_ATOMIC allocation which would otherwise succeed may fail when issued through mempool.

Spose so. It would be strange to call mempool_alloc() with GFP_ATOMIC. Because "wait for an item to be returned" is the whole point of the thing.

Quoted 2 lines

Maybe the RTTD is clearing __GFP_NOMEMALLOC on retry if the gfp requsted by the caller is !__GFP_WAIT && !__GFP_NOMEMALLOC?

What the heck is an RTTD?

Quoted 4 lines

(quoted diff) +4 -0

+	/*
+	 * We use gfp mask w/o __GFP_WAIT or IO for the first round.  If
+	 * alloc failed with that and @pool was empty, retry immediately.
+	 */

Review

Quoted 6 lines

(quoted diff) +6 -0

+	if (gfp_temp != gfp_mask) {
+		gfp_temp = gfp_mask;
+		spin_unlock_irqrestore(&pool->lock, flags);
+		goto repeat_alloc;
+	}
+

Here, have a faster kernel ;)

I see. It just seemed like a weird behavior and looking at the commit log, there was originally code to kick reclaim there, so the sequence made sense - first try w/o reclaim, look at the mempool, kick reclaim and retry w/ GFP_WAIT and then wait for someone else to free. That part was removed by 20a77776c24 "[PATCH] mempool: simplify alloc" back in 05. In the process, it also lost retry w/ reclaim before waiting for mempool reserves.

Hello, Andrew.

Quoted 17 lines · Andrew Morton

If the pool is empty and the memory allocator is down into its emergency reserves then we have:

Old behaviour: Wait for someone to return an item, then retry

New behaviour: enable page reclaim in gfp_mask, retry a
single time then wait for someone to return an item.

So what we can expect to see is that in this low-memory situation, mempool_alloc() will perform a lot more page reclaim, and more mempool items will be let loose into the kernel.

I'm not sure what the effects of this will be. I can't immediately point at any bad ones. Probably not much, as the mempool_alloc() caller will probably be doing other allocations, using the reclaim-permitting gfp_mask.

Quoted 5 lines

But I have painful memories of us (me and Jens, iirc) churning this code over and over again until it stopped causing problems. Some were subtle and nasty. Much dumpster diving into the pre-git changelogs should be done before changing it, lest we rediscover long-fixed problems :(

I see. It just seemed like a weird behavior and looking at the commit log, there was originally code to kick reclaim there, so the sequence made sense - first try w/o reclaim, look at the mempool, kick reclaim and retry w/ GFP_WAIT and then wait for someone else to free. That part was removed by 20a77776c24 "[PATCH] mempool: simplify alloc" back in 05. In the process, it also lost retry w/ reclaim before waiting for mempool reserves.

I was trying to add percpu mempool and this bit me as percpu allocator can't to NOIO and the above delayed retry logic ended up adding random 5s delay (or until the next free).

For blkio to use these percpu mempool, I guess we shall have to change this notion that an allocated object from mempool is returned to the pool soon. We might not be returning these per cpu stat objects to mempool for a very long time (These will be returned to pool only when somebody deletes the cgroup).

Quoted 34 lines · Tejun Heo

Hello, Andrew.

Quoted 17 lines · Andrew Morton

If the pool is empty and the memory allocator is down into its emergency reserves then we have:

Old behaviour: Wait for someone to return an item, then retry

New behaviour: enable page reclaim in gfp_mask, retry a
single time then wait for someone to return an item.

So what we can expect to see is that in this low-memory situation, mempool_alloc() will perform a lot more page reclaim, and more mempool items will be let loose into the kernel.

I'm not sure what the effects of this will be. I can't immediately point at any bad ones. Probably not much, as the mempool_alloc() caller will probably be doing other allocations, using the reclaim-permitting gfp_mask.

Quoted 5 lines

But I have painful memories of us (me and Jens, iirc) churning this code over and over again until it stopped causing problems. Some were subtle and nasty. Much dumpster diving into the pre-git changelogs should be done before changing it, lest we rediscover long-fixed problems :(

I see. It just seemed like a weird behavior and looking at the commit log, there was originally code to kick reclaim there, so the sequence made sense - first try w/o reclaim, look at the mempool, kick reclaim and retry w/ GFP_WAIT and then wait for someone else to free. That part was removed by 20a77776c24 "[PATCH] mempool: simplify alloc" back in 05. In the process, it also lost retry w/ reclaim before waiting for mempool reserves.

Quoted 4 lines

I was trying to add percpu mempool and this bit me as percpu allocator can't to NOIO and the above delayed retry logic ended up adding random 5s delay (or until the next free).

Tejun,

For blkio to use these percpu mempool, I guess we shall have to change this notion that an allocated object from mempool is returned to the pool soon. We might not be returning these per cpu stat objects to mempool for a very long time (These will be returned to pool only when somebody deletes the cgroup).

Also mempool expects all the objects to be returned to the pool at the time of destroy. Given the fact that blkio groups are reference counted, theoritically they can be freed later.

Hello,

Quoted 3 lines · Vivek Goyal

Also mempool expects all the objects to be returned to the pool at the time of destroy. Given the fact that blkio groups are reference counted, theoritically they can be freed later.

I removed that part but no you still can't free objects after mempool destruction refcnted or not. mempool is destructed on module unload. Which text would free those objects? If that's theoretically possible, the code is broken.

That's a good point. I did not think about module unload. I think then my current patch for allocating per cpu object from worker thread is buggy as on IO scheduler exit I don't try to flush the possibly in progress work[s].