Messages in this thread | | | Subject | Re: [PATCH 2/3] lightnvm: encapsule rqd dma allocations | From | Matias Bjørling <> | Date | Wed, 29 Aug 2018 15:42:57 +0200 |
| |
On 08/29/2018 03:41 PM, Javier Gonzalez wrote: > >> On 29 Aug 2018, at 15.36, Matias Bjørling <mb@lightnvm.io> wrote: >> >> On 08/29/2018 03:18 PM, Javier Gonzalez wrote: >>>> On 29 Aug 2018, at 15.00, Matias Bjørling <mb@lightnvm.io> wrote: >>>> >>>> On 08/29/2018 10:56 AM, Javier González wrote: >>>>> dma allocations for ppa_list and meta_list in rqd are replicated in >>>>> several places across the pblk codebase. Make helpers to encapsulate >>>>> creation and deletion to simplify the code. >>>>> Signed-off-by: Javier González <javier@cnexlabs.com> >>>>> --- >>>>> drivers/lightnvm/pblk-core.c | 69 +++++++++++++++++++++++++--------------- >>>>> drivers/lightnvm/pblk-read.c | 35 ++++++++++---------- >>>>> drivers/lightnvm/pblk-recovery.c | 29 ++++++----------- >>>>> drivers/lightnvm/pblk-write.c | 15 ++------- >>>>> drivers/lightnvm/pblk.h | 3 ++ >>>>> 5 files changed, 74 insertions(+), 77 deletions(-) >>>>> diff --git a/drivers/lightnvm/pblk-core.c b/drivers/lightnvm/pblk-core.c >>>>> index 09160ec02c5f..767178185f19 100644 >>>>> --- a/drivers/lightnvm/pblk-core.c >>>>> +++ b/drivers/lightnvm/pblk-core.c >>>>> @@ -237,6 +237,34 @@ static void pblk_invalidate_range(struct pblk *pblk, sector_t slba, >>>>> spin_unlock(&pblk->trans_lock); >>>>> } >>>>> +int pblk_setup_rqd(struct pblk *pblk, struct nvm_rq *rqd, gfp_t mem_flags, >>>>> + bool is_vector) >>>> >>>> >>>> The mem_flags argument can be removed. It is GFP_KERNEL from all the >>>> places it is called. >>> Thought it was better to have the flexibility in a helper function, but >>> we can always add it later on if needed... >>>> is_vector, will be better to do nr_ppas (or similar name). Then >>>> pblk_submit_read/pblk_submit_read_gc are a bit cleaner. >>> We can do that too, yes. >>>>> +{ >>>>> + struct nvm_tgt_dev *dev = pblk->dev; >>>>> + >>>>> + rqd->meta_list = nvm_dev_dma_alloc(dev->parent, mem_flags, >>>>> + &rqd->dma_meta_list); >>>>> + if (!rqd->meta_list) >>>>> + return -ENOMEM; >>>>> + >>>>> + if (!is_vector) >>>>> + return 0; >>>>> + >>>>> + rqd->ppa_list = rqd->meta_list + pblk_dma_meta_size; >>>>> + rqd->dma_ppa_list = rqd->dma_meta_list + pblk_dma_meta_size; >>>> >>>> Wrt to is_vector, does it matter if we just set ppa_list and >>>> dma_ppa_list? If we have them, we use them, else leave them alone? >>> If we only have 1 address then ppa_addr is set and the ppa_list attempt >>> to free in the completion path interpreting ppa_addr as the dma address. >>> So I don't think so - unless I'm missing something? >> >> In that case, we could drop is_vector/nr_ppas all together? That would be nice. >> > > The problem is that the metadata region still needs to be used, even if > the ppa_list is not set. Thing that the oob area can be larger than > 64bits, so we cannot do the dma address is the actual value trick. > > So if encapsulating, we need to know if we need to allocate the ppa_list > or not. > > Does it make sense?
Cool. We'll just leave it as is.
| |