On Thu, 14 May 2015, Sasha Levin wrote:
> diff --git a/include/linux/mmdebug.h b/include/linux/mmdebug.h
> index 202ebdf..8b3f5a0 100644
> --- a/include/linux/mmdebug.h
> +++ b/include/linux/mmdebug.h
> @@ -7,9 +7,7 @@ struct page;
> struct vm_area_struct;
> struct mm_struct;
>
> -extern void dump_page(struct page *page, const char *reason);
> -extern void dump_page_badflags(struct page *page, const char *reason,
> - unsigned long badflags);
> +char *format_page(struct page *page, char *buf, char *end);
>
> #ifdef CONFIG_DEBUG_VM
> char *format_vma(const struct vm_area_struct *vma, char *buf, char *end);
> @@ -18,7 +16,7 @@ char *format_mm(const struct mm_struct *mm, char *buf, char *end);
> #define VM_BUG_ON_PAGE(cond, page) \
> do { \
> if (unlikely(cond)) { \
> - dump_page(page, "VM_BUG_ON_PAGE(" __stringify(cond)")");\
> + pr_emerg("%pZp", page); \
> BUG(); \
> } \
> } while (0)
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index 595bf50..1f045ae 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -1382,6 +1382,8 @@ char *mm_pointer(char *buf, char *end, const void *ptr,
> switch (fmt[1]) {
> case 'm':
> return format_mm(ptr, buf, end);
> + case 'p':
> + return format_page(ptr, buf, end);
> case 'v':
> return format_vma(ptr, buf, end);
> default:
> @@ -1482,9 +1484,10 @@ int kptr_restrict __read_mostly;
> * (legacy clock framework) of the clock
> * - 'Cr' For a clock, it prints the current rate of the clock
> * - 'T' task_struct->comm
> - * - 'Z[mv]' Outputs a readable version of a type of memory management struct:
> + * - 'Z[mpv]' Outputs a readable version of a type of memory management struct:
> * v struct vm_area_struct
> * m struct mm_struct
> + * p struct page
> *
> * Note: The difference between 'S' and 'F' is that on ia64 and ppc64
> * function pointers are really function descriptors, which contain a
> diff --git a/mm/balloon_compaction.c b/mm/balloon_compaction.c
> index fcad832..88b3cae 100644
> --- a/mm/balloon_compaction.c
> +++ b/mm/balloon_compaction.c
> @@ -187,7 +187,7 @@ void balloon_page_putback(struct page *page)
> put_page(page);
> } else {
> WARN_ON(1);
> - dump_page(page, "not movable balloon page");
> + pr_alert("Not movable balloon page:\n%pZp", page);
> }
> unlock_page(page);
> }
I don't know how others feel, but this looks strange to me and seems like
it's only a result of how we must now dump page information
(dump_page(page) is no longer available, we must do pr_alert("%pZp",
page)).
Since we're relying on print formats, this would arguably be better as
pr_alert("Not movable balloon page:\n");
pr_alert("%pZp", page);
to avoid introducing newlines into potentially lengthy messages that need
a specified loglevel like you've done above.
But that's not much different than the existing dump_page()
implementation.
So for this to be worth it, it seems like we'd need a compelling usecase
for something like pr_alert("%pZp %pZv", page, vma) and I'm not sure we're
ever actually going to see that. I would argue that
dump_page(page);
dump_vma(vma);
would be simpler in such circumstances.
I do understand the problem with the current VM_BUG_ON_PAGE() and
VM_BUG_ON_VMA() stuff, and it compels me to ask about just going back to
the normal
VM_BUG_ON(cond);
coupled with dump_page(), dump_vma(), dump_whatever(). It all seems so
much simpler to me.
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxx. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>