| Message ID | 33437f45-d3e8-17ec-d5b0-0118d50ff6ec@aimvalley.nl |
|---|---|
| State | Accepted |
| Headers | show |
On 12/06/2016 09:29 AM, Kees Trommel wrote: > David, > > Attached a rebased patch file. > > Kees. Applied to mtd-utils.git. Thanks, David
On 12/06/2016 02:19 PM, David Oberhollenzer wrote: > On 12/06/2016 09:29 AM, Kees Trommel wrote: >> David, >> >> Attached a rebased patch file. >> >> Kees. > Applied to mtd-utils.git. It'd be great to have the patch posted to the list with git send-email , otherwise it's not possible to review it :-( Also, the code doesn't look awesome, see below: + if (skipallffs) + { + for (ii = 0; ii < mtd.min_io_size; ii += sizeof(uint32_t)) + { + if (*(uint32_t*)(writebuf + ii) != 0xffffffff) + break; Is this memcmp()-alike function ? + } + if (ii == mtd.min_io_size) + allffs = 1; + } + if (!allffs) { This could be simply turned to: ret = 0; if (!memcmp(writebuf, writebuf + 1, mtd.min_io_size - 1)) { ret = mtd_write(....); } And then you don't need to introduce any new vars (which have weird names like 'ii' ) either. + /* Write out data */ + ret = mtd_write(mtd_desc, &mtd, fd, mtdoffset / mtd.eb_size, + mtdoffset % mtd.eb_size, + onlyoob ? NULL : writebuf, + onlyoob ? 0 : mtd.min_io_size, + writeoob ? oobbuf : NULL, + writeoob ? mtd.oob_size : 0, + write_mode); + } else { + ret = 0; + }
On 12/07/2016 06:01 AM, Marek Vasut wrote: > + if (skipallffs) > + { > + for (ii = 0; ii < mtd.min_io_size; ii += > sizeof(uint32_t)) > + { > + if (*(uint32_t*)(writebuf + ii) != > 0xffffffff) > + break; > > Is this memcmp()-alike function ? > > + } > + if (ii == mtd.min_io_size) > + allffs = 1; > + } > + if (!allffs) { The point of the patch is not to write a page that is filled with 0xFF bytes. A feature that is turned off by default and can be activated via a command line switch. The code you are trying to replace tries to figure out if an entire block of memory is set to 0xFF. The code path should be conditional, as this feature is turned off unless explicitly requested. > This could be simply turned to: > > ret = 0; > if (!memcmp(writebuf, writebuf + 1, mtd.min_io_size - 1)) { > ret = mtd_write(....); > } This does something completely different.
On 12/07/2016 09:21 AM, David Oberhollenzer wrote: > On 12/07/2016 06:01 AM, Marek Vasut wrote: >> + if (skipallffs) >> + { >> + for (ii = 0; ii < mtd.min_io_size; ii += >> sizeof(uint32_t)) >> + { >> + if (*(uint32_t*)(writebuf + ii) != >> 0xffffffff) >> + break; >> >> Is this memcmp()-alike function ? >> >> + } >> + if (ii == mtd.min_io_size) >> + allffs = 1; >> + } >> + if (!allffs) { > The point of the patch is not to write a page that is filled with 0xFF bytes. > A feature that is turned off by default and can be activated via a command > line switch. That much I figured out > The code you are trying to replace tries to figure out if an entire block of > memory is set to 0xFF. The code path should be conditional, as this feature > is turned off unless explicitly requested. And to do that, we need to open-code it like above ? Looks pretty crappy. >> This could be simply turned to: >> >> ret = 0; >> if (!memcmp(writebuf, writebuf + 1, mtd.min_io_size - 1)) { >> ret = mtd_write(....); >> } > This does something completely different. It checks whether the buffer if full of the same bytes, yes ? Add additional conditions as needed (check whether user provided the arg and whether byte 0 in the buffer is 0xff and you should be done).
On Wed, 7 Dec 2016 15:09:49 +0100 Marek Vasut <marek.vasut@gmail.com> wrote: > On 12/07/2016 09:21 AM, David Oberhollenzer wrote: > > On 12/07/2016 06:01 AM, Marek Vasut wrote: > >> + if (skipallffs) > >> + { > >> + for (ii = 0; ii < mtd.min_io_size; ii += > >> sizeof(uint32_t)) > >> + { > >> + if (*(uint32_t*)(writebuf + ii) != > >> 0xffffffff) > >> + break; > >> > >> Is this memcmp()-alike function ? > >> > >> + } > >> + if (ii == mtd.min_io_size) > >> + allffs = 1; > >> + } > >> + if (!allffs) { > > The point of the patch is not to write a page that is filled with 0xFF bytes. > > A feature that is turned off by default and can be activated via a command > > line switch. > > That much I figured out > > > The code you are trying to replace tries to figure out if an entire block of > > memory is set to 0xFF. The code path should be conditional, as this feature > > is turned off unless explicitly requested. > > And to do that, we need to open-code it like above ? > Looks pretty crappy. I agree with Marek on one point: we're likely to need the 0xff-pattern check in other places, so how about implementing a helper (or several helpers) in libmtd? > > >> This could be simply turned to: > >> > >> ret = 0; > >> if (!memcmp(writebuf, writebuf + 1, mtd.min_io_size - 1)) { > >> ret = mtd_write(....); > >> } Oh, nice trick! It works with and additional writebuf[0] == 0xff test. However I'm concerned about readability (can be addressed with a comment explaining what you're doing), and perfs (memcmp is likely to be optimized for aligned accesses, and you defeat that with your writebuf + 1). Here's a proposal for the pattern comparison API: struct pattern_buf { unsigned int size; void *buf; } const struct pattern_buf *mtd_create_pattern_buf(int size, u8 pattern); void mtd_free_pattern_buf(const struct pattern_buf *pbuf); bool mtd_check_pattern(const struct pattern_buf *pbuf, void *buf, int size); This way you can allocate a pattern buffer of pagesize and re-use it for each new page you want to check. This also solves the unaligned memcmp() problem.
On 12/07/2016 05:07 PM, Boris Brezillon wrote: > On Wed, 7 Dec 2016 15:09:49 +0100 > Marek Vasut <marek.vasut@gmail.com> wrote: > >> On 12/07/2016 09:21 AM, David Oberhollenzer wrote: >>> On 12/07/2016 06:01 AM, Marek Vasut wrote: >>>> + if (skipallffs) >>>> + { >>>> + for (ii = 0; ii < mtd.min_io_size; ii += >>>> sizeof(uint32_t)) >>>> + { >>>> + if (*(uint32_t*)(writebuf + ii) != >>>> 0xffffffff) >>>> + break; >>>> >>>> Is this memcmp()-alike function ? >>>> >>>> + } >>>> + if (ii == mtd.min_io_size) >>>> + allffs = 1; >>>> + } >>>> + if (!allffs) { >>> The point of the patch is not to write a page that is filled with 0xFF bytes. >>> A feature that is turned off by default and can be activated via a command >>> line switch. >> >> That much I figured out >> >>> The code you are trying to replace tries to figure out if an entire block of >>> memory is set to 0xFF. The code path should be conditional, as this feature >>> is turned off unless explicitly requested. >> >> And to do that, we need to open-code it like above ? >> Looks pretty crappy. > > I agree with Marek on one point: we're likely to need the 0xff-pattern > check in other places, so how about implementing a helper (or several > helpers) in libmtd? Helper would be great, yeah. >>>> This could be simply turned to: >>>> >>>> ret = 0; >>>> if (!memcmp(writebuf, writebuf + 1, mtd.min_io_size - 1)) { >>>> ret = mtd_write(....); >>>> } > > Oh, nice trick! It works with and additional writebuf[0] == 0xff test. > However I'm concerned about readability (can be addressed with a comment > explaining what you're doing) Yeah, it's not exactly obvious. You need the writebuf[0] == pattern test and bufsize >= 2 test. >, and perfs (memcmp is likely to be > optimized for aligned accesses, and you defeat that with your writebuf > + 1). That's up to libc optimization, really. memcpy() implementation in glibc and musl at least does a lot of magic to optimize copying regardless of alignment. > Here's a proposal for the pattern comparison API: > > struct pattern_buf { > unsigned int size; > void *buf; > } > > const struct pattern_buf *mtd_create_pattern_buf(int size, u8 pattern); > void mtd_free_pattern_buf(const struct pattern_buf *pbuf); > bool mtd_check_pattern(const struct pattern_buf *pbuf, void *buf, > int size); > > This way you can allocate a pattern buffer of pagesize and re-use it for > each new page you want to check. This also solves the unaligned > memcmp() problem. Here you're filling memory with stuff, which for large buffer will trigger IO to main memory, which is slow. This is unnecessary since you already have such data available to you (it's your own buffer and it's in cache), just use it. And the implementation would be: int mtd_buffer_is_filled_with_byte(void *buf, size_t size, u8 pattern) { int match; if (size == 0) return 0; match = (*buf == pattern); return (size == 1) ? match : !memcmp(buf, buf + 1, size - 1) }
On Wed, 7 Dec 2016 17:31:04 +0100 Marek Vasut <marek.vasut@gmail.com> wrote: > On 12/07/2016 05:07 PM, Boris Brezillon wrote: > > On Wed, 7 Dec 2016 15:09:49 +0100 > > Marek Vasut <marek.vasut@gmail.com> wrote: > > > >> On 12/07/2016 09:21 AM, David Oberhollenzer wrote: > >>> On 12/07/2016 06:01 AM, Marek Vasut wrote: > >>>> + if (skipallffs) > >>>> + { > >>>> + for (ii = 0; ii < mtd.min_io_size; ii += > >>>> sizeof(uint32_t)) > >>>> + { > >>>> + if (*(uint32_t*)(writebuf + ii) != > >>>> 0xffffffff) > >>>> + break; > >>>> > >>>> Is this memcmp()-alike function ? > >>>> > >>>> + } > >>>> + if (ii == mtd.min_io_size) > >>>> + allffs = 1; > >>>> + } > >>>> + if (!allffs) { > >>> The point of the patch is not to write a page that is filled with 0xFF bytes. > >>> A feature that is turned off by default and can be activated via a command > >>> line switch. > >> > >> That much I figured out > >> > >>> The code you are trying to replace tries to figure out if an entire block of > >>> memory is set to 0xFF. The code path should be conditional, as this feature > >>> is turned off unless explicitly requested. > >> > >> And to do that, we need to open-code it like above ? > >> Looks pretty crappy. > > > > I agree with Marek on one point: we're likely to need the 0xff-pattern > > check in other places, so how about implementing a helper (or several > > helpers) in libmtd? > > Helper would be great, yeah. > > >>>> This could be simply turned to: > >>>> > >>>> ret = 0; > >>>> if (!memcmp(writebuf, writebuf + 1, mtd.min_io_size - 1)) { > >>>> ret = mtd_write(....); > >>>> } > > > > Oh, nice trick! It works with and additional writebuf[0] == 0xff test. > > However I'm concerned about readability (can be addressed with a comment > > explaining what you're doing) > > Yeah, it's not exactly obvious. You need the writebuf[0] == pattern test > and bufsize >= 2 test. > > >, and perfs (memcmp is likely to be > > optimized for aligned accesses, and you defeat that with your writebuf > > + 1). > > That's up to libc optimization, really. memcpy() implementation in glibc > and musl at least does a lot of magic to optimize copying regardless of > alignment. > > > Here's a proposal for the pattern comparison API: > > > > struct pattern_buf { > > unsigned int size; > > void *buf; > > } > > > > const struct pattern_buf *mtd_create_pattern_buf(int size, u8 pattern); > > void mtd_free_pattern_buf(const struct pattern_buf *pbuf); > > bool mtd_check_pattern(const struct pattern_buf *pbuf, void *buf, > > int size); > > > > This way you can allocate a pattern buffer of pagesize and re-use it for > > each new page you want to check. This also solves the unaligned > > memcmp() problem. > > Here you're filling memory with stuff, which for large buffer will > trigger IO to main memory, which is slow. This is unnecessary since > you already have such data available to you (it's your own buffer and > it's in cache), just use it. Indeed, I didn't consider caches, and as you said, memcmp is likely to be optimized for the unaligned case, and even if it's not, I'm not sure we really care about perfs here. > > And the implementation would be: > > int mtd_buffer_is_filled_with_byte(void *buf, size_t size, u8 pattern) > { > int match; > > if (size == 0) > return 0; > > match = (*buf == pattern); > > return (size == 1) ? match : !memcmp(buf, buf + 1, size - 1) > } > Looks good to me. Feel free to post a patch ;-).
On 12/07/2016 05:59 PM, Boris Brezillon wrote: > On Wed, 7 Dec 2016 17:31:04 +0100 > Marek Vasut <marek.vasut@gmail.com> wrote: > >> On 12/07/2016 05:07 PM, Boris Brezillon wrote: >>> On Wed, 7 Dec 2016 15:09:49 +0100 >>> Marek Vasut <marek.vasut@gmail.com> wrote: >>> >>>> On 12/07/2016 09:21 AM, David Oberhollenzer wrote: >>>>> On 12/07/2016 06:01 AM, Marek Vasut wrote: >>>>>> + if (skipallffs) >>>>>> + { >>>>>> + for (ii = 0; ii < mtd.min_io_size; ii += >>>>>> sizeof(uint32_t)) >>>>>> + { >>>>>> + if (*(uint32_t*)(writebuf + ii) != >>>>>> 0xffffffff) >>>>>> + break; >>>>>> >>>>>> Is this memcmp()-alike function ? >>>>>> >>>>>> + } >>>>>> + if (ii == mtd.min_io_size) >>>>>> + allffs = 1; >>>>>> + } >>>>>> + if (!allffs) { >>>>> The point of the patch is not to write a page that is filled with 0xFF bytes. >>>>> A feature that is turned off by default and can be activated via a command >>>>> line switch. >>>> >>>> That much I figured out >>>> >>>>> The code you are trying to replace tries to figure out if an entire block of >>>>> memory is set to 0xFF. The code path should be conditional, as this feature >>>>> is turned off unless explicitly requested. >>>> >>>> And to do that, we need to open-code it like above ? >>>> Looks pretty crappy. >>> >>> I agree with Marek on one point: we're likely to need the 0xff-pattern >>> check in other places, so how about implementing a helper (or several >>> helpers) in libmtd? >> >> Helper would be great, yeah. >> >>>>>> This could be simply turned to: >>>>>> >>>>>> ret = 0; >>>>>> if (!memcmp(writebuf, writebuf + 1, mtd.min_io_size - 1)) { >>>>>> ret = mtd_write(....); >>>>>> } >>> >>> Oh, nice trick! It works with and additional writebuf[0] == 0xff test. >>> However I'm concerned about readability (can be addressed with a comment >>> explaining what you're doing) >> >> Yeah, it's not exactly obvious. You need the writebuf[0] == pattern test >> and bufsize >= 2 test. >> >>> , and perfs (memcmp is likely to be >>> optimized for aligned accesses, and you defeat that with your writebuf >>> + 1). >> >> That's up to libc optimization, really. memcpy() implementation in glibc >> and musl at least does a lot of magic to optimize copying regardless of >> alignment. >> >>> Here's a proposal for the pattern comparison API: >>> >>> struct pattern_buf { >>> unsigned int size; >>> void *buf; >>> } >>> >>> const struct pattern_buf *mtd_create_pattern_buf(int size, u8 pattern); >>> void mtd_free_pattern_buf(const struct pattern_buf *pbuf); >>> bool mtd_check_pattern(const struct pattern_buf *pbuf, void *buf, >>> int size); >>> >>> This way you can allocate a pattern buffer of pagesize and re-use it for >>> each new page you want to check. This also solves the unaligned >>> memcmp() problem. >> >> Here you're filling memory with stuff, which for large buffer will >> trigger IO to main memory, which is slow. This is unnecessary since >> you already have such data available to you (it's your own buffer and >> it's in cache), just use it. > > Indeed, I didn't consider caches, and as you said, memcmp is likely to > be optimized for the unaligned case, and even if it's not, I'm not sure > we really care about perfs here. > >> >> And the implementation would be: >> >> int mtd_buffer_is_filled_with_byte(void *buf, size_t size, u8 pattern) >> { >> int match; >> >> if (size == 0) >> return 0; >> >> match = (*buf == pattern); >> >> return (size == 1) ? match : !memcmp(buf, buf + 1, size - 1) >> } >> > > Looks good to me. Feel free to post a patch ;-). Grumble ... done.
From 8bf3ad295aee71fdcee48fe462908503ac82ff95 Mon Sep 17 00:00:00 2001 From: Kees Trommel <ctrommel@linvm302.aimsys.nl> Date: Tue, 6 Dec 2016 09:21:19 +0100 Subject: [PATCH] skip-all-ff-pages-option Signed-off-by: Kees Trommel <ctrommel@linvm302.aimsys.nl> --- nand-utils/nandwrite.c | 41 ++++++++++++++++++++++++++++++++--------- 1 file changed, 32 insertions(+), 9 deletions(-) diff --git a/nand-utils/nandwrite.c b/nand-utils/nandwrite.c index 1c00cdf..b376869 100644 --- a/nand-utils/nandwrite.c +++ b/nand-utils/nandwrite.c @@ -49,6 +49,7 @@ static void display_help(int status) "Writes to the specified MTD device.\n" "\n" " -a, --autoplace Use auto OOB layout\n" +" -k, --skip-all-ffs Skip pages that contain only 0xff bytes\n" " -m, --markbad Mark blocks bad if write fails\n" " -n, --noecc Write without ecc\n" " -N, --noskipbad Write without bad block skipping\n" @@ -92,6 +93,7 @@ static bool onlyoob = false; static bool markbad = false; static bool noecc = false; static bool autoplace = false; +static bool skipallffs = false; static bool noskipbad = false; static bool pad = false; static int blockalign = 1; /* default to using actual block size */ @@ -102,7 +104,7 @@ static void process_options(int argc, char * const argv[]) for (;;) { int option_index = 0; - static const char short_options[] = "hb:mnNoOpqs:aV"; + static const char short_options[] = "hb:mnNoOpqs:akV"; static const struct option long_options[] = { /* Order of these args with val==0 matters; see option_index. */ {"version", no_argument, 0, 'V'}, @@ -119,6 +121,7 @@ static void process_options(int argc, char * const argv[]) {"quiet", no_argument, 0, 'q'}, {"start", required_argument, 0, 's'}, {"autoplace", no_argument, 0, 'a'}, + {"skip-all-ffs", no_argument, 0, 'k'}, {0, 0, 0, 0}, }; @@ -172,6 +175,9 @@ static void process_options(int argc, char * const argv[]) case 'a': autoplace = true; break; + case 'k': + skipallffs = true; + break; case 'h': display_help(EXIT_SUCCESS); break; @@ -230,6 +236,8 @@ static void erase_buffer(void *buffer, size_t size) */ int main(int argc, char * const argv[]) { + int allffs; + int ii; int fd = -1; int ifd = -1; int pagelen; @@ -515,14 +523,29 @@ int main(int argc, char * const argv[]) } } - /* Write out data */ - ret = mtd_write(mtd_desc, &mtd, fd, mtdoffset / mtd.eb_size, - mtdoffset % mtd.eb_size, - onlyoob ? NULL : writebuf, - onlyoob ? 0 : mtd.min_io_size, - writeoob ? oobbuf : NULL, - writeoob ? mtd.oob_size : 0, - write_mode); + allffs = 0; + if (skipallffs) + { + for (ii = 0; ii < mtd.min_io_size; ii += sizeof(uint32_t)) + { + if (*(uint32_t*)(writebuf + ii) != 0xffffffff) + break; + } + if (ii == mtd.min_io_size) + allffs = 1; + } + if (!allffs) { + /* Write out data */ + ret = mtd_write(mtd_desc, &mtd, fd, mtdoffset / mtd.eb_size, + mtdoffset % mtd.eb_size, + onlyoob ? NULL : writebuf, + onlyoob ? 0 : mtd.min_io_size, + writeoob ? oobbuf : NULL, + writeoob ? mtd.oob_size : 0, + write_mode); + } else { + ret = 0; + } if (ret) { long long i; if (errno != EIO) { -- 2.5.5