Re: [PATCH] tools/vm/page_owner_sort.c: count and sort by mem

From: Andrew Morton
Date: Sat Sep 11 2021 - 18:25:30 EST


On Fri, 10 Sep 2021 11:03:43 +0800 Zhenliang Wei <weizhenliang@xxxxxxxxxx> wrote:

> When viewing page owner information, we may be more concerned about the
> total memory than the number of stack occurrences. Therefore, the
> following adjustments are made:
> 1. Added the statistics on the total number of pages.
> 2. Added the optional parameter "-m" to configure the program to sort by
> memory (total pages).
>

Why does it add regexp matching to add_list()? Presumably this is some
enhancement to the user interface which I cannot see documented in the
changelog or the code comments,

Can we please add/maintain a full description of the user interface in,
I guess, Documentation/vm/page_owner.rst?

> @@ -59,12 +65,50 @@ static int compare_num(const void *p1, const void *p2)
> return l2->num - l1->num;
> }
>
> +static int compare_page_num(const void *p1, const void *p2)
> +{
> + const struct block_list *l1 = p1, *l2 = p2;
> +
> + return l2->page_num - l1->page_num;
> +}
> +
> +static int get_page_num(char *buf)
> +{
> + int err, val_len, order_val;
> + char order_str[4] = {0};
> + char *endptr;
> + regmatch_t pmatch[2];
> +
> + err = regexec(&order_pattern, buf, 2, pmatch, REG_NOTBOL);
> + if (err != 0 || pmatch[1].rm_so == -1) {
> + printf("no order pattern in %s\n", buf);

Shouldn't error messages normally be directed to stderr? We aren't
very consistent about this but it was the accepted thing to do 20-30
years ago, lol.

> + return 0;
> + }
> + val_len = pmatch[1].rm_eo - pmatch[1].rm_so;
> + if (val_len > 2) /* max_order should not exceed 2 digits */
> + goto wrong_order;
> +
> + memcpy(order_str, buf + pmatch[1].rm_so, val_len);
> +
> + errno = 0;
> + order_val = strtol(order_str, &endptr, 10);
> + if (errno != 0 || endptr == order_str || *endptr != '\0')
> + goto wrong_order;
> +
> + return 1 << order_val;
> +
> +wrong_order:
> + printf("wrong order in follow buf:\n%s\n", buf);
> + return 0;
> +}
> +
> static void add_list(char *buf, int len)
> {
> if (list_size != 0 &&
> len == list[list_size-1].len &&
> memcmp(buf, list[list_size-1].txt, len) == 0) {
> list[list_size-1].num++;
> + list[list_size-1].page_num += get_page_num(buf);
> return;
> }
> if (list_size == max_size) {
> @@ -74,6 +118,7 @@ static void add_list(char *buf, int len)
> list[list_size].txt = malloc(len+1);
> list[list_size].len = len;
> list[list_size].num = 1;
> + list[list_size].page_num = get_page_num(buf);
> memcpy(list[list_size].txt, buf, len);
> list[list_size].txt[len] = 0;
> list_size++;
> @@ -85,6 +130,13 @@ static void add_list(char *buf, int len)
>
> #define BUF_SIZE (128 * 1024)
>
> +static void usage(void)
> +{
> + printf("Usage: ./page_owner_sort [-m] <input> <output>\n"
> + "-m Sort by total memory. If not set this option, sort by times\n"

s/If not set this option/If this option is unset/