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

["Matias Bjørling" <mb@lightnvm.io>]

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.

> 
>>> +
>>> +	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.

> 
> Javier
>