[U-Boot] [PATCH 1/2] fs: fat: dynamically allocate memory for temporary buffer

[Chee, Tien Fong]

On Fri, 2019-01-18 at 07:26 +0100, Marek Vasut wrote:
> On 1/17/19 7:52 AM, tien.fong.chee at intel.com wrote:
> > 
> > From: Stefan Agner <stefan.ag... at toradex.com>
> > 
> > Drop the statically allocated get_contents_vfatname_block and
> > dynamically allocate a buffer only if required. This saves
> > 64KiB of memory.
> > 
> > Signed-off-by: Stefan Agner <stefan.ag... at toradex.com>
> > Signed-off-by: Tien Fong Chee <tien.fong.chee at intel.com>
> > ---
> >  fs/fat/fat.c |   19 +++++++++++++------
> >  1 files changed, 13 insertions(+), 6 deletions(-)
> > 
> > diff --git a/fs/fat/fat.c b/fs/fat/fat.c
> > index 8803fb4..aa4636d 100644
> > --- a/fs/fat/fat.c
> > +++ b/fs/fat/fat.c
> > @@ -307,9 +307,6 @@ get_cluster(fsdata *mydata, __u32 clustnum,
> > __u8 *buffer, unsigned long size)
> >   * into 'buffer'.
> >   * Update the number of bytes read in *gotsize or return -1 on
> > fatal errors.
> >   */
> > -__u8 get_contents_vfatname_block[MAX_CLUSTSIZE]
> > -	__aligned(ARCH_DMA_MINALIGN);
> > -
> >  static int get_contents(fsdata *mydata, dir_entry *dentptr, loff_t
> > pos,
> >  			__u8 *buffer, loff_t maxsize, loff_t
> > *gotsize)
> >  {
> > @@ -320,7 +317,7 @@ static int get_contents(fsdata *mydata,
> > dir_entry *dentptr, loff_t pos,
> >  	loff_t actsize;
> >  
> >  	*gotsize = 0;
> > -	debug("Filesize: %llu bytes\n", filesize);
> > +	debug("Filesize: %llu bytes, starting at pos %llu\n",
> > filesize, pos);
> This looks like a separate change.
Okay.
> 
> > 
> >  	if (pos >= filesize) {
> >  		debug("Read position past EOF: %llu\n", pos);
> > @@ -352,15 +349,25 @@ static int get_contents(fsdata *mydata,
> > dir_entry *dentptr, loff_t pos,
> >  
> >  	/* align to beginning of next cluster if any */
> >  	if (pos) {
> > +		__u8 *tmp_buffer;
> > +
> > +		tmp_buffer = malloc_cache_aligned(MAX_CLUSTSIZE);
> Don't you know the cluster size by now ?
Yeah, i think so, we can run allocate memory based on actsize. Please
correct me if i'm wrong.
> 
> > 
> > +		if (!tmp_buffer) {
> > +			debug("Error: allocating buffer\n");
> > +			return -ENOMEM;
> > +		}
> > +
> >  		actsize = min(filesize, (loff_t)bytesperclust);
> > -		if (get_cluster(mydata, curclust,
> > get_contents_vfatname_block,
> > +		if (get_cluster(mydata, curclust, tmp_buffer,
> >  				(int)actsize) != 0) {
> >  			printf("Error reading cluster\n");
> > +			free(tmp_buffer);
> >  			return -1;
> >  		}
> >  		filesize -= actsize;
> >  		actsize -= pos;
> > -		memcpy(buffer, get_contents_vfatname_block + pos,
> > actsize);
> > +		memcpy(buffer, tmp_buffer + pos, actsize);
> > +		free(tmp_buffer);
> How many times is this malloc()/free() called ? I wonder how much
> this
> slows things down and how much it fragments the heap. Maybe the
> amount
> of calls to this can be reduced somehow.
There is performance penalty for when reading file based on offset
chunk by chunk at small memory such as OCRAM. However, i believe this
doesn't impact in most use cases because most of them running in large
DDR size. Most of the time, the reading of the file is starting from
offset zero(no memory allocation is required).

I can "feel" that is not much difference actually in performance based
on print out responding for my chunk by chunk file reading use case in
these changes. But these changes with [patch 2/2] would help to
maximize reusable from memory pool, which lead to only 1 block of max
cluster is required allocated in memory pool instead of two blocks.

What do you think?
> 
> > 
> >  		*gotsize += actsize;
> >  		if (!filesize)
> >  			return 0;
> > 
>