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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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;
>
These comments were not responded to:
On Mon, 29 Nov 2021 21:23:41 -0500 Sean Anderson <[email protected]> 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 [email protected] on changes to page_owner.
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<[email protected]> 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 [email protected] on changes to page_owner.
>>
On Tue, 22 Mar 2022 10:22:05 +0800 Yinan Zhang <[email protected]> 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<[email protected]> 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 [email protected] 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/[email protected]/T/#u
On Mon, 21 Mar 2022 23:55:29 -0400 Sean Anderson <[email protected]> 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.
On Thu, 24 Mar 2022 08:54:34 +0800 Yinan Zhang <[email protected]> wrote:
> on 2022/3/24 3:08, Andrew Morton wrote:
> > On Wed, 23 Mar 2022 10:05:28 +0800 Yinan Zhang<[email protected]> 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.
On Wed, 23 Mar 2022 10:05:28 +0800 Yinan Zhang <[email protected]> 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?