Re: [U-Boot] [PATCH v2] ext2: Cache line aligned partial sector bounce buffer
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 ? -mike signature.asc Description: This is a digitally signed message part. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2] ext2: Cache line aligned partial sector bounce buffer
On Tue, Aug 23, 2011 at 10:23 AM, Mike Frysinger vap...@gentoo.org 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. Thanks, Anton -mike ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2] ext2: Cache line aligned partial sector bounce buffer
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 ? if the malloc performance is poor, then why not fix that ? -mike signature.asc Description: This is a digitally signed message part. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2] ext2: Cache line aligned partial sector bounce buffer
Dear Anton Staaf, In message CAF6FioXyMnhK=gkbwdegpvggz-xms-hr6c4q3ssttj29crs...@mail.gmail.com you 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. Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, MD: Wolfgang Denk Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de No question is too silly to ask. Of course, some questions are too silly to to answer... - L. Wall R. L. Schwartz, _Programming Perl_ ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2] ext2: Cache line aligned partial sector bounce buffer
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); but i'm not sure this is better than the proposed: char *sec_buf = dma_buffer_alloca(SECTOR_SIZE); 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 signature.asc Description: This is a digitally signed message part. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2] ext2: Cache line aligned partial sector bounce buffer
On Tue, Aug 23, 2011 at 11:32 AM, Mike Frysinger vap...@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 ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2] ext2: Cache line aligned partial sector bounce buffer
On Tue, Aug 23, 2011 at 11:47 AM, Mike Frysinger vap...@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 ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2] ext2: Cache line aligned partial sector bounce buffer
On Tuesday, August 23, 2011 14:48:24 Anton Staaf wrote: On Tue, Aug 23, 2011 at 11:32 AM, Mike Frysinger wrote: 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. yes, i missed that. still no excuse for leaking though ! :) -mike signature.asc Description: This is a digitally signed message part. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2] ext2: Cache line aligned partial sector bounce buffer
On Tue, Aug 23, 2011 at 11:55 AM, Mike Frysinger vap...@gentoo.org wrote: On Tuesday, August 23, 2011 14:48:24 Anton Staaf wrote: On Tue, Aug 23, 2011 at 11:32 AM, Mike Frysinger wrote: 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. yes, i missed that. still no excuse for leaking though ! :) Fair enough. I'm happier not leaking as well. I'll resubmit this patch once the aligned alloca thread has come to a consensus. Thanks, Anton -mike ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2] ext2: Cache line aligned partial sector bounce buffer
Dear Mike Frysinger, In message 201108231447.39934.vap...@gentoo.org you wrote: then people would simply do: DMA_DECLARE_BUFFER(char, sec_buf, SECTOR_SIZE); but i'm not sure this is better than the proposed: char *sec_buf = dma_buffer_alloca(SECTOR_SIZE); 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. Even if the code is different the former is so ugly that chances for it to go into mainline are _really_ small. Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, MD: Wolfgang Denk Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de Just because your doctor has a name for your condition doesn't mean he knows what it is. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2] ext2: Cache line aligned partial sector bounce buffer
Dear Anton Staaf, In message caf6fiouorx50_6nekmc0myakf6rzwsbu9tonrc+4c2uwuh+...@mail.gmail.com you wrote: 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. Strictly speaking, only the _pointer_to_ the allocated buffer is stored in a static variable. [I know you meant this, but we should try and be exact.] Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, MD: Wolfgang Denk Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de You have the capacity to learn from mistakes. You'll learn a lot today. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2] ext2: Cache line aligned partial sector bounce buffer
On Tue, Aug 23, 2011 at 1:18 PM, Wolfgang Denk w...@denx.de wrote: Dear Anton Staaf, In message caf6fiouorx50_6nekmc0myakf6rzwsbu9tonrc+4c2uwuh+...@mail.gmail.com you wrote: 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. Strictly speaking, only the _pointer_to_ the allocated buffer is stored in a static variable. [I know you meant this, but we should try and be exact.] Yes, absolutely. Thanks, Anton Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, MD: Wolfgang Denk Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de You have the capacity to learn from mistakes. You'll learn a lot today. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2] ext2: Cache line aligned partial sector bounce buffer
Dear Anton Staaf, In message 1314043924-22130-1-git-send-email-robot...@chromium.org you 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. Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, MD: Wolfgang Denk Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de NOTE: The Most Fundamental Particles in This Product Are Held Together by a Gluing Force About Which Little is Currently Known and Whose Adhesive Power Can Therefore Not Be Permanently Guaranteed. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2] ext2: Cache line aligned partial sector bounce buffer
On Mon, Aug 22, 2011 at 2:42 PM, Wolfgang Denk w...@denx.de wrote: Dear Anton Staaf, In message 1314043924-22130-1-git-send-email-robot...@chromium.org you 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. Thanks, Anton Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, MD: Wolfgang Denk Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de NOTE: The Most Fundamental Particles in This Product Are Held Together by a Gluing Force About Which Little is Currently Known and Whose Adhesive Power Can Therefore Not Be Permanently Guaranteed. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
