Hi Tao,
Thanks for your review.
>
> This is a big patch, can we separate it into multiple smaller ones?
> From my understanding about this patch, it involved 2 main features,
> one for dump the page_owner allocated stack trace, one for dump the
> slab debug trace. So at least it can be divided into 2 from my view.
>
Sure, I will divided it into 2 changes and resend them for review.
>
> Why put the tflag check within the pflag check? Do you think the
> following would be better?
>
> if (tflag)
> meminfo.flags |= GET_PAGE_OWNER;
> if (fplag) {...}
>
because the -t flag is introduced for only two usage:
+#define GET_SLAB_DEBUG_TRACE (ADDRESS_SPECIFIED << 28)
+#define GET_PAGE_OWNER (ADDRESS_SPECIFIED << 29)
so the extra tflag must be used with -p and -S or -s.
>
> if (tflag)
> meminfo.flags = GET_PAGE_OWNER;
> if (pflag == 1) {...}
>
>
> This variable is not used, can you check its usage?
>
Here is used with "kmem -pt" without specified address to get page_owner for
all allocated page in buddy system.
> Also for this large inserting hunk, please reformat your patch, keep
> the length of code no longer than 80 chars.
>
>
> There is no need to write a new function for it, we can reuse the
> existing function get_uptime().
>
>
> better put the check into the caller:
>
> if (si->flags & GET_SLAB_DEBUG_TRACE)
> slab_debug_trace_show();
Okay, I will make an improvement in patch V2 as your suggestions.
Thanks
Qiwu
--
Crash-utility mailing list -- [email protected]
To unsubscribe send an email to [email protected]
https://${domain_name}/admin/lists/devel.lists.crash-utility.osci.io/
Contribution Guidelines: https://github.com/crash-utility/crash/wiki
