[PATCH] tools/mm/page_owner_sort: free per-record allocations

[PATCH] tools/mm/page_owner_sort: free per-record allocations

[chenyichong]

add_list() allocates a command name and text buffer for every page owner record.  The cleanup path only frees the outer list array, leaving both allocations for every retained record behind until process exit.

Records discarded while culling also lose their allocation references when the list is compacted.  Free those records as they are merged, track the compacted list size, and release the remaining per-record allocations on exit.  Also handle allocation failures in get_comm() and unwind the command name if allocating the text buffer fails.

Signed-off-by: chenyichong <chenyichong@uniontech.com>
---
 tools/mm/page_owner_sort.c | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/tools/mm/page_owner_sort.c b/tools/mm/page_owner_sort.c
index e6954909401c..67a7fc6d9de2 100644
--- a/tools/mm/page_owner_sort.c
+++ b/tools/mm/page_owner_sort.c
@@ -372,6 +372,9 @@ static char *get_comm(char *buf)
 {
 	char *comm_str = malloc(TASK_COMM_LEN);
 
+	if (!comm_str)
+		return NULL;
+
 	memset(comm_str, 0, TASK_COMM_LEN);
 
 	search_pattern(&comm_pattern, comm_str, buf);
@@ -386,6 +389,12 @@ static char *get_comm(char *buf)
 	return comm_str;
 }
 
+static void free_block_list(struct block_list *block)
+{
+	free(block->comm);
+	free(block->txt);
+}
+
 static int get_arg_type(const char *arg)
 {
 	if (!strcmp(arg, "pid") || !strcmp(arg, "p"))
@@ -480,9 +489,15 @@ static bool add_list(char *buf, int len, char *ext_buf)
 	list[list_size].pid = get_pid(buf);
 	list[list_size].tgid = get_tgid(buf);
 	list[list_size].comm = get_comm(buf);
+	if (!list[list_size].comm) {
+		fprintf(stderr, "Out of memory\n");
+		return false;
+	}
 	list[list_size].txt = malloc(len+1);
 	if (!list[list_size].txt) {
 		fprintf(stderr, "Out of memory\n");
+		free(list[list_size].comm);
+		list[list_size].comm = NULL;
 		return false;
 	}
 	memcpy(list[list_size].txt, buf, len);
@@ -841,8 +856,10 @@ int main(int argc, char **argv)
 		} else {
 			list[count-1].num += list[i].num;
 			list[count-1].page_num += list[i].page_num;
+			free_block_list(&list[i]);
 		}
 	}
+	list_size = count;
 
 	qsort(list, count, sizeof(list[0]), compare_sort_condition);
 
@@ -876,8 +893,11 @@ int main(int argc, char **argv)
 		free(ext_buf);
 	if (buf)
 		free(buf);
-	if (list)
+	if (list) {
+		for (i = 0; i < list_size; i++)
+			free_block_list(&list[i]);
 		free(list);
+	}
 out_ts:
 	regfree(&ts_nsec_pattern);
 out_comm:
-- 
2.51.0

Re: [PATCH] tools/mm/page_owner_sort: free per-record allocations

[Vishal Moola]

On Wed, Jun 10, 2026 at 10:11:08AM +0800, chenyichong wrote:
> add_list() allocates a command name and text buffer for every page owner record.  The cleanup path only frees the outer list array, leaving both allocations for every retained record behind until process exit.
> 
> Records discarded while culling also lose their allocation references when the list is compacted.  Free those records as they are merged, track the compacted list size, and release the remaining per-record allocations on exit.  Also handle allocation failures in get_comm() and unwind the command name if allocating the text buffer fails.

Please take a look at the process[1]. Aka wrap commit messages at ~75
characters.

Running scripts/checkpatch.pl should catch this for you.

> Signed-off-by: chenyichong <chenyichong@uniontech.com>

Also I hope chenyichong is your name :). I'm not that familiar with
chinese names, and the lowercase one-word makes it easy to mistake as an
alias (we don't allow aliases). I'm used to seeing
"[First Name] [Surname]."

> ---
>  tools/mm/page_owner_sort.c | 22 +++++++++++++++++++++-
>  1 file changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/mm/page_owner_sort.c b/tools/mm/page_owner_sort.c
> index e6954909401c..67a7fc6d9de2 100644
> --- a/tools/mm/page_owner_sort.c
> +++ b/tools/mm/page_owner_sort.c
> @@ -372,6 +372,9 @@ static char *get_comm(char *buf)
>  {
>  	char *comm_str = malloc(TASK_COMM_LEN);
>  
> +	if (!comm_str)
> +		return NULL;
> +
>  	memset(comm_str, 0, TASK_COMM_LEN);
>  
>  	search_pattern(&comm_pattern, comm_str, buf);
> @@ -386,6 +389,12 @@ static char *get_comm(char *buf)
>  	return comm_str;
>  }
>  
> +static void free_block_list(struct block_list *block)
> +{
> +	free(block->comm);
> +	free(block->txt);
> +}
> +
>  static int get_arg_type(const char *arg)
>  {
>  	if (!strcmp(arg, "pid") || !strcmp(arg, "p"))
> @@ -480,9 +489,15 @@ static bool add_list(char *buf, int len, char *ext_buf)
>  	list[list_size].pid = get_pid(buf);
>  	list[list_size].tgid = get_tgid(buf);
>  	list[list_size].comm = get_comm(buf);
> +	if (!list[list_size].comm) {
> +		fprintf(stderr, "Out of memory\n");
> +		return false;
> +	}
>  	list[list_size].txt = malloc(len+1);
>  	if (!list[list_size].txt) {
>  		fprintf(stderr, "Out of memory\n");
> +		free(list[list_size].comm);
> +		list[list_size].comm = NULL;
>  		return false;

Returning false here sends us back to the error handling path in main()
where you end up calling your free_block_list() anyway. So we don't need
this here, right?

>  	}
>  	memcpy(list[list_size].txt, buf, len);
> @@ -841,8 +856,10 @@ int main(int argc, char **argv)
>  		} else {
>  			list[count-1].num += list[i].num;
>  			list[count-1].page_num += list[i].page_num;
> +			free_block_list(&list[i]);
>  		}
>  	}
> +	list_size = count;
>  
>  	qsort(list, count, sizeof(list[0]), compare_sort_condition);
>  
> @@ -876,8 +893,11 @@ int main(int argc, char **argv)
>  		free(ext_buf);
>  	if (buf)
>  		free(buf);
> -	if (list)
> +	if (list) {
> +		for (i = 0; i < list_size; i++)
> +			free_block_list(&list[i]);
>  		free(list);
> +	}
>  out_ts:
>  	regfree(&ts_nsec_pattern);
>  out_comm:

[1] https://docs.kernel.org/process/submitting-patches.html