On Mon, Jun 29, 2020 at 02:55:21PM -0700, John Hubbard wrote:
> Cool! I've got a few minor feedback comments coming for the individual
> patches, but an easy question first:
>
> For the FOLL_PIN pages, I left in a minor mess: the lines are basically
> 2x too long. Would you be interested in folding in something approximately
> like this, below, to one of your patches? It's just a line break, effectively.
> It generates output like this:
>
> [ 260.545212] page:0000000035202f6e refcount:513 mapcount:1 mapping:0000000000000000 index:0x0
> [ 260.552834] head:0000000035202f6e order:9 compound_mapcount:1 compound_pincount:512
> [ 260.560087] anon flags: 0x17ffe000001000e(referenced|uptodate|dirty|head)
> [ 260.565993] raw: 017ffe000001000e ffffffff83649ca0 ffffea0020758008 ffff888890f03839
> [ 260.572888] raw: 0000000000000000 0000000000000000 00000201ffffffff 0000000000000000
> [ 260.579745] page dumped because: gup_benchmark: head page: dump_page test
>
> (I'm not sure if the indentation is a good idea or not, actually.)
>
> Or if it's too much fuss, I can send it separately with your series as a
> prerequisite:
>
> diff --git a/mm/debug.c b/mm/debug.c
> index fcf3a16902b2..d522bf24e3ff 100644
> --- a/mm/debug.c
> +++ b/mm/debug.c
> @@ -87,19 +87,22 @@ void __dump_page(struct page *page, const char *reason)
> if (compound)
> if (hpage_pincount_available(page)) {
> pr_warn("page:%p refcount:%d mapcount:%d mapping:%p "
> - "index:%#lx head:%p order:%u "
> - "compound_mapcount:%d compound_pincount:%d\n",
> + "index:%#lx\n",
> page, page_ref_count(head), mapcount,
> - mapping, page_to_pgoff(page), head,
> - compound_order(head), compound_mapcount(page),
> + mapping, page_to_pgoff(page));
> + pr_warn(" head:%p order:%u compound_mapcount:%d "
> + "compound_pincount:%d\n",
> + head, compound_order(head),
> + compound_mapcount(page),
> compound_pincount(page));
> } else {
> pr_warn("page:%p refcount:%d mapcount:%d mapping:%p "
> - "index:%#lx head:%p order:%u "
> - "compound_mapcount:%d\n",
> - page, page_ref_count(head), mapcount,
> - mapping, page_to_pgoff(page), head,
> - compound_order(head), compound_mapcount(page));
> + "index:%#lx\n",
> + page, page_ref_count(head), mapcount, mapping,
> + page_to_pgoff(page));
> + pr_warn(" head:%p order:%u compound_mapcount:%d\n",
> + head, compound_order(head),
> + compound_mapcount(page));
> }
Hmm ... If we're going to two lines, then I'd rather do something like this:
+++ b/mm/debug.c
@@ -84,27 +84,22 @@ void __dump_page(struct page *page, const char *reason)
*/
mapcount = PageSlab(head) ? 0 : page_mapcount(page);
- if (compound)
+ pr_warn("page:%p refcount:%d mapcount:%d mapping:%p index:%#lx\n",
+ page, page_ref_count(head), mapcount, mapping,
+ page_to_pgoff(page));
+ if (compound) {
if (hpage_pincount_available(page)) {
- pr_warn("page:%p refcount:%d mapcount:%d mapping:%p "
- "index:%#lx head:%p order:%u "
- "compound_mapcount:%d compound_pincount:%d\n",
- page, page_ref_count(head), mapcount,
- mapping, page_to_pgoff(page), head,
- compound_order(head), compound_mapcount(page),
- compound_pincount(page));
+ pr_warn("head:%p order:%u compound_mapcount:%d compound_pincount:%d\n",
+ head, compound_order(head),
+ compound_mapcount(page),
+ compound_pincount(page));
} else {
- pr_warn("page:%p refcount:%d mapcount:%d mapping:%p "
- "index:%#lx head:%p order:%u "
- "compound_mapcount:%d\n",
- page, page_ref_count(head), mapcount,
- mapping, page_to_pgoff(page), head,
- compound_order(head), compound_mapcount(page));
+ pr_warn("head:%p order:%u compound_mapcount:%d\n",
+ head, compound_order(head),
+ compound_mapcount(page));
}
- else
- pr_warn("page:%p refcount:%d mapcount:%d mapping:%p index:%#lx\n",
- page, page_ref_count(page), mapcount,
- mapping, page_to_pgoff(page));
+ }
+
if (PageKsm(page))
type = "ksm ";
else if (PageAnon(page))
I haven't dumped a page with this yet, so I may change my mind, but
this seems like a good reduction in the amount of redundant code in
this function.
