Re: [PATCH 1/4] raid5-ppl: use a single mempool for ppl_io_unit and header_page

[Artur Paszkiewicz <artur.paszkiewicz@intel.com>]

On 04/11/2017 10:28 AM, Artur Paszkiewicz wrote:
> On 04/10/2017 09:09 PM, Shaohua Li wrote:
>>> +static void *ppl_io_pool_alloc(gfp_t gfp_mask, void *pool_data)
>>> +{
>>> +	struct kmem_cache *kc = pool_data;
>>> +	struct ppl_io_unit *io;
>>> +
>>> +	io = kmem_cache_alloc(kc, gfp_mask);
>>> +	if (!io)
>>> +		return NULL;
>>> +
>>> +	io->header_page = alloc_page(gfp_mask);
>>> +	if (!io->header_page) {
>>> +		kmem_cache_free(kc, io);
>>> +		return NULL;
>>> +	}
>>> +
>>> +	return io;
>>
>> Maybe directly use GFP_NOWAIT here, we don't use other gfp
> 
> Do you mean ignore the gfp_mask parameter? I don't think we should do
> that. It is provided by the mempool implementation. We only use
> GFP_NOWAIT for explicit allocations, but GFP_KERNEL is used for
> allocating initial items in mempool_create().
> 
>>> +}
>>> +
>>> +static void ppl_io_pool_free(void *element, void *pool_data)
>>> +{
>>> +	struct kmem_cache *kc = pool_data;
>>> +	struct ppl_io_unit *io = element;
>>> +
>>> +	__free_page(io->header_page);
>>> +	kmem_cache_free(kc, io);
>>> +}
>>> +
>>>  static struct ppl_io_unit *ppl_new_iounit(struct ppl_log *log,
>>>  					  struct stripe_head *sh)
>>>  {
>>> @@ -202,18 +228,19 @@ static struct ppl_io_unit *ppl_new_iounit(struct ppl_log *log,
>>>  	struct ppl_io_unit *io;
>>>  	struct ppl_header *pplhdr;
>>>  
>>> -	io = mempool_alloc(ppl_conf->io_pool, GFP_ATOMIC);
>>> +	io = mempool_alloc(ppl_conf->io_pool, GFP_NOWAIT);
>>>  	if (!io)
>>>  		return NULL;
>>>  
>>> -	memset(io, 0, sizeof(*io));
>>>  	io->log = log;
>>> +	io->entries_count = 0;
>>> +	io->pp_size = 0;
>>> +	io->submitted = false;
>>
>> I'd suggest moving the memset to ppl_io_pool_alloc. Don't think we need to
>> optimize to avoid setting header_page. And doing memset is less error prone,
>> for example adding new fields.
> 
> OK, I'll change this and resend.

Well, on second thought, actually I can't move the memset to
ppl_io_pool_alloc. Mempool can use the preallocated elements, which are
then returned back to the pool after calling mempool_free() and can be
reused later. So a ppl_io_unit must be initialized after retrieving it
from mempool_alloc(). I can leave the memset in ppl_new_iounit() but
io->header_page pointer will have to be temporarily copied before
zeroing the iounit and then set back. Is this ok?

Thanks,
Artur