| Message ID | 1504710066-4699-2-git-send-email-javier@cnexlabs.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Sep 06, 2017 at 05:01:01PM +0200, Javier González wrote:
> Check for failed mempool allocations and act accordingly.
Are you sure it is needed? Quoting from mempool_alloc()s Documentation:
"[...] Note that due to preallocation, this function *never* fails when called
from process contexts. (it might fail if called from an IRQ context.) [...]"
Byte,
Johannes
On 09/06/2017 09:08 AM, Johannes Thumshirn wrote: > On Wed, Sep 06, 2017 at 05:01:01PM +0200, Javier González wrote: >> Check for failed mempool allocations and act accordingly. > > Are you sure it is needed? Quoting from mempool_alloc()s Documentation: > "[...] Note that due to preallocation, this function *never* fails when called > from process contexts. (it might fail if called from an IRQ context.) [...]" It's not needed, mempool() will never fail if __GFP_WAIT is set in the mask. The use case here is GFP_KERNEL, which does include __GFP_WAIT.
> On 6 Sep 2017, at 17.09, Jens Axboe <axboe@kernel.dk> wrote: > > On 09/06/2017 09:08 AM, Johannes Thumshirn wrote: >> On Wed, Sep 06, 2017 at 05:01:01PM +0200, Javier González wrote: >>> Check for failed mempool allocations and act accordingly. >> >> Are you sure it is needed? Quoting from mempool_alloc()s Documentation: >> "[...] Note that due to preallocation, this function *never* fails when called >> from process contexts. (it might fail if called from an IRQ context.) [...]" > > It's not needed, mempool() will never fail if __GFP_WAIT is set in the > mask. The use case here is GFP_KERNEL, which does include __GFP_WAIT. Thanks for the clarification. Do you just drop the patch, or do you want me to re-send the series? I see that I do this other places, I'll clean it up for next window. Thanks, Javier
On Wed, Sep 06, 2017 at 09:09:29AM -0600, Jens Axboe wrote: > On 09/06/2017 09:08 AM, Johannes Thumshirn wrote: > > On Wed, Sep 06, 2017 at 05:01:01PM +0200, Javier González wrote: > >> Check for failed mempool allocations and act accordingly. > > > > Are you sure it is needed? Quoting from mempool_alloc()s Documentation: > > "[...] Note that due to preallocation, this function *never* fails when called > > from process contexts. (it might fail if called from an IRQ context.) [...]" > > It's not needed, mempool() will never fail if __GFP_WAIT is set in the > mask. The use case here is GFP_KERNEL, which does include __GFP_WAIT. Exactly. Maybe I shouldn't have it phrased as a question though...
On 09/06/2017 09:12 AM, Javier González wrote: >> On 6 Sep 2017, at 17.09, Jens Axboe <axboe@kernel.dk> wrote: >> >> On 09/06/2017 09:08 AM, Johannes Thumshirn wrote: >>> On Wed, Sep 06, 2017 at 05:01:01PM +0200, Javier González wrote: >>>> Check for failed mempool allocations and act accordingly. >>> >>> Are you sure it is needed? Quoting from mempool_alloc()s Documentation: >>> "[...] Note that due to preallocation, this function *never* fails when called >>> from process contexts. (it might fail if called from an IRQ context.) [...]" >> >> It's not needed, mempool() will never fail if __GFP_WAIT is set in the >> mask. The use case here is GFP_KERNEL, which does include __GFP_WAIT. > > Thanks for the clarification. Do you just drop the patch, or do you want > me to re-send the series? No need to resend. I'll pick up the others in a day or two, once people have had some time to go over them.
> On 6 Sep 2017, at 17.13, Jens Axboe <axboe@kernel.dk> wrote: > > On 09/06/2017 09:12 AM, Javier González wrote: >>> On 6 Sep 2017, at 17.09, Jens Axboe <axboe@kernel.dk> wrote: >>> >>> On 09/06/2017 09:08 AM, Johannes Thumshirn wrote: >>>> On Wed, Sep 06, 2017 at 05:01:01PM +0200, Javier González wrote: >>>>> Check for failed mempool allocations and act accordingly. >>>> >>>> Are you sure it is needed? Quoting from mempool_alloc()s Documentation: >>>> "[...] Note that due to preallocation, this function *never* fails when called >>>> from process contexts. (it might fail if called from an IRQ context.) [...]" >>> >>> It's not needed, mempool() will never fail if __GFP_WAIT is set in the >>> mask. The use case here is GFP_KERNEL, which does include __GFP_WAIT. >> >> Thanks for the clarification. Do you just drop the patch, or do you want >> me to re-send the series? > > No need to resend. I'll pick up the others in a day or two, once people > have had some time to go over them. > Thanks. And apologies for the delay on the patches... Javier
On 09/06/2017 09:13 AM, Jens Axboe wrote: > On 09/06/2017 09:12 AM, Javier González wrote: >>> On 6 Sep 2017, at 17.09, Jens Axboe <axboe@kernel.dk> wrote: >>> >>> On 09/06/2017 09:08 AM, Johannes Thumshirn wrote: >>>> On Wed, Sep 06, 2017 at 05:01:01PM +0200, Javier González wrote: >>>>> Check for failed mempool allocations and act accordingly. >>>> >>>> Are you sure it is needed? Quoting from mempool_alloc()s Documentation: >>>> "[...] Note that due to preallocation, this function *never* fails when called >>>> from process contexts. (it might fail if called from an IRQ context.) [...]" >>> >>> It's not needed, mempool() will never fail if __GFP_WAIT is set in the >>> mask. The use case here is GFP_KERNEL, which does include __GFP_WAIT. >> >> Thanks for the clarification. Do you just drop the patch, or do you want >> me to re-send the series? > > No need to resend. I'll pick up the others in a day or two, once people > have had some time to go over them. I took a quick look at your mempool usage, and I'm not sure it's correct. For a mempool to work, you have to ensure that you provide a forward progress guarantee. With that guarantee, you know that if you do end up sleeping on allocation, you already have items inflight that will be freed when that operation completes. In other words, all allocations must have a defined and finite life time, as any allocation can potentially sleep/block for that life time. You can't allocate something and hold on to it forever, then you are violating the terms of agreement that makes a mempool work. The first one that caught my eye is pblk->page_pool. You have this loop: for (i = 0; i < nr_pages; i++) { page = mempool_alloc(pblk->page_pool, flags); if (!page) goto err; ret = bio_add_pc_page(q, bio, page, PBLK_EXPOSED_PAGE_SIZE, 0); if (ret != PBLK_EXPOSED_PAGE_SIZE) { pr_err("pblk: could not add page to bio\n"); mempool_free(page, pblk->page_pool); goto err; } } which looks suspect. This mempool is created with a reserve pool of PAGE_POOL_SIZE (16) members. Do we know if the bio has 16 pages or less? If not, then this is broken and can deadlock forever. You have a lot of mempool usage in the code, would probably not hurt to audit all of them.
> On 6 Sep 2017, at 17.20, Jens Axboe <axboe@kernel.dk> wrote: > > On 09/06/2017 09:13 AM, Jens Axboe wrote: >> On 09/06/2017 09:12 AM, Javier González wrote: >>>> On 6 Sep 2017, at 17.09, Jens Axboe <axboe@kernel.dk> wrote: >>>> >>>> On 09/06/2017 09:08 AM, Johannes Thumshirn wrote: >>>>> On Wed, Sep 06, 2017 at 05:01:01PM +0200, Javier González wrote: >>>>>> Check for failed mempool allocations and act accordingly. >>>>> >>>>> Are you sure it is needed? Quoting from mempool_alloc()s Documentation: >>>>> "[...] Note that due to preallocation, this function *never* fails when called >>>>> from process contexts. (it might fail if called from an IRQ context.) [...]" >>>> >>>> It's not needed, mempool() will never fail if __GFP_WAIT is set in the >>>> mask. The use case here is GFP_KERNEL, which does include __GFP_WAIT. >>> >>> Thanks for the clarification. Do you just drop the patch, or do you want >>> me to re-send the series? >> >> No need to resend. I'll pick up the others in a day or two, once people >> have had some time to go over them. > > I took a quick look at your mempool usage, and I'm not sure it's > correct. For a mempool to work, you have to ensure that you provide a > forward progress guarantee. With that guarantee, you know that if you do > end up sleeping on allocation, you already have items inflight that will > be freed when that operation completes. In other words, all allocations > must have a defined and finite life time, as any allocation can > potentially sleep/block for that life time. You can't allocate something > and hold on to it forever, then you are violating the terms of agreement > that makes a mempool work. I understood the part of guaranteeing the number of inflight items to keep the mempool active without waiting, but I must admit that I assumed that the mempool would resize when getting pressure and that the penalty would be increased latency, not the mempool giving up and causing a deadlock. > > The first one that caught my eye is pblk->page_pool. You have this loop: > > for (i = 0; i < nr_pages; i++) { > page = mempool_alloc(pblk->page_pool, flags); > if (!page) > goto err; > > ret = bio_add_pc_page(q, bio, page, PBLK_EXPOSED_PAGE_SIZE, 0); > if (ret != PBLK_EXPOSED_PAGE_SIZE) { > pr_err("pblk: could not add page to bio\n"); > mempool_free(page, pblk->page_pool); > goto err; > } > } > > which looks suspect. This mempool is created with a reserve pool of > PAGE_POOL_SIZE (16) members. Do we know if the bio has 16 pages or less? > If not, then this is broken and can deadlock forever. I can see that in this case, the 16 elements do not hold. In the read path, we can guarantee that a read will be <= 64 sectors (4KB pages), so this is definitely wrong. I'll fix it tomorrow. Since we are at it, I have for some time wondered what's the right way to balance the mempool sizes so that we are a good citizen and perform at the same time. I don't see a lot of code using mempool_resize to tune the min_nr based on load. For example, in our write path, the numbers are easy to calculate, but on the read path I completely over-dimensioned the mempool to ensure not having to wait for the completion path. Any good rule of thumb here? > You have a lot of mempool usage in the code, would probably not hurt to > audit all of them. Yes. I will take a look and add comments to the sizes. Thanks Jens, Javier
diff --git a/drivers/lightnvm/pblk-core.c b/drivers/lightnvm/pblk-core.c index 81501644fb15..acb07bbcb416 100644 --- a/drivers/lightnvm/pblk-core.c +++ b/drivers/lightnvm/pblk-core.c @@ -165,6 +165,8 @@ struct nvm_rq *pblk_alloc_rqd(struct pblk *pblk, int rw) } rqd = mempool_alloc(pool, GFP_KERNEL); + if (!rqd) + return NULL; memset(rqd, 0, rq_size); return rqd; @@ -1478,6 +1480,8 @@ int pblk_blk_erase_async(struct pblk *pblk, struct ppa_addr ppa) int err; rqd = mempool_alloc(pblk->g_rq_pool, GFP_KERNEL); + if (!rqd) + return -ENOMEM; memset(rqd, 0, pblk_g_rq_size); pblk_setup_e_rq(pblk, rqd, ppa);