[U-Boot] [PATCH v2] ext2: Cache line aligned partial sector bounce buffer

[Anton Staaf]

On Tue, Aug 23, 2011 at 11:32 AM, Mike Frysinger <vapier at gentoo.org> wrote:
> On Tuesday, August 23, 2011 13:58:01 Anton Staaf wrote:
>> On Tue, Aug 23, 2011 at 10:23 AM, Mike Frysinger wrote:
>> > On Monday, August 22, 2011 17:48:47 Anton Staaf wrote:
>> >> On Mon, Aug 22, 2011 at 2:42 PM, Wolfgang Denk wrote:
>> >> > Anton Staaf wrote:
>> >> >> Currently, if a device read request is done that does not begin or
>> >> >> end on a sector boundary a stack allocated bounce buffer is used to
>> >> >> perform the read, and then just the part of the sector that is
>> >> >> needed is copied into the users buffer.  This stack allocation can
>> >> >> mean that the bounce buffer will not be aligned to the dcache line
>> >> >> size.  This is a problem when caches are enabled because unaligned
>> >> >> cache invalidates are not safe.
>> >> >>
>> >> >> This patch allocates a cache line size aligned sector sized bounce
>> >> >> buffer the first time that ext2fs_devread is called.
>> >> >
>> >> > ...and never frees ist, which is a bad thing.  Please fix.
>> >>
>> >> That was actually intentional.  To free the buffer the code would need
>> >> to know when it was done doing ext2 accesses.  This information isn't
>> >> really available.  And it would be a performance hit to allocate and
>> >> free the buffer every time a read was performed, instead this patch
>> >> re-uses the same allocated buffer every time that the read is called.
>> >> Would you prefer that I allocate and free the buffer each time?  I can
>> >> see an argument for that since it would mean that the code could also
>> >> be called from multiple threads simultaneously, not that we have any
>> >> such thing to worry about at the moment.
>> >
>> > i'm not sure i follow ... the current code always frees it upon func
>> > exit. why cant yours do the same ?
>>
>> I certainly could.  But as I mentioned it would be a performance hit
>> to do so.  The devread function is called many times.  And there is no
>> way of knowing when the last one finishes.  And since it's likely that
>> a kernel will be loaded shortly it seems better to be fast than to
>> free this buffer.  But I would be happy to change this if people
>> disagree (which it sounds like they do).  An alternative would be to
>> allocate the buffer the first time it is needed in the devread
>> function and then free it in the ext2fs_close function.  Or if we know
>> that ext2fs_mount will always be called first we could allocate the
>> buffer there.
>
> and what do you do when there is no memory left in the malloc arena because
> you leaked it all and so can't service any new read requests ?

I think you've miss-understood my patch.  The allocated buffer is
stored in a static variable.  So only the first time through is it
allocated.

> if the malloc performance is poor, then why not fix that ?
> -mike
>

I think our other thread has provided the correct solution here.

Thanks,
    Anton