Re: [PATCH 2/3] lightnvm: encapsule rqd dma allocations

[Javier Gonzalez]

> On 29 Aug 2018, at 15.36, Matias BjÃrling <mb@xxxxxxxxxxx> wrote:
> 
> On 08/29/2018 03:18 PM, Javier Gonzalez wrote:
>>> 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?
> 
> 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?

>>>> +
>>>> +	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.
> 
> Yep, that was what I felt as well. Renaming will be good.

Cool. Will do.

Attachment: signature.asc
Description: Message signed with OpenPGP