RE: [PATCH v4 19/43] hpsa: add ioaccel sg chaining for the ioaccel2 path

[brace <brace@pmcs.com>]

The last I read about mempools was from:
Linux Device Drivers
 By Jonathan Corbet, Alessandro Rubini, Greg Kroah-Hartman

https://lwn.net/images/pdf/LDD3/ch08.pdf


If  you  are  considering  using  a  mempool  in  your  driver,  please  keep  one  thing  in
mind: mempools allocate a chunk of memory that sits in a list, idle and unavailable
for any real use. It is easy to consume a great deal of memory with mempools. In
almost every case, the preferred alternative is to do without the mempool and simply
deal with the possibility of allocation failures instead. If there is any way for your
driver to respond to an allocation failure in a way that does not endanger the integ-
rity of the system, do things that way. Use of mempools in driver code should be
rare

But this book is now quite old in Linux terms.

So, I can investigate using mempools. However, I would prefer to continue using the existing methods in this current patchset.

I hope that is ok with you...


> -----Original Message-----
> From: Hannes Reinecke [mailto:hare@suse.de]
> Sent: Friday, April 17, 2015 8:14 AM
> To: Don Brace; Scott Teel; Kevin Barnett; james.bottomley@parallels.com;
> hch@infradead.org; Justin Lindley; brace
> Cc: linux-scsi@vger.kernel.org
> Subject: Re: [PATCH v4 19/43] hpsa: add ioaccel sg chaining for the ioaccel2
> path
> 
> On 04/16/2015 03:48 PM, Don Brace wrote:
> > From: Webb Scales <webbnh@hp.com>
> >
> > Increase the request size for ioaccel2 path.
> >
> > The error, if any, returned by hpsa_allocate_ioaccel2_sg_chain_blocks
> > to hpsa_alloc_ioaccel2_cmd_and_bft should be returned upstream rather
> > than assumed to be -ENOMEM.
> >
> > This differs slightly from hpsa_alloc_ioaccel1_cmd_and_bft,
> > which does not call another hpsa_allocate function and only
> > has -ENOMEM to return from some kmalloc calls.
> >
> > Reviewed-by: Scott Teel <scott.teel@pmcs.com>
> > Reviewed-by: Kevin Barnett <kevin.barnett@pmcs.com>
> > Signed-off-by: Robert Elliott <elliott@hp.com>
> > Signed-off-by: Don Brace <don.brace@pmcs.com>
> > ---
> >  drivers/scsi/hpsa.c |  125
> +++++++++++++++++++++++++++++++++++++++++++++++----
> >  drivers/scsi/hpsa.h |    1
> >  2 files changed, 116 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
> > index c9c42e9..1839761 100644
> > --- a/drivers/scsi/hpsa.c
> > +++ b/drivers/scsi/hpsa.c
> > @@ -1704,6 +1704,46 @@ static void hpsa_slave_destroy(struct scsi_device
> *sdev)
> >  	/* nothing to do. */
> >  }
> >
> > +static void hpsa_free_ioaccel2_sg_chain_blocks(struct ctlr_info *h)
> > +{
> > +	int i;
> > +
> > +	if (!h->ioaccel2_cmd_sg_list)
> > +		return;
> > +	for (i = 0; i < h->nr_cmds; i++) {
> > +		kfree(h->ioaccel2_cmd_sg_list[i]);
> > +		h->ioaccel2_cmd_sg_list[i] = NULL;
> > +	}
> > +	kfree(h->ioaccel2_cmd_sg_list);
> > +	h->ioaccel2_cmd_sg_list = NULL;
> > +}
> > +
> > +static int hpsa_allocate_ioaccel2_sg_chain_blocks(struct ctlr_info *h)
> > +{
> > +	int i;
> > +
> > +	if (h->chainsize <= 0)
> > +		return 0;
> > +
> > +	h->ioaccel2_cmd_sg_list =
> > +		kzalloc(sizeof(*h->ioaccel2_cmd_sg_list) * h->nr_cmds,
> > +					GFP_KERNEL);
> > +	if (!h->ioaccel2_cmd_sg_list)
> > +		return -ENOMEM;
> > +	for (i = 0; i < h->nr_cmds; i++) {
> > +		h->ioaccel2_cmd_sg_list[i] =
> > +			kmalloc(sizeof(*h->ioaccel2_cmd_sg_list[i]) *
> > +					h->maxsgentries, GFP_KERNEL);
> > +		if (!h->ioaccel2_cmd_sg_list[i])
> > +			goto clean;
> > +	}
> > +	return 0;
> > +
> > +clean:
> > +	hpsa_free_ioaccel2_sg_chain_blocks(h);
> > +	return -ENOMEM;
> > +}
> > +
> >  static void hpsa_free_sg_chain_blocks(struct ctlr_info *h)
> >  {
> >  	int i;
> Any reason why you didn't use mempools here?
> 
> Cheers,
> 
> Hannes
> --
> Dr. Hannes Reinecke		               zSeries & Storage
> hare@suse.de			               +49 911 74053 688
> SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
> GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
> HRB 21284 (AG Nürnberg)