Re: [PATCH v2 2/2] hexdump: Allow skipping identical lines

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Fri, Jan 10, 2025 at 07:42:05PM +0100, Miquel Raynal wrote:
> When dumping long buffers (especially for debug purposes) it may be very
> convenient to sometimes avoid spitting all the lines of the buffer if
> the lines are identical. Typically on embedded devices, the console
> would be wired to a UART running at 115200 bauds, which makes the dumps
> very (very) slow. In this case, having a flag to avoid printing
> duplicated lines is handy.
> 
> Example of a made up repetitive output:
> 0f 53 63 47 56 55 78 7a aa b7 8c ff ff ff ff ff
> ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
> ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
> ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
> ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
> ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
> ff ff ff ff ff ff ff ff ff ff ff ff 01 2a 39 eb
> 
> Same but with the flag enabled:
> 0f 53 63 47 56 55 78 7a aa b7 8c ff ff ff ff ff
> ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
> *
> ff ff ff ff ff ff ff ff ff ff ff ff 01 2a 39 eb

Still thinking that it's not okay to leave the cases where hex_dump_to_buffer()
is being used for the similar. I would expect that to be modified as well.
As told in v1 thread this can be achieved using a context data, instead of
providing zillion fields, one of which may be a kind of CRC32 checksum that
makes this work without any additional allocation.

But I won't prevent you to go with this if you get a blessing from other
PRINTK/PRINTF maintainers/reviewers.

...

>  #include <linux/types.h>
> +#include <linux/string.h>

Can we keep it ordered (to some extent)? I know that types.h is misplaced here.

>  #include <linux/ctype.h>
>  #include <linux/errno.h>
>  #include <linux/kernel.h>

...

> +		if (flags & DUMP_FLAG_SKIP_IDENTICAL_LINES) {

> +			if (i && !memcmp(ptr + i, ptr + prev_i, linelen)) {
> +				prev_i = i;

Can we rather use a hash function or so instead of memcmp()?

> +				if (same_line)
> +					continue;
> +				same_line = true;
> +				printk("%s*\n", level);
> +				continue;

> +			} else {

Redundant 'else'.

> +				prev_i = i;
> +				same_line = false;
> +			}
> +		}

Something like

	unsigned long hcur, hprev = ~0; // any unrealistic init value
	...
		if (flags & DUMP_FLAG_SKIP_IDENTICAL_LINES) {
			hcur = $HASH($LINE);
			if (hcur == hprev) {
				...
				continue;
			}
			hprev = hcur;
		}


-- 
With Best Regards,
Andy Shevchenko