> On 29 Aug 2018, at 15.00, Matias Bjørling <mb@xxxxxxxxxxx> 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@xxxxxxxxxxxx> >> --- >> 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? >> + >> + return 0; >> +} >> + >> +void pblk_clear_rqd(struct pblk *pblk, struct nvm_rq *rqd) >> +{ >> + struct nvm_tgt_dev *dev = pblk->dev; >> + >> + if (rqd->meta_list) >> + nvm_dev_dma_free(dev->parent, rqd->meta_list, >> + rqd->dma_meta_list); >> +} > > Looks like setup/clear is mainly about managing the metadata. Would > pblk_alloc_rqd_meta()/pblk_free/rqd_meta be better names? Unless we > can fold it all into pblk_alloc_rqd/pblk_free_rqd. > It's not easy to fold them there as we use nvm_rq allocations without extra space in the rqd for metadata. This is also a problem for rqd allocated in the stack. But I can change the names to make the functionality more clear. Javier
Attachment:
signature.asc
Description: Message signed with OpenPGP