RE: [PATCH 01/10] aacraid: SCSI blk tag support

[Raghava Aditya Renukunta <RaghavaAditya.Renukunta@pmcs.com>]

Hello Tomas,

> -----Original Message-----
> From: Tomas Henzl [mailto:thenzl@redhat.com]
> Sent: Thursday, December 3, 2015 7:52 AM
> To: Raghava Aditya Renukunta; JBottomley@Parallels.com; linux-
> scsi@vger.kernel.org
> Cc: Mahesh Rajashekhara; Murthy Bhat; Santosh Akula; Gana Sridaran;
> aacraid@pmc-sierra.com; Rich Bono
> Subject: Re: [PATCH 01/10] aacraid: SCSI blk tag support
> 
> On 1.12.2015 13:39, Raghava Aditya Renukunta wrote:
> > From: Raghava Aditya Renukunta <raghavaaditya.renukunta@pmcs.com>
> >
> > The method to allocate and free FIB's in the present code utilizes
> > spinlocks.Multiple IO's have to wait on the spinlock to acquire or
> > free fibs creating a performance bottleneck.
> >
> > An alternative solution would be to use block layer tags to keep track
> > of the fibs allocated and freed. To this end 2 functions
> > aac_fib_alloc_tag and aac_fib_free_tag were created which utilize the
> > blk layer tags to plug into the Fib pool.These functions are used
> > exclusively in the IO path. 8 fibs are reserved for the use of AIF
> > management software and utilize the previous spinlock based
> > implementations.
> >


[....]

> >
> > @@ -166,6 +166,49 @@ int aac_fib_setup(struct aac_dev * dev)
> >   *	aac_fib_alloc	-	allocate a fib
> >   *	@dev: Adapter to allocate the fib for
> >   *
> > + *	Allocate a fib from the adapter fib pool using tags
> > + *	from the blk layer.
> > + */
> > +
> > +struct fib *aac_fib_alloc_tag(struct aac_dev *dev, struct scsi_cmnd
> *scmd)
> > +{
> > +	struct fib *fibptr;
> > +
> > +	fibptr = &dev->fibs[scmd->request->tag];
> > +	/*
> > +	 *	Set the proper node type code and node byte size
> > +	 */
> 
> Can't you just make most of the initialisation below just once when the driver
> starts?
> 
> > +	fibptr->type = FSAFS_NTC_FIB_CONTEXT;
> > +	fibptr->size = sizeof(struct fib);
> > +	/*
> > +	 *	Null out fields that depend on being zero at the start of
> > +	 *	each I/O
> > +	 */
> > +	fibptr->hw_fib_va->header.XferState = 0;
> > +	fibptr->flags = 0;
> 
> For example the flags field is initialised  here to '0', then
> in aac_fib_send again to zero and later is 'fibptr->flags =
> FIB_CONTEXT_FLAG;'
> Removing part of it would help I think.
> The code here is not new so, if I am right, please plan it for a next series.

I will look into it and see where the code is being duplicated and will remove it,
If it does not do anything new.

> 
> > +	fibptr->callback = NULL;
> > +	fibptr->callback_data = NULL;
> > +
> > +	return fibptr;
> > +}
> > +
> > +/**
> > + *	aac_fib_free_tag	free a fib
> > + *	@fibptr: fib to free up
> > + *
> > + *	Placeholder to free tag allocated fibs
> > + *	Does not do anything
> > + */
> > +
> > +void aac_fib_free_tag(struct fib *fibptr)
> > +{
> > +	(void)fibptr;
> > +}
> 
> I agree with Johannes, I also don't like the aac_fib_free_tag as it is.
> With the aac_fib_free_tag you may add -

The reason it is still there is that , future versions can use it to add 
something meaningful(set flags etc).I could remove it now since 
it serves no purpose

> Reviewed-by: Tomas Henzl <thenzl@redhat.com>

Regards,
Raghava Aditya