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

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

[Zhenliang Wei]

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).

Signed-off-by: Zhenliang Wei <weizhenliang@huawei.com>
---
 tools/vm/page_owner_sort.c | 94 +++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 85 insertions(+), 9 deletions(-)

diff --git a/tools/vm/page_owner_sort.c b/tools/vm/page_owner_sort.c
index 0e75f22c9475..30dcc606c493 100644
--- a/tools/vm/page_owner_sort.c
+++ b/tools/vm/page_owner_sort.c
@@ -5,6 +5,8 @@
  * Example use:
  * cat /sys/kernel/debug/page_owner > page_owner_full.txt
  * ./page_owner_sort page_owner_full.txt sorted_page_owner.txt
+ * Or sort by total memory:
+ * ./page_owner_sort -m page_owner_full.txt sorted_page_owner.txt
  *
  * See Documentation/vm/page_owner.rst
 */
@@ -16,14 +18,18 @@
 #include <fcntl.h>
 #include <unistd.h>
 #include <string.h>
+#include <regex.h>
+#include <errno.h>
 
 struct block_list {
 	char *txt;
 	int len;
 	int num;
+	int page_num;
 };
 
-
+static int sort_by_memory;
+static regex_t order_pattern;
 static struct block_list *list;
 static int list_size;
 static int max_size;
@@ -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);
+		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"
+	);
+}
+
 int main(int argc, char **argv)
 {
 	FILE *fin, *fout;
@@ -92,21 +144,39 @@ int main(int argc, char **argv)
 	int ret, i, count;
 	struct block_list *list2;
 	struct stat st;
+	int err;
+	int opt;
 
-	if (argc < 3) {
-		printf("Usage: ./program <input> <output>\n");
-		perror("open: ");
+	while ((opt = getopt(argc, argv, "m")) != -1)
+		switch (opt) {
+		case 'm':
+			sort_by_memory = 1;
+			break;
+		default:
+			usage();
+			exit(1);
+		}
+
+	if (optind >= (argc - 1)) {
+		usage();
 		exit(1);
 	}
 
-	fin = fopen(argv[1], "r");
-	fout = fopen(argv[2], "w");
+	fin = fopen(argv[optind], "r");
+	fout = fopen(argv[optind + 1], "w");
 	if (!fin || !fout) {
-		printf("Usage: ./program <input> <output>\n");
+		usage();
 		perror("open: ");
 		exit(1);
 	}
 
+	err = regcomp(&order_pattern, "order\\s*([0-9]*),", REG_EXTENDED|REG_NEWLINE);
+	if (err != 0 || order_pattern.re_nsub != 1) {
+		printf("%s: Invalid pattern 'order\\s*([0-9]*),' code %d\n",
+			argv[0], err);
+		exit(1);
+	}
+
 	fstat(fileno(fin), &st);
 	max_size = st.st_size / 100; /* hack ... */
 
@@ -145,13 +215,19 @@ int main(int argc, char **argv)
 			list2[count++] = list[i];
 		} else {
 			list2[count-1].num += list[i].num;
+			list2[count-1].page_num += list[i].page_num;
 		}
 	}
 
-	qsort(list2, count, sizeof(list[0]), compare_num);
+	if (sort_by_memory)
+		qsort(list2, count, sizeof(list[0]), compare_page_num);
+	else
+		qsort(list2, count, sizeof(list[0]), compare_num);
 
 	for (i = 0; i < count; i++)
-		fprintf(fout, "%d times:\n%s\n", list2[i].num, list2[i].txt);
+		fprintf(fout, "%d times, %d pages:\n%s\n",
+				list2[i].num, list2[i].page_num, list2[i].txt);
 
+	regfree(&order_pattern);
 	return 0;
 }
-- 
2.12.3

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

[Andrew Morton]

On Fri, 10 Sep 2021 11:03:43 +0800 Zhenliang Wei <weizhenliang@huawei.com> 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/

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

[weizhenliang]

On Fri, 12 Sep 2021 Andrew Morton wrote:

>On Fri, 10 Sep 2021 11:03:43 +0800 Zhenliang Wei <weizhenliang@huawei.com> 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?

Thanks for reviewing, I did omit the documentation part, I will improve git msg and page_owner.rst later.

The general output of page_owner is as follows:

		Page allocated via order XXX, ...
		PFN XXX ...
		 // Detailed stack

		Page allocated via order XXX, ...
		PFN XXX ...
		 // Detailed stack

The original page_owner_sort tool ignores PFN rows, puts the remaining rows in buf, counts the times of buf, and finally sorts them according to the times.
General output:

		XXX times:
		Page allocated via order XXX, ...
		 // Detailed stack

Now, we use regexp to extract the page order value from the buf, and count the total pages for the buf.
General output:

		XXX times, XXX pages:
		Page allocated via order XXX, ...
		 // Detailed stack

By default, it is still sorted by the times of buf; If we want to sort by the pages nums of buf, use the new -m parameter.

>> @@ -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.

On this point, it does not affect the use of the tool. Personally, I prefer to retain the original coding style of the tool. Is this ok? 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/

Okay, thank you, I will adjust the Usage description later

And any other suggestions about the patch?

Wei.