[PATCH] tools/vm/page_owner_sort.c: add switch between culling by stacktrace and txt

[PATCH] tools/vm/page_owner_sort.c: add switch between culling by stacktrace and txt

[Yinan Zhang]

Culling by comparing stacktrace would casue loss of some
information. For example, if there exists 2 blocks which
have the same stacktrace and the different head info

Page allocated via order 0, mask 0x108c48(...), pid 73696,
 ts 1578829190639010 ns, free_ts 1576583851324450 ns
 prep_new_page+0x80/0xb8
 get_page_from_freelist+0x924/0xee8
 __alloc_pages+0x138/0xc18
 alloc_pages+0x80/0xf0
 __page_cache_alloc+0x90/0xc8

Page allocated via order 0, mask 0x108c48(...), pid 61806,
 ts 1354113726046100 ns, free_ts 1354104926841400 ns
 prep_new_page+0x80/0xb8
 get_page_from_freelist+0x924/0xee8
 __alloc_pages+0x138/0xc18
 alloc_pages+0x80/0xf0
 __page_cache_alloc+0x90/0xc8

After culling, it would be like this

2 times, 2 pages:
Page allocated via order 0, mask 0x108c48(...), pid 73696,
 ts 1578829190639010 ns, free_ts 1576583851324450 ns
 prep_new_page+0x80/0xb8
 get_page_from_freelist+0x924/0xee8
 __alloc_pages+0x138/0xc18
 alloc_pages+0x80/0xf0
 __page_cache_alloc+0x90/0xc8

The info of second block missed. So, add -c to turn on culling
by stacktrace. By default, it will cull by txt.

Signed-off-by: Yinan Zhang <zhangyinan2019@email.szu.edu.cn>
---
 tools/vm/page_owner_sort.c | 23 ++++++++++++++++++++---
 1 file changed, 20 insertions(+), 3 deletions(-)

diff --git a/tools/vm/page_owner_sort.c b/tools/vm/page_owner_sort.c
index 1b2acf02d3cd..492be7f752c0 100644
--- a/tools/vm/page_owner_sort.c
+++ b/tools/vm/page_owner_sort.c
@@ -51,6 +51,13 @@ int read_block(char *buf, int buf_size, FILE *fin)
 	return -1; /* EOF or no space left in buf. */
 }
 
+static int compare_txt(const void *p1, const void *p2)
+{
+	const struct block_list *l1 = p1, *l2 = p2;
+
+	return strcmp(l1->txt, l2->txt);
+}
+
 static int compare_stacktrace(const void *p1, const void *p2)
 {
 	const struct block_list *l1 = p1, *l2 = p2;
@@ -137,12 +144,14 @@ static void usage(void)
 		"-m	Sort by total memory.\n"
 		"-s	Sort by the stack trace.\n"
 		"-t	Sort by times (default).\n"
+		"-c	cull by comparing stacktrace instead of total block.\n"
 	);
 }
 
 int main(int argc, char **argv)
 {
 	int (*cmp)(const void *, const void *) = compare_num;
+	int cull_st = 0;
 	FILE *fin, *fout;
 	char *buf;
 	int ret, i, count;
@@ -151,7 +160,7 @@ int main(int argc, char **argv)
 	int err;
 	int opt;
 
-	while ((opt = getopt(argc, argv, "mst")) != -1)
+	while ((opt = getopt(argc, argv, "mstc")) != -1)
 		switch (opt) {
 		case 'm':
 			cmp = compare_page_num;
@@ -162,6 +171,9 @@ int main(int argc, char **argv)
 		case 't':
 			cmp = compare_num;
 			break;
+		case 'c':
+			cull_st = 1;
+			break;
 		default:
 			usage();
 			exit(1);
@@ -209,7 +221,10 @@ int main(int argc, char **argv)
 
 	printf("sorting ....\n");
 
-	qsort(list, list_size, sizeof(list[0]), compare_stacktrace);
+	if (cull_st == 1)
+		qsort(list, list_size, sizeof(list[0]), compare_stacktrace);
+	else
+		qsort(list, list_size, sizeof(list[0]), compare_txt);
 
 	list2 = malloc(sizeof(*list) * list_size);
 	if (!list2) {
@@ -219,9 +234,11 @@ int main(int argc, char **argv)
 
 	printf("culling\n");
 
+	long offset = cull_st ? &list[0].stacktrace - &list[0].txt : 0;
+
 	for (i = count = 0; i < list_size; i++) {
 		if (count == 0 ||
-		    strcmp(list2[count-1].stacktrace, list[i].stacktrace) != 0) {
+		    strcmp(*(&list2[count-1].txt+offset), *(&list[i].txt+offset)) != 0) {
 			list2[count++] = list[i];
 		} else {
 			list2[count-1].num += list[i].num;
-- 
2.23.0

Re: [PATCH] tools/vm/page_owner_sort.c: add switch between culling by stacktrace and txt

[Sean Anderson]

On 11/29/21 9:56 AM, Yinan Zhang wrote:
> Culling by comparing stacktrace would casue loss of some
> information. For example, if there exists 2 blocks which
> have the same stacktrace and the different head info
> 
> Page allocated via order 0, mask 0x108c48(...), pid 73696,
>   ts 1578829190639010 ns, free_ts 1576583851324450 ns
>   prep_new_page+0x80/0xb8
>   get_page_from_freelist+0x924/0xee8
>   __alloc_pages+0x138/0xc18
>   alloc_pages+0x80/0xf0
>   __page_cache_alloc+0x90/0xc8
> 
> Page allocated via order 0, mask 0x108c48(...), pid 61806,
>   ts 1354113726046100 ns, free_ts 1354104926841400 ns
>   prep_new_page+0x80/0xb8
>   get_page_from_freelist+0x924/0xee8
>   __alloc_pages+0x138/0xc18
>   alloc_pages+0x80/0xf0
>   __page_cache_alloc+0x90/0xc8
> 
> After culling, it would be like this
> 
> 2 times, 2 pages:
> Page allocated via order 0, mask 0x108c48(...), pid 73696,
>   ts 1578829190639010 ns, free_ts 1576583851324450 ns
>   prep_new_page+0x80/0xb8
>   get_page_from_freelist+0x924/0xee8
>   __alloc_pages+0x138/0xc18
>   alloc_pages+0x80/0xf0
>   __page_cache_alloc+0x90/0xc8
> 

This is working as designed. IMO there's no point in separating
allocations like this which differ only in PID and timestamp, since you
will get no grouping at all.

> The info of second block missed. So, add -c to turn on culling
> by stacktrace. By default, it will cull by txt.

Please keep the default to actually do something in the cull step.

> Signed-off-by: Yinan Zhang <zhangyinan2019@email.szu.edu.cn>
> ---
>   tools/vm/page_owner_sort.c | 23 ++++++++++++++++++++---
>   1 file changed, 20 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/vm/page_owner_sort.c b/tools/vm/page_owner_sort.c
> index 1b2acf02d3cd..492be7f752c0 100644
> --- a/tools/vm/page_owner_sort.c
> +++ b/tools/vm/page_owner_sort.c
> @@ -51,6 +51,13 @@ int read_block(char *buf, int buf_size, FILE *fin)
>   	return -1; /* EOF or no space left in buf. */
>   }
>   
> +static int compare_txt(const void *p1, const void *p2)
> +{
> +	const struct block_list *l1 = p1, *l2 = p2;
> +
> +	return strcmp(l1->txt, l2->txt);
> +}
> +
>   static int compare_stacktrace(const void *p1, const void *p2)
>   {
>   	const struct block_list *l1 = p1, *l2 = p2;
> @@ -137,12 +144,14 @@ static void usage(void)
>   		"-m	Sort by total memory.\n"
>   		"-s	Sort by the stack trace.\n"
>   		"-t	Sort by times (default).\n"
> +		"-c	cull by comparing stacktrace instead of total block.\n"
>   	);
>   }
>   
>   int main(int argc, char **argv)
>   {
>   	int (*cmp)(const void *, const void *) = compare_num;
> +	int cull_st = 0;
>   	FILE *fin, *fout;
>   	char *buf;
>   	int ret, i, count;
> @@ -151,7 +160,7 @@ int main(int argc, char **argv)
>   	int err;
>   	int opt;
>   
> -	while ((opt = getopt(argc, argv, "mst")) != -1)
> +	while ((opt = getopt(argc, argv, "mstc")) != -1)
>   		switch (opt) {
>   		case 'm':
>   			cmp = compare_page_num;
> @@ -162,6 +171,9 @@ int main(int argc, char **argv)
>   		case 't':
>   			cmp = compare_num;
>   			break;
> +		case 'c':
> +			cull_st = 1;
> +			break;

Can we set a "cull_cmp" variable like cmp?

Looking forward, I think something like

	page_owner_sort --cull=stacktrace --sort=times foo bar

would be nice.

--Sean

>   		default:
>   			usage();
>   			exit(1);
> @@ -209,7 +221,10 @@ int main(int argc, char **argv)
>   
>   	printf("sorting ....\n");
>   
> -	qsort(list, list_size, sizeof(list[0]), compare_stacktrace);
> +	if (cull_st == 1)
> +		qsort(list, list_size, sizeof(list[0]), compare_stacktrace);
> +	else
> +		qsort(list, list_size, sizeof(list[0]), compare_txt);
>   
>   	list2 = malloc(sizeof(*list) * list_size);
>   	if (!list2) {
> @@ -219,9 +234,11 @@ int main(int argc, char **argv)
>   
>   	printf("culling\n");
>   
> +	long offset = cull_st ? &list[0].stacktrace - &list[0].txt : 0;
> +
>   	for (i = count = 0; i < list_size; i++) {
>   		if (count == 0 ||
> -		    strcmp(list2[count-1].stacktrace, list[i].stacktrace) != 0) {
> +		    strcmp(*(&list2[count-1].txt+offset), *(&list[i].txt+offset)) != 0) {
>   			list2[count++] = list[i];
>   		} else {
>   			list2[count-1].num += list[i].num;
> 

Re: [PATCH] tools/vm/page_owner_sort.c: add switch between culling by stacktrace and txt

[Andrew Morton]

These comments were not responded to:

On Mon, 29 Nov 2021 21:23:41 -0500 Sean Anderson <seanga2@gmail.com> wrote:
> 
> This is working as designed. IMO there's no point in separating
> allocations like this which differ only in PID and timestamp, since you
> will get no grouping at all.
> 
> > The info of second block missed. So, add -c to turn on culling
> > by stacktrace. By default, it will cull by txt.
> 
> Please keep the default to actually do something in the cull step.
> 
> ...
>
> > @@ -162,6 +171,9 @@ int main(int argc, char **argv)
> >   		case 't':
> >   			cmp = compare_num;
> >   			break;
> > +		case 'c':
> > +			cull_st = 1;
> > +			break;
> 
> Can we set a "cull_cmp" variable like cmp?
> 
> Looking forward, I think something like
> 
> 	page_owner_sort --cull=stacktrace --sort=times foo bar
> 
> would be nice.
> 

Which is unfortunate.   

I'll send the patch in to Linus anyway, as many other patches
syntactically depend on it.  Please work with Sean to address these
issues and lets get this resolved over the next few weeks.

Also, please cc linux-mm@kvack.org on changes to page_owner.

Re: [PATCH] tools/vm/page_owner_sort.c: add switch between culling by stacktrace and txt

[Sean Anderson]

On 3/21/22 10:22 PM, Yinan Zhang wrote:
> I replied to the email a few months ago. Did you receive it?

The patch was applied anyway. Anything in this subsystem gets
applied within a day or two regardless of feedback. Personally,
I'm not motivated to review anything because of that.

--Sean

> 
> on 2022/3/22 4:38, Andrew Morton wrote:
> 
>> These comments were not responded to:
>>
>> On Mon, 29 Nov 2021 21:23:41 -0500 Sean Anderson<seanga2@gmail.com>  wrote:
>>> This is working as designed. IMO there's no point in separating
>>> allocations like this which differ only in PID and timestamp, since you
>>> will get no grouping at all.
>>>
>>>> The info of second block missed. So, add -c to turn on culling
>>>> by stacktrace. By default, it will cull by txt.
>>> Please keep the default to actually do something in the cull step.
>>>
>>> ...
>>>
>>>> @@ -162,6 +171,9 @@ int main(int argc, char **argv)
>>>>            case 't':
>>>>                cmp = compare_num;
>>>>                break;
>>>> +        case 'c':
>>>> +            cull_st = 1;
>>>> +            break;
>>> Can we set a "cull_cmp" variable like cmp?
>>>
>>> Looking forward, I think something like
>>>
>>>     page_owner_sort --cull=stacktrace --sort=times foo bar
>>>
>>> would be nice.
>>>
>> Which is unfortunate.
>>
>> I'll send the patch in to Linus anyway, as many other patches
>> syntactically depend on it.  Please work with Sean to address these
>> issues and lets get this resolved over the next few weeks.
>>
>> Also, please cclinux-mm@kvack.org  on changes to page_owner.
>>

Re: [PATCH] tools/vm/page_owner_sort.c: add switch between culling by stacktrace and txt

[Andrew Morton]

On Tue, 22 Mar 2022 10:22:05 +0800 Yinan Zhang <zhangyinan2019@email.szu.edu.cn> wrote:

> on 2022/3/22 4:38, Andrew Morton wrote:
> 
> > These comments were not responded to:
> >
> > On Mon, 29 Nov 2021 21:23:41 -0500 Sean Anderson<seanga2@gmail.com>  wrote:
> >> This is working as designed. IMO there's no point in separating
> >> allocations like this which differ only in PID and timestamp, since you
> >> will get no grouping at all.
> >>
> >>> The info of second block missed. So, add -c to turn on culling
> >>> by stacktrace. By default, it will cull by txt.
> >> Please keep the default to actually do something in the cull step.
> >>
> >> ...
> >>
> >>> @@ -162,6 +171,9 @@ int main(int argc, char **argv)
> >>>    		case 't':
> >>>    			cmp = compare_num;
> >>>    			break;
> >>> +		case 'c':
> >>> +			cull_st = 1;
> >>> +			break;
> >> Can we set a "cull_cmp" variable like cmp?
> >>
> >> Looking forward, I think something like
> >>
> >> 	page_owner_sort --cull=stacktrace --sort=times foo bar
> >>
> >> would be nice.
> >>
> > Which is unfortunate.
> >
> > I'll send the patch in to Linus anyway, as many other patches
> > syntactically depend on it.  Please work with Sean to address these
> > issues and lets get this resolved over the next few weeks.
> >
> > Also, please cclinux-mm@kvack.org  on changes to page_owner.
> >

(top-posting repaired)

> I replied to the email a few months ago. Did you receive it?

I didn't, and it isn't in the list archives at

https://lore.kernel.org/all/20211129145658.2491-1-zhangyinan2019@email.szu.edu.cn/T/#u

Re: [PATCH] tools/vm/page_owner_sort.c: add switch between culling by stacktrace and txt

[Andrew Morton]

On Mon, 21 Mar 2022 23:55:29 -0400 Sean Anderson <seanga2@gmail.com> wrote:

> On 3/21/22 10:22 PM, Yinan Zhang wrote:
> > I replied to the email a few months ago. Did you receive it?
> 
> The patch was applied anyway. Anything in this subsystem gets
> applied within a day or two regardless of feedback. Personally,
> I'm not motivated to review anything because of that.
> 

The feedback wasn't ignored, as you're seeing here.

If the feedback appears to be non-fatal I'll hold onto the patch with
an expectation that the feedback will be addressed and we can move
forward with an updated version of the code.

Re: [PATCH] tools/vm/page_owner_sort.c: add switch between culling by stacktrace and txt

[Andrew Morton]

On Wed, 23 Mar 2022 10:05:28 +0800 Yinan Zhang <zhangyinan2019@email.szu.edu.cn> wrote:

> >> *(&list[i].txt+offset)) != 0) {
> >>               list2[count++] = list[i];
> >>           } else {
> >>               list2[count-1].num += list[i].num;
> >>
> >
> >
> You are right. Due to timestamp etc, it's almost impossible to
> 
> cull by txt. And I did observe some blocks differ in head and same in
> 
> stack trace on my linux. The examples I mentioned in patch are
> 
> part of them.
> 
> 
> Besides, --cull has been added in patch:
> 
> tools/vm/page_owner_sort.c: support for user-defined culling rules
> 

So should I drop this patch?

Re: [PATCH] tools/vm/page_owner_sort.c: add switch between culling by stacktrace and txt

[Andrew Morton]

On Thu, 24 Mar 2022 08:54:34 +0800 Yinan Zhang <zhangyinan2019@email.szu.edu.cn> wrote:

> on 2022/3/24 3:08, Andrew Morton wrote:
> > On Wed, 23 Mar 2022 10:05:28 +0800 Yinan Zhang<zhangyinan2019@email.szu.edu.cn>  wrote:
> >
> >>>> *(&list[i].txt+offset)) != 0) {
> >>>>                list2[count++] = list[i];
> >>>>            } else {
> >>>>                list2[count-1].num += list[i].num;
> >>>>
> >>>
> >> You are right. Due to timestamp etc, it's almost impossible to
> >>
> >> cull by txt. And I did observe some blocks differ in head and same in
> >>
> >> stack trace on my linux. The examples I mentioned in patch are
> >>
> >> part of them.
> >>
> >>
> >> Besides, --cull has been added in patch:
> >>
> >> tools/vm/page_owner_sort.c: support for user-defined culling rules
> >>
> > So should I drop this patch?
> 
> you decide.

I'd rather not.

And dropping this patch makes a huge mess of the series.  So I either
ask for the whole series to be redone and we might miss the merge
window or I send it all in as-is.

Sigh.  I'll send it all in as-is.  Please continue to work on this
issue and if we decide to drop the -c option, please send along a patch
to remove it over the next week or two.