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

[Anton Staaf]

On Tue, Aug 23, 2011 at 11:47 AM, Mike Frysinger <vapier at gentoo.org> wrote:
> On Tuesday, August 23, 2011 14:39:48 Wolfgang Denk wrote:
>> Anton Staaf wrote:
>> > >> 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. =A0Please 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
>>
>> As Mike pointed out, this information is of course available: the
>> bufer was on the stack before, so it disappears upon return from this
>> function.
>>
>> > 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
>>
>> Do we really need malloc at all? Why cannot we just use a slightly
>> larger buffer on the stack and align the pointer into it?  We're
>> talking about cache line sizes here, i. e. a few tens of bytes -
>> that is probably way less than the code you add here.
>
> i think we want to make this easy to support and hard to screw up, otherwise
> people are going to screw it up and cause more problems :).
>
> if get_dcache_line_size() were a static inline, and we allow flexible array
> initializers (is that c99?  i forget), we could do:
> #define DMA_ALIGN_SIZE(size) \
>        (((size) + get_dcache_line_size() - 1) & ~(get_dcache_line_size() - 1))
> #define DMA_DECLARE_BUFFER(type, name, size) \
>        void *__##name[DMA_ALIGN_SIZE(size)]; \
>        type name = __##name;
>
> then people would simply do:
>        DMA_DECLARE_BUFFER(char, sec_buf, SECTOR_SIZE);

Wow!  Yes, please let's not do this and let's just use the simpler
aligned alloca that we discussed in the other thread.  Also, the above
macro does not create an aligned buffer, it just creates a buffer that
is a multiple of the cache line size.  It there is nothing that
changes the alignment of the __##name array.

> but i'm not sure this is better than the proposed:
>        char *sec_buf = dma_buffer_alloca(SECTOR_SIZE);

I would much prefer this solution.

Thanks,
    Anton

> i'd have to look at the code gcc generates to see if it is simply the same ...
> in which case i'd stick with the latter as it's more natural to people.
> -mike
>