iopoll bio pcpu cache fix
Subsystems (2)
cover iopoll bio pcpu cache fix
There are ways to deprive bioset mempool of requests using pcpu caches and never return them back, which breaks forward progress guarantees bioset tried to provide. Fix it.
Pavel Begunkov (2):
mempool: introduce mempool_is_saturated
bio: don't rob bios from starving bioset
block/bio.c | 2 ++ include/linux/mempool.h | 5 +++++ 2 files changed, 7 insertions(+)
Signature · 1 line
2.38.0
PATCH 1/2 mempool: introduce mempool_is_saturated
Introduce a helper mempool_is_saturated(), which tells if the mempool is under-filled or not. We need it to figure out whether it should be freed right into the mempool or could be cached with top level caches.
--- include/linux/mempool.h | 5 +++++ 1 file changed, 5 insertions(+)
include/linux/mempool.h +5 -0
@@ -30,6 +30,11 @@ static inline bool mempool_initialized(mempool_t *pool)return pool->elements !=NULL ; } +static inline bool mempool_is_saturated (mempool_t *pool) +{ +return READ_ONCE (pool->curr_nr) >= pool->min_nr; +} +void mempool_exit (mempool_t *pool); int mempool_init_node(mempool_t *pool, int min_nr, mempool_alloc_t *alloc_fn, mempool_free_t *free_fn, void *pool_data,
Signature · 1 line
2.38.0
PATCH 2/2 bio: don't rob bios from starving bioset
In reply to Pavel BegunkovBiosets keep a mempool, so as long as requests complete we can always can allocate and have forward progress. Percpu bio caches break that assumptions as we may complete into the cache of one CPU and after try and fail to allocate with another CPU. We also can't grab from another CPU's cache without tricky sync.
If we're allocating with a bio while the mempool is undersaturated, remove REQ_ALLOC_CACHE flag, so on put it will go straight to mempool. It might try to free into mempool more requests than required, but assuming than there is no memory starvation in the system it'll stabilise and never hit that path.
--- block/bio.c | 2 ++ 1 file changed, 2 insertions(+)
block/bio.c +2 -0
@@ -526,6 +526,8 @@ struct bio *bio_alloc_bioset(struct block_device *bdev, unsigned short nr_vecs, }if (unlikely (!p))return NULL ; +if (!mempool_is_saturated (&bs->bio_pool)) + opf &= ~REQ_ALLOC_CACHE ; bio = p + bs->front_pad;if (nr_vecs >BIO_INLINE_VECS ) {
Signature · 1 line
2.38.0
Jens Axboe
maintainerQuoted 4 lines · Pavel Begunkov
There are ways to deprive bioset mempool of requests using pcpu caches and never return them back, which breaks forward progress guarantees bioset tried to provide. Fix it.
Quoted 8 lines
Pavel Begunkov (2):
mempool: introducemempool_is_saturated
bio: don't rob bios from starving biosetblock/bio.c | 2 ++ include/linux/mempool.h | 5 +++++ 2 files changed, 7 insertions(+)
This isn't really a concern for 6.1 and earlier, because the caching is just for polled IO. Polled IO will not be grabbing any of the reserved inflight units on the mempool side, which is what guarantees the forward progress.
It'll be a concern for the 6.2 irq based general caching, so it would need to be handled there. So perhaps this can be a pre-series for a reposting of that patchset.
Signature · 1 line
Jens Axboe
Jens Axboe
maintainerQuoted 14 lines · Jens Axboe
Quoted 4 lines · Pavel Begunkov
There are ways to deprive bioset mempool of requests using pcpu caches and never return them back, which breaks forward progress guarantees bioset tried to provide. Fix it.
Quoted 8 lines
Pavel Begunkov (2):
mempool: introducemempool_is_saturated
bio: don't rob bios from starving biosetblock/bio.c | 2 ++ include/linux/mempool.h | 5 +++++ 2 files changed, 7 insertions(+)
Quoted 8 lines
This isn't really a concern for 6.1 and earlier, because the caching is just for polled IO. Polled IO will not be grabbing any of the reserved inflight units on the mempool side, which is what guarantees the forward progress.
It'll be a concern for the 6.2 irq based general caching, so it would need to be handled there. So perhaps this can be a pre-series for a reposting of that patchset.
Just a followup, since we had some out-of-band discussion. This is a potential concern on the bio side, though not on the request side. But this approach is racy, we'll figure something else out.
Signature · 1 line
Jens Axboe
Quoted 15 lines · Jens Axboe
Quoted 14 lines · Jens Axboe
Quoted 12 lines · Pavel Begunkov
There are ways to deprive bioset mempool of requests using pcpu caches and never return them back, which breaks forward progress guarantees bioset tried to provide. Fix it.
Pavel Begunkov (2):
mempool: introducemempool_is_saturated
bio: don't rob bios from starving biosetblock/bio.c | 2 ++ include/linux/mempool.h | 5 +++++ 2 files changed, 7 insertions(+)
Quoted 4 lines
Quoted 4 lines
This isn't really a concern for 6.1 and earlier, because the caching is just for polled IO. Polled IO will not be grabbing any of the reserved inflight units on the mempool side, which is what guarantees the forward progress.
And after looking it up, apparently it can allocate from reserves.
Quoted 7 lines
Quoted 3 lines
It'll be a concern for the 6.2 irq based general caching, so it would need to be handled there. So perhaps this can be a pre-series for a reposting of that patchset.
Just a followup, since we had some out-of-band discussion. This is a potential concern on the bio side, though not on the request side. But this approach is racy, we'll figure something else out.
I agree that it _may_ be, but I can't think of an counter example for arches w/ strong ordering and would need to think more if there is an issue in the looser ephemeral world.
Signature · 1 line
Pavel Begunkov
Quoted 28 lines · Pavel Begunkov
Quoted 15 lines · Jens Axboe
Quoted 14 lines · Jens Axboe
Quoted 12 lines · Pavel Begunkov
There are ways to deprive bioset mempool of requests using pcpu caches and never return them back, which breaks forward progress guarantees bioset tried to provide. Fix it.
Pavel Begunkov (2):
mempool: introducemempool_is_saturated
bio: don't rob bios from starving biosetblock/bio.c | 2 ++
include/linux/mempool.h| 5 +++++2 files changed, 7 insertions(+)Quoted 4 lines
Quoted 4 lines
This isn't really a concern for 6.1 and earlier, because the caching is just for polled IO. Polled IO will not be grabbing any of the reserved inflight units on the mempool side, which is what guarantees the forward progress.
And after looking it up, apparently it can allocate from reserves.
Quoted 4 lines
Quoted 3 lines
It'll be a concern for the 6.2 irq based general caching, so it would need to be handled there. So perhaps this can be a pre-series for a reposting of that patchset.
Quoted 7 lines
Quoted 3 lines
Just a followup, since we had some out-of-band discussion. This is a potential concern on the bio side, though not on the request side. But this approach is racy, we'll figure something else out.
I agree that it _may_ be, but I can't think of an counter example for arches w/ strong ordering and would need to think more if there is an issue in the looser ephemeral world.
Let me try to prove it's not a problem:
It's not interesting if didn't allocate from reserves as it won't affect mempool. Neither we care about falsely avoiding pcpu cache, i.e. trying to return into the mempool more than needed. So, the only potential race problem that requires explanation is when the bio is from reserves but we see that the mempool is full and it's not. Because of spinlocks around ->curr_nr modifications and the fact that we won't see random invented values, it might have only happened if there were both bio allocations and bio puts strictly after our allocation, after spin_unlock to be more specific. But then those puts and allocs are serialised by the mutex, we can take a bio alloc that happened right after a put that filled the mempool full and recurse all the reasoning.
I'm rusty on formal proves and it can be more formal, but would be interesting to see if it's flawed and/or get a counter example. In any case, it only returns more bios into mempool, which won't hurt the issue.
Signature · 1 line
Pavel Begunkov