All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2] makedumpfile: support _count -> _refcount rename in struct page
@ 2016-06-17  9:41 Vitaly Kuznetsov
  2016-06-21  5:43 ` Atsushi Kumagai
  0 siblings, 1 reply; 7+ messages in thread
From: Vitaly Kuznetsov @ 2016-06-17  9:41 UTC (permalink / raw)
  To: kexec; +Cc: Masaki Tachibana, Atsushi Kumagai, Minoru Usui, David Anderson

_count member was renamed to _refcount in linux commit 0139aa7b7fa12
("mm: rename _count, field of the struct page, to _refcount") and this
broke makedumpfile. The reason for making the change was to find all users
accessing it directly and not through the recommended API. I tried
suggesting to revert the change but failed, I see no other choice than to
start supporting both _count and _refcount in makedumpfile.

Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
Changes since 'v1':
- Make '_refcount' the default [Atsushi Kumagai]
---
 makedumpfile.c | 26 +++++++++++++++++++++-----
 makedumpfile.h |  3 ++-
 2 files changed, 23 insertions(+), 6 deletions(-)

diff --git a/makedumpfile.c b/makedumpfile.c
index 853b999..fd884d3 100644
--- a/makedumpfile.c
+++ b/makedumpfile.c
@@ -1579,7 +1579,14 @@ get_structure_info(void)
 	 */
 	SIZE_INIT(page, "page");
 	OFFSET_INIT(page.flags, "page", "flags");
-	OFFSET_INIT(page._count, "page", "_count");
+	OFFSET_INIT(page._refcount, "page", "_refcount");
+	if (OFFSET(page._refcount) == NOT_FOUND_STRUCTURE) {
+		info->flag_use_count = TRUE;
+		OFFSET_INIT(page._refcount, "page", "_count");
+	} else {
+		info->flag_use_count = FALSE;
+	}
+
 	OFFSET_INIT(page.mapping, "page", "mapping");
 	OFFSET_INIT(page._mapcount, "page", "_mapcount");
 	OFFSET_INIT(page.private, "page", "private");
@@ -2044,7 +2051,7 @@ get_mem_type(void)
 
 	if ((SIZE(page) == NOT_FOUND_STRUCTURE)
 	    || (OFFSET(page.flags) == NOT_FOUND_STRUCTURE)
-	    || (OFFSET(page._count) == NOT_FOUND_STRUCTURE)
+	    || (OFFSET(page._refcount) == NOT_FOUND_STRUCTURE)
 	    || (OFFSET(page.mapping) == NOT_FOUND_STRUCTURE)) {
 		ret = NOT_FOUND_MEMTYPE;
 	} else if ((((SYMBOL(node_data) != NOT_FOUND_SYMBOL)
@@ -2151,7 +2158,10 @@ write_vmcoreinfo_data(void)
 	 * write the member offset of 1st kernel
 	 */
 	WRITE_MEMBER_OFFSET("page.flags", page.flags);
-	WRITE_MEMBER_OFFSET("page._count", page._count);
+	if (info->flag_use_count)
+		WRITE_MEMBER_OFFSET("page._count", page._refcount);
+	else
+		WRITE_MEMBER_OFFSET("page._refcount", page._refcount);
 	WRITE_MEMBER_OFFSET("page.mapping", page.mapping);
 	WRITE_MEMBER_OFFSET("page.lru", page.lru);
 	WRITE_MEMBER_OFFSET("page._mapcount", page._mapcount);
@@ -2491,7 +2501,13 @@ read_vmcoreinfo(void)
 
 
 	READ_MEMBER_OFFSET("page.flags", page.flags);
-	READ_MEMBER_OFFSET("page._count", page._count);
+	READ_MEMBER_OFFSET("page._refcount", page._refcount);
+	if (OFFSET(page._refcount) == NOT_FOUND_STRUCTURE) {
+		info->flag_use_count = TRUE;
+		READ_MEMBER_OFFSET("page._count", page._refcount);
+	} else {
+		info->flag_use_count = FALSE;
+	}
 	READ_MEMBER_OFFSET("page.mapping", page.mapping);
 	READ_MEMBER_OFFSET("page.lru", page.lru);
 	READ_MEMBER_OFFSET("page._mapcount", page._mapcount);
@@ -5615,7 +5631,7 @@ __exclude_unnecessary_pages(unsigned long mem_map,
 		pcache  = page_cache + (index_pg * SIZE(page));
 
 		flags   = ULONG(pcache + OFFSET(page.flags));
-		_count  = UINT(pcache + OFFSET(page._count));
+		_count  = UINT(pcache + OFFSET(page._refcount));
 		mapping = ULONG(pcache + OFFSET(page.mapping));
 
 		if (OFFSET(page.compound_order) != NOT_FOUND_STRUCTURE) {
diff --git a/makedumpfile.h b/makedumpfile.h
index 251d4bf..533e5b8 100644
--- a/makedumpfile.h
+++ b/makedumpfile.h
@@ -1100,6 +1100,7 @@ struct DumpInfo {
 	int		flag_nospace;	     /* the flag of "No space on device" error */
 	int		flag_vmemmap;        /* kernel supports vmemmap address space */
 	int		flag_excludevm;      /* -e - excluding unused vmemmap pages */
+	int		flag_use_count;      /* _refcount is named _count in struct page */
 	unsigned long	vaddr_for_vtop;      /* virtual address for debugging */
 	long		page_size;           /* size of page */
 	long		page_shift;
@@ -1483,7 +1484,7 @@ struct size_table {
 struct offset_table {
 	struct page {
 		long	flags;
-		long	_count;
+		long	_refcount;
 		long	mapping;
 		long	lru;
 		long	_mapcount;
-- 
2.5.5


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

^ permalink raw reply related	[flat|nested] 7+ messages in thread
* RE: [PATCH v2] makedumpfile: support _count -> _refcount rename in struct page
  2016-06-17  9:41 [PATCH v2] makedumpfile: support _count -> _refcount rename in struct page Vitaly Kuznetsov
@ 2016-06-21  5:43 ` Atsushi Kumagai
  2016-06-21  6:45   ` Pratyush Anand
  0 siblings, 1 reply; 7+ messages in thread
From: Atsushi Kumagai @ 2016-06-21  5:43 UTC (permalink / raw)
  To: Vitaly Kuznetsov; +Cc: Masaki Tachibana, Minoru Usui, kexec, David Anderson

Hello Vitaly,

>_count member was renamed to _refcount in linux commit 0139aa7b7fa12
>("mm: rename _count, field of the struct page, to _refcount") and this
>broke makedumpfile. The reason for making the change was to find all users
>accessing it directly and not through the recommended API. I tried
>suggesting to revert the change but failed, I see no other choice than to
>start supporting both _count and _refcount in makedumpfile.
>
>Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
>---
>Changes since 'v1':
>- Make '_refcount' the default [Atsushi Kumagai]

Good fix, I'll merge this into v1.6.1.

Thanks,
Atsushi Kumagai

>---
> makedumpfile.c | 26 +++++++++++++++++++++-----
> makedumpfile.h |  3 ++-
> 2 files changed, 23 insertions(+), 6 deletions(-)
>
>diff --git a/makedumpfile.c b/makedumpfile.c
>index 853b999..fd884d3 100644
>--- a/makedumpfile.c
>+++ b/makedumpfile.c
>@@ -1579,7 +1579,14 @@ get_structure_info(void)
> 	 */
> 	SIZE_INIT(page, "page");
> 	OFFSET_INIT(page.flags, "page", "flags");
>-	OFFSET_INIT(page._count, "page", "_count");
>+	OFFSET_INIT(page._refcount, "page", "_refcount");
>+	if (OFFSET(page._refcount) == NOT_FOUND_STRUCTURE) {
>+		info->flag_use_count = TRUE;
>+		OFFSET_INIT(page._refcount, "page", "_count");
>+	} else {
>+		info->flag_use_count = FALSE;
>+	}
>+
> 	OFFSET_INIT(page.mapping, "page", "mapping");
> 	OFFSET_INIT(page._mapcount, "page", "_mapcount");
> 	OFFSET_INIT(page.private, "page", "private");
>@@ -2044,7 +2051,7 @@ get_mem_type(void)
>
> 	if ((SIZE(page) == NOT_FOUND_STRUCTURE)
> 	    || (OFFSET(page.flags) == NOT_FOUND_STRUCTURE)
>-	    || (OFFSET(page._count) == NOT_FOUND_STRUCTURE)
>+	    || (OFFSET(page._refcount) == NOT_FOUND_STRUCTURE)
> 	    || (OFFSET(page.mapping) == NOT_FOUND_STRUCTURE)) {
> 		ret = NOT_FOUND_MEMTYPE;
> 	} else if ((((SYMBOL(node_data) != NOT_FOUND_SYMBOL)
>@@ -2151,7 +2158,10 @@ write_vmcoreinfo_data(void)
> 	 * write the member offset of 1st kernel
> 	 */
> 	WRITE_MEMBER_OFFSET("page.flags", page.flags);
>-	WRITE_MEMBER_OFFSET("page._count", page._count);
>+	if (info->flag_use_count)
>+		WRITE_MEMBER_OFFSET("page._count", page._refcount);
>+	else
>+		WRITE_MEMBER_OFFSET("page._refcount", page._refcount);
> 	WRITE_MEMBER_OFFSET("page.mapping", page.mapping);
> 	WRITE_MEMBER_OFFSET("page.lru", page.lru);
> 	WRITE_MEMBER_OFFSET("page._mapcount", page._mapcount);
>@@ -2491,7 +2501,13 @@ read_vmcoreinfo(void)
>
>
> 	READ_MEMBER_OFFSET("page.flags", page.flags);
>-	READ_MEMBER_OFFSET("page._count", page._count);
>+	READ_MEMBER_OFFSET("page._refcount", page._refcount);
>+	if (OFFSET(page._refcount) == NOT_FOUND_STRUCTURE) {
>+		info->flag_use_count = TRUE;
>+		READ_MEMBER_OFFSET("page._count", page._refcount);
>+	} else {
>+		info->flag_use_count = FALSE;
>+	}
> 	READ_MEMBER_OFFSET("page.mapping", page.mapping);
> 	READ_MEMBER_OFFSET("page.lru", page.lru);
> 	READ_MEMBER_OFFSET("page._mapcount", page._mapcount);
>@@ -5615,7 +5631,7 @@ __exclude_unnecessary_pages(unsigned long mem_map,
> 		pcache  = page_cache + (index_pg * SIZE(page));
>
> 		flags   = ULONG(pcache + OFFSET(page.flags));
>-		_count  = UINT(pcache + OFFSET(page._count));
>+		_count  = UINT(pcache + OFFSET(page._refcount));
> 		mapping = ULONG(pcache + OFFSET(page.mapping));
>
> 		if (OFFSET(page.compound_order) != NOT_FOUND_STRUCTURE) {
>diff --git a/makedumpfile.h b/makedumpfile.h
>index 251d4bf..533e5b8 100644
>--- a/makedumpfile.h
>+++ b/makedumpfile.h
>@@ -1100,6 +1100,7 @@ struct DumpInfo {
> 	int		flag_nospace;	     /* the flag of "No space on device" error */
> 	int		flag_vmemmap;        /* kernel supports vmemmap address space */
> 	int		flag_excludevm;      /* -e - excluding unused vmemmap pages */
>+	int		flag_use_count;      /* _refcount is named _count in struct page */
> 	unsigned long	vaddr_for_vtop;      /* virtual address for debugging */
> 	long		page_size;           /* size of page */
> 	long		page_shift;
>@@ -1483,7 +1484,7 @@ struct size_table {
> struct offset_table {
> 	struct page {
> 		long	flags;
>-		long	_count;
>+		long	_refcount;
> 		long	mapping;
> 		long	lru;
> 		long	_mapcount;
>--
>2.5.5


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

^ permalink raw reply	[flat|nested] 7+ messages in thread
* Re: [PATCH v2] makedumpfile: support _count -> _refcount rename in struct page
  2016-06-21  5:43 ` Atsushi Kumagai
@ 2016-06-21  6:45   ` Pratyush Anand
  2016-06-21  8:05     ` Vitaly Kuznetsov
  0 siblings, 1 reply; 7+ messages in thread
From: Pratyush Anand @ 2016-06-21  6:45 UTC (permalink / raw)
  To: Atsushi Kumagai
  Cc: Masaki Tachibana, Vitaly Kuznetsov, Minoru Usui, kexec, David Anderson

On 21/06/2016:05:43:29 AM, Atsushi Kumagai wrote:
> Hello Vitaly,
> 
> >_count member was renamed to _refcount in linux commit 0139aa7b7fa12
> >("mm: rename _count, field of the struct page, to _refcount") and this
> >broke makedumpfile. The reason for making the change was to find all users
> >accessing it directly and not through the recommended API. I tried
> >suggesting to revert the change but failed, I see no other choice than to
> >start supporting both _count and _refcount in makedumpfile.
> >
> >Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> >---
> >Changes since 'v1':
> >- Make '_refcount' the default [Atsushi Kumagai]

Sorry, I missed this conversation earlier.

> 
> Good fix, I'll merge this into v1.6.1.
> 
> Thanks,
> Atsushi Kumagai
> 
> >---
> > makedumpfile.c | 26 +++++++++++++++++++++-----
> > makedumpfile.h |  3 ++-
> > 2 files changed, 23 insertions(+), 6 deletions(-)
> >
> >diff --git a/makedumpfile.c b/makedumpfile.c
> >index 853b999..fd884d3 100644
> >--- a/makedumpfile.c
> >+++ b/makedumpfile.c
> >@@ -1579,7 +1579,14 @@ get_structure_info(void)
> > 	 */
> > 	SIZE_INIT(page, "page");
> > 	OFFSET_INIT(page.flags, "page", "flags");
> >-	OFFSET_INIT(page._count, "page", "_count");
> >+	OFFSET_INIT(page._refcount, "page", "_refcount");
> >+	if (OFFSET(page._refcount) == NOT_FOUND_STRUCTURE) {
> >+		info->flag_use_count = TRUE;
> >+		OFFSET_INIT(page._refcount, "page", "_count");
> >+	} else {
> >+		info->flag_use_count = FALSE;

Probably we could have avoided this flag and could have solved it same way as we
have solved for other such kernel changes, like that of "module.module_core".
Infact, we do not need to add "_refcount" in makedumpfile's page struct.

See:
https://github.com/pratyushanand/makedumpfile/commit/8c58c177d27d4ac31db2ee42442ef056dbbd2c93

~Pratyush

> >+	}
> >+
> > 	OFFSET_INIT(page.mapping, "page", "mapping");
> > 	OFFSET_INIT(page._mapcount, "page", "_mapcount");
> > 	OFFSET_INIT(page.private, "page", "private");
> >@@ -2044,7 +2051,7 @@ get_mem_type(void)
> >
> > 	if ((SIZE(page) == NOT_FOUND_STRUCTURE)
> > 	    || (OFFSET(page.flags) == NOT_FOUND_STRUCTURE)
> >-	    || (OFFSET(page._count) == NOT_FOUND_STRUCTURE)
> >+	    || (OFFSET(page._refcount) == NOT_FOUND_STRUCTURE)
> > 	    || (OFFSET(page.mapping) == NOT_FOUND_STRUCTURE)) {
> > 		ret = NOT_FOUND_MEMTYPE;
> > 	} else if ((((SYMBOL(node_data) != NOT_FOUND_SYMBOL)
> >@@ -2151,7 +2158,10 @@ write_vmcoreinfo_data(void)
> > 	 * write the member offset of 1st kernel
> > 	 */
> > 	WRITE_MEMBER_OFFSET("page.flags", page.flags);
> >-	WRITE_MEMBER_OFFSET("page._count", page._count);
> >+	if (info->flag_use_count)
> >+		WRITE_MEMBER_OFFSET("page._count", page._refcount);
> >+	else
> >+		WRITE_MEMBER_OFFSET("page._refcount", page._refcount);
> > 	WRITE_MEMBER_OFFSET("page.mapping", page.mapping);
> > 	WRITE_MEMBER_OFFSET("page.lru", page.lru);
> > 	WRITE_MEMBER_OFFSET("page._mapcount", page._mapcount);
> >@@ -2491,7 +2501,13 @@ read_vmcoreinfo(void)
> >
> >
> > 	READ_MEMBER_OFFSET("page.flags", page.flags);
> >-	READ_MEMBER_OFFSET("page._count", page._count);
> >+	READ_MEMBER_OFFSET("page._refcount", page._refcount);
> >+	if (OFFSET(page._refcount) == NOT_FOUND_STRUCTURE) {
> >+		info->flag_use_count = TRUE;
> >+		READ_MEMBER_OFFSET("page._count", page._refcount);
> >+	} else {
> >+		info->flag_use_count = FALSE;
> >+	}
> > 	READ_MEMBER_OFFSET("page.mapping", page.mapping);
> > 	READ_MEMBER_OFFSET("page.lru", page.lru);
> > 	READ_MEMBER_OFFSET("page._mapcount", page._mapcount);
> >@@ -5615,7 +5631,7 @@ __exclude_unnecessary_pages(unsigned long mem_map,
> > 		pcache  = page_cache + (index_pg * SIZE(page));
> >
> > 		flags   = ULONG(pcache + OFFSET(page.flags));
> >-		_count  = UINT(pcache + OFFSET(page._count));
> >+		_count  = UINT(pcache + OFFSET(page._refcount));
> > 		mapping = ULONG(pcache + OFFSET(page.mapping));
> >
> > 		if (OFFSET(page.compound_order) != NOT_FOUND_STRUCTURE) {
> >diff --git a/makedumpfile.h b/makedumpfile.h
> >index 251d4bf..533e5b8 100644
> >--- a/makedumpfile.h
> >+++ b/makedumpfile.h
> >@@ -1100,6 +1100,7 @@ struct DumpInfo {
> > 	int		flag_nospace;	     /* the flag of "No space on device" error */
> > 	int		flag_vmemmap;        /* kernel supports vmemmap address space */
> > 	int		flag_excludevm;      /* -e - excluding unused vmemmap pages */
> >+	int		flag_use_count;      /* _refcount is named _count in struct page */
> > 	unsigned long	vaddr_for_vtop;      /* virtual address for debugging */
> > 	long		page_size;           /* size of page */
> > 	long		page_shift;
> >@@ -1483,7 +1484,7 @@ struct size_table {
> > struct offset_table {
> > 	struct page {
> > 		long	flags;
> >-		long	_count;
> >+		long	_refcount;
> > 		long	mapping;
> > 		long	lru;
> > 		long	_mapcount;
> >--
> >2.5.5
> 
> 
> _______________________________________________
> kexec mailing list
> kexec@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/kexec

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

^ permalink raw reply	[flat|nested] 7+ messages in thread
* Re: [PATCH v2] makedumpfile: support _count -> _refcount rename in struct page
  2016-06-21  6:45   ` Pratyush Anand
@ 2016-06-21  8:05     ` Vitaly Kuznetsov
  2016-06-21 10:03       ` Pratyush Anand
  0 siblings, 1 reply; 7+ messages in thread
From: Vitaly Kuznetsov @ 2016-06-21  8:05 UTC (permalink / raw)
  To: Pratyush Anand
  Cc: Masaki Tachibana, Atsushi Kumagai, Minoru Usui, kexec, David Anderson

Pratyush Anand <panand@redhat.com> writes:

> On 21/06/2016:05:43:29 AM, Atsushi Kumagai wrote:
>> Hello Vitaly,
>> 
>> >_count member was renamed to _refcount in linux commit 0139aa7b7fa12
>> >("mm: rename _count, field of the struct page, to _refcount") and this
>> >broke makedumpfile. The reason for making the change was to find all users
>> >accessing it directly and not through the recommended API. I tried
>> >suggesting to revert the change but failed, I see no other choice than to
>> >start supporting both _count and _refcount in makedumpfile.
>> >
>> >Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
>> >---
>> >Changes since 'v1':
>> >- Make '_refcount' the default [Atsushi Kumagai]
>
> Sorry, I missed this conversation earlier.
>
>> 
>> Good fix, I'll merge this into v1.6.1.
>> 
>> Thanks,
>> Atsushi Kumagai
>> 
>> >---
>> > makedumpfile.c | 26 +++++++++++++++++++++-----
>> > makedumpfile.h |  3 ++-
>> > 2 files changed, 23 insertions(+), 6 deletions(-)
>> >
>> >diff --git a/makedumpfile.c b/makedumpfile.c
>> >index 853b999..fd884d3 100644
>> >--- a/makedumpfile.c
>> >+++ b/makedumpfile.c
>> >@@ -1579,7 +1579,14 @@ get_structure_info(void)
>> > 	 */
>> > 	SIZE_INIT(page, "page");
>> > 	OFFSET_INIT(page.flags, "page", "flags");
>> >-	OFFSET_INIT(page._count, "page", "_count");
>> >+	OFFSET_INIT(page._refcount, "page", "_refcount");
>> >+	if (OFFSET(page._refcount) == NOT_FOUND_STRUCTURE) {
>> >+		info->flag_use_count = TRUE;
>> >+		OFFSET_INIT(page._refcount, "page", "_count");
>> >+	} else {
>> >+		info->flag_use_count = FALSE;
>
> Probably we could have avoided this flag and could have solved it same way as we
> have solved for other such kernel changes, like that of "module.module_core".
> Infact, we do not need to add "_refcount" in makedumpfile's page struct.
>

Yes, the flag is only needed to do the write. Why do you think we don't
need it?

> See:
> https://github.com/pratyushanand/makedumpfile/commit/8c58c177d27d4ac31db2ee42442ef056dbbd2c93
>
> ~Pratyush
>
>> >+	}
>> >+
>> > 	OFFSET_INIT(page.mapping, "page", "mapping");
>> > 	OFFSET_INIT(page._mapcount, "page", "_mapcount");
>> > 	OFFSET_INIT(page.private, "page", "private");
>> >@@ -2044,7 +2051,7 @@ get_mem_type(void)
>> >
>> > 	if ((SIZE(page) == NOT_FOUND_STRUCTURE)
>> > 	    || (OFFSET(page.flags) == NOT_FOUND_STRUCTURE)
>> >-	    || (OFFSET(page._count) == NOT_FOUND_STRUCTURE)
>> >+	    || (OFFSET(page._refcount) == NOT_FOUND_STRUCTURE)
>> > 	    || (OFFSET(page.mapping) == NOT_FOUND_STRUCTURE)) {
>> > 		ret = NOT_FOUND_MEMTYPE;
>> > 	} else if ((((SYMBOL(node_data) != NOT_FOUND_SYMBOL)
>> >@@ -2151,7 +2158,10 @@ write_vmcoreinfo_data(void)
>> > 	 * write the member offset of 1st kernel
>> > 	 */
>> > 	WRITE_MEMBER_OFFSET("page.flags", page.flags);
>> >-	WRITE_MEMBER_OFFSET("page._count", page._count);
>> >+	if (info->flag_use_count)
>> >+		WRITE_MEMBER_OFFSET("page._count", page._refcount);
>> >+	else
>> >+		WRITE_MEMBER_OFFSET("page._refcount", page._refcount);
>> > 	WRITE_MEMBER_OFFSET("page.mapping", page.mapping);
>> > 	WRITE_MEMBER_OFFSET("page.lru", page.lru);
>> > 	WRITE_MEMBER_OFFSET("page._mapcount", page._mapcount);
>> >@@ -2491,7 +2501,13 @@ read_vmcoreinfo(void)
>> >
>> >
>> > 	READ_MEMBER_OFFSET("page.flags", page.flags);
>> >-	READ_MEMBER_OFFSET("page._count", page._count);
>> >+	READ_MEMBER_OFFSET("page._refcount", page._refcount);
>> >+	if (OFFSET(page._refcount) == NOT_FOUND_STRUCTURE) {
>> >+		info->flag_use_count = TRUE;
>> >+		READ_MEMBER_OFFSET("page._count", page._refcount);
>> >+	} else {
>> >+		info->flag_use_count = FALSE;
>> >+	}
>> > 	READ_MEMBER_OFFSET("page.mapping", page.mapping);
>> > 	READ_MEMBER_OFFSET("page.lru", page.lru);
>> > 	READ_MEMBER_OFFSET("page._mapcount", page._mapcount);
>> >@@ -5615,7 +5631,7 @@ __exclude_unnecessary_pages(unsigned long mem_map,
>> > 		pcache  = page_cache + (index_pg * SIZE(page));
>> >
>> > 		flags   = ULONG(pcache + OFFSET(page.flags));
>> >-		_count  = UINT(pcache + OFFSET(page._count));
>> >+		_count  = UINT(pcache + OFFSET(page._refcount));
>> > 		mapping = ULONG(pcache + OFFSET(page.mapping));
>> >
>> > 		if (OFFSET(page.compound_order) != NOT_FOUND_STRUCTURE) {
>> >diff --git a/makedumpfile.h b/makedumpfile.h
>> >index 251d4bf..533e5b8 100644
>> >--- a/makedumpfile.h
>> >+++ b/makedumpfile.h
>> >@@ -1100,6 +1100,7 @@ struct DumpInfo {
>> > 	int		flag_nospace;	     /* the flag of "No space on device" error */
>> > 	int		flag_vmemmap;        /* kernel supports vmemmap address space */
>> > 	int		flag_excludevm;      /* -e - excluding unused vmemmap pages */
>> >+	int		flag_use_count;      /* _refcount is named _count in struct page */
>> > 	unsigned long	vaddr_for_vtop;      /* virtual address for debugging */
>> > 	long		page_size;           /* size of page */
>> > 	long		page_shift;
>> >@@ -1483,7 +1484,7 @@ struct size_table {
>> > struct offset_table {
>> > 	struct page {
>> > 		long	flags;
>> >-		long	_count;
>> >+		long	_refcount;
>> > 		long	mapping;
>> > 		long	lru;
>> > 		long	_mapcount;
>> >--
>> >2.5.5
>> 
>> 
>> _______________________________________________
>> kexec mailing list
>> kexec@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/kexec

-- 
  Vitaly

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

^ permalink raw reply	[flat|nested] 7+ messages in thread
* Re: [PATCH v2] makedumpfile: support _count -> _refcount rename in struct page
  2016-06-21  8:05     ` Vitaly Kuznetsov
@ 2016-06-21 10:03       ` Pratyush Anand
  2016-06-21 10:36         ` Vitaly Kuznetsov
  0 siblings, 1 reply; 7+ messages in thread
From: Pratyush Anand @ 2016-06-21 10:03 UTC (permalink / raw)
  To: Vitaly Kuznetsov
  Cc: Masaki Tachibana, Atsushi Kumagai, Minoru Usui, kexec, David Anderson

On 21/06/2016:10:05:42 AM, Vitaly Kuznetsov wrote:
> Pratyush Anand <panand@redhat.com> writes:
> 
> > On 21/06/2016:05:43:29 AM, Atsushi Kumagai wrote:
> >> Hello Vitaly,
> >> 
> >> >_count member was renamed to _refcount in linux commit 0139aa7b7fa12
> >> >("mm: rename _count, field of the struct page, to _refcount") and this
> >> >broke makedumpfile. The reason for making the change was to find all users
> >> >accessing it directly and not through the recommended API. I tried
> >> >suggesting to revert the change but failed, I see no other choice than to
> >> >start supporting both _count and _refcount in makedumpfile.
> >> >
> >> >Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> >> >---
> >> >Changes since 'v1':
> >> >- Make '_refcount' the default [Atsushi Kumagai]
> >
> > Sorry, I missed this conversation earlier.
> >
> >> 
> >> Good fix, I'll merge this into v1.6.1.
> >> 
> >> Thanks,
> >> Atsushi Kumagai
> >> 
> >> >---
> >> > makedumpfile.c | 26 +++++++++++++++++++++-----
> >> > makedumpfile.h |  3 ++-
> >> > 2 files changed, 23 insertions(+), 6 deletions(-)
> >> >
> >> >diff --git a/makedumpfile.c b/makedumpfile.c
> >> >index 853b999..fd884d3 100644
> >> >--- a/makedumpfile.c
> >> >+++ b/makedumpfile.c
> >> >@@ -1579,7 +1579,14 @@ get_structure_info(void)
> >> > 	 */
> >> > 	SIZE_INIT(page, "page");
> >> > 	OFFSET_INIT(page.flags, "page", "flags");
> >> >-	OFFSET_INIT(page._count, "page", "_count");
> >> >+	OFFSET_INIT(page._refcount, "page", "_refcount");
> >> >+	if (OFFSET(page._refcount) == NOT_FOUND_STRUCTURE) {
> >> >+		info->flag_use_count = TRUE;
> >> >+		OFFSET_INIT(page._refcount, "page", "_count");
> >> >+	} else {
> >> >+		info->flag_use_count = FALSE;
> >
> > Probably we could have avoided this flag and could have solved it same way as we
> > have solved for other such kernel changes, like that of "module.module_core".
> > Infact, we do not need to add "_refcount" in makedumpfile's page struct.
> >
> 
> Yes, the flag is only needed to do the write. Why do you think we don't
> need it?

IMHO, even if we write with "page._count", we would be able to get correct
value in next read, because we prefer reading with "page._count" over
"page._refcount". I think "crash utility" also gives priority for reading with
_count, so things should work without any issue.

~Pratyush

> 
> > See:
> > https://github.com/pratyushanand/makedumpfile/commit/8c58c177d27d4ac31db2ee42442ef056dbbd2c93
> >
> > ~Pratyush
> >
> >> >+	}
> >> >+
> >> > 	OFFSET_INIT(page.mapping, "page", "mapping");
> >> > 	OFFSET_INIT(page._mapcount, "page", "_mapcount");
> >> > 	OFFSET_INIT(page.private, "page", "private");
> >> >@@ -2044,7 +2051,7 @@ get_mem_type(void)
> >> >
> >> > 	if ((SIZE(page) == NOT_FOUND_STRUCTURE)
> >> > 	    || (OFFSET(page.flags) == NOT_FOUND_STRUCTURE)
> >> >-	    || (OFFSET(page._count) == NOT_FOUND_STRUCTURE)
> >> >+	    || (OFFSET(page._refcount) == NOT_FOUND_STRUCTURE)
> >> > 	    || (OFFSET(page.mapping) == NOT_FOUND_STRUCTURE)) {
> >> > 		ret = NOT_FOUND_MEMTYPE;
> >> > 	} else if ((((SYMBOL(node_data) != NOT_FOUND_SYMBOL)
> >> >@@ -2151,7 +2158,10 @@ write_vmcoreinfo_data(void)
> >> > 	 * write the member offset of 1st kernel
> >> > 	 */
> >> > 	WRITE_MEMBER_OFFSET("page.flags", page.flags);
> >> >-	WRITE_MEMBER_OFFSET("page._count", page._count);
> >> >+	if (info->flag_use_count)
> >> >+		WRITE_MEMBER_OFFSET("page._count", page._refcount);
> >> >+	else
> >> >+		WRITE_MEMBER_OFFSET("page._refcount", page._refcount);
> >> > 	WRITE_MEMBER_OFFSET("page.mapping", page.mapping);
> >> > 	WRITE_MEMBER_OFFSET("page.lru", page.lru);
> >> > 	WRITE_MEMBER_OFFSET("page._mapcount", page._mapcount);
> >> >@@ -2491,7 +2501,13 @@ read_vmcoreinfo(void)
> >> >
> >> >
> >> > 	READ_MEMBER_OFFSET("page.flags", page.flags);
> >> >-	READ_MEMBER_OFFSET("page._count", page._count);
> >> >+	READ_MEMBER_OFFSET("page._refcount", page._refcount);
> >> >+	if (OFFSET(page._refcount) == NOT_FOUND_STRUCTURE) {
> >> >+		info->flag_use_count = TRUE;
> >> >+		READ_MEMBER_OFFSET("page._count", page._refcount);
> >> >+	} else {
> >> >+		info->flag_use_count = FALSE;
> >> >+	}
> >> > 	READ_MEMBER_OFFSET("page.mapping", page.mapping);
> >> > 	READ_MEMBER_OFFSET("page.lru", page.lru);
> >> > 	READ_MEMBER_OFFSET("page._mapcount", page._mapcount);
> >> >@@ -5615,7 +5631,7 @@ __exclude_unnecessary_pages(unsigned long mem_map,
> >> > 		pcache  = page_cache + (index_pg * SIZE(page));
> >> >
> >> > 		flags   = ULONG(pcache + OFFSET(page.flags));
> >> >-		_count  = UINT(pcache + OFFSET(page._count));
> >> >+		_count  = UINT(pcache + OFFSET(page._refcount));
> >> > 		mapping = ULONG(pcache + OFFSET(page.mapping));
> >> >
> >> > 		if (OFFSET(page.compound_order) != NOT_FOUND_STRUCTURE) {
> >> >diff --git a/makedumpfile.h b/makedumpfile.h
> >> >index 251d4bf..533e5b8 100644
> >> >--- a/makedumpfile.h
> >> >+++ b/makedumpfile.h
> >> >@@ -1100,6 +1100,7 @@ struct DumpInfo {
> >> > 	int		flag_nospace;	     /* the flag of "No space on device" error */
> >> > 	int		flag_vmemmap;        /* kernel supports vmemmap address space */
> >> > 	int		flag_excludevm;      /* -e - excluding unused vmemmap pages */
> >> >+	int		flag_use_count;      /* _refcount is named _count in struct page */
> >> > 	unsigned long	vaddr_for_vtop;      /* virtual address for debugging */
> >> > 	long		page_size;           /* size of page */
> >> > 	long		page_shift;
> >> >@@ -1483,7 +1484,7 @@ struct size_table {
> >> > struct offset_table {
> >> > 	struct page {
> >> > 		long	flags;
> >> >-		long	_count;
> >> >+		long	_refcount;
> >> > 		long	mapping;
> >> > 		long	lru;
> >> > 		long	_mapcount;
> >> >--
> >> >2.5.5
> >> 
> >> 
> >> _______________________________________________
> >> kexec mailing list
> >> kexec@lists.infradead.org
> >> http://lists.infradead.org/mailman/listinfo/kexec
> 
> -- 
>   Vitaly

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

^ permalink raw reply	[flat|nested] 7+ messages in thread
* Re: [PATCH v2] makedumpfile: support _count -> _refcount rename in struct page
  2016-06-21 10:03       ` Pratyush Anand
@ 2016-06-21 10:36         ` Vitaly Kuznetsov
  2016-06-22  0:15           ` Atsushi Kumagai
  0 siblings, 1 reply; 7+ messages in thread
From: Vitaly Kuznetsov @ 2016-06-21 10:36 UTC (permalink / raw)
  To: Pratyush Anand
  Cc: Masaki Tachibana, Atsushi Kumagai, Minoru Usui, kexec, David Anderson

Pratyush Anand <panand@redhat.com> writes:

> On 21/06/2016:10:05:42 AM, Vitaly Kuznetsov wrote:
>> Pratyush Anand <panand@redhat.com> writes:
>> 
>> > On 21/06/2016:05:43:29 AM, Atsushi Kumagai wrote:
>> >> Hello Vitaly,
>> >> 
>> >> >_count member was renamed to _refcount in linux commit 0139aa7b7fa12
>> >> >("mm: rename _count, field of the struct page, to _refcount") and this
>> >> >broke makedumpfile. The reason for making the change was to find all users
>> >> >accessing it directly and not through the recommended API. I tried
>> >> >suggesting to revert the change but failed, I see no other choice than to
>> >> >start supporting both _count and _refcount in makedumpfile.
>> >> >
>> >> >Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
>> >> >---
>> >> >Changes since 'v1':
>> >> >- Make '_refcount' the default [Atsushi Kumagai]
>> >
>> > Sorry, I missed this conversation earlier.
>> >
>> >> 
>> >> Good fix, I'll merge this into v1.6.1.
>> >> 
>> >> Thanks,
>> >> Atsushi Kumagai
>> >> 
>> >> >---
>> >> > makedumpfile.c | 26 +++++++++++++++++++++-----
>> >> > makedumpfile.h |  3 ++-
>> >> > 2 files changed, 23 insertions(+), 6 deletions(-)
>> >> >
>> >> >diff --git a/makedumpfile.c b/makedumpfile.c
>> >> >index 853b999..fd884d3 100644
>> >> >--- a/makedumpfile.c
>> >> >+++ b/makedumpfile.c
>> >> >@@ -1579,7 +1579,14 @@ get_structure_info(void)
>> >> > 	 */
>> >> > 	SIZE_INIT(page, "page");
>> >> > 	OFFSET_INIT(page.flags, "page", "flags");
>> >> >-	OFFSET_INIT(page._count, "page", "_count");
>> >> >+	OFFSET_INIT(page._refcount, "page", "_refcount");
>> >> >+	if (OFFSET(page._refcount) == NOT_FOUND_STRUCTURE) {
>> >> >+		info->flag_use_count = TRUE;
>> >> >+		OFFSET_INIT(page._refcount, "page", "_count");
>> >> >+	} else {
>> >> >+		info->flag_use_count = FALSE;
>> >
>> > Probably we could have avoided this flag and could have solved it same way as we
>> > have solved for other such kernel changes, like that of "module.module_core".
>> > Infact, we do not need to add "_refcount" in makedumpfile's page struct.
>> >
>> 
>> Yes, the flag is only needed to do the write. Why do you think we don't
>> need it?
>
> IMHO, even if we write with "page._count", we would be able to get correct
> value in next read, because we prefer reading with "page._count" over
> "page._refcount". I think "crash utility" also gives priority for reading with
> _count, so things should work without any issue.

While things will probably work now while everyone still remembers the
old "_count" name it may happen that some tool will drop this support in
future and this will lead to an awkward situation when everythin works
with /proc/vmcore but doesn't work with makedumpfile's output. Said that
I think it makes sense for makedumpfile to be precise and write what we
read. IMO, of course, I'm not a regular makedumpfile contributor.

<skip>

-- 
  Vitaly

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

^ permalink raw reply	[flat|nested] 7+ messages in thread
* RE: [PATCH v2] makedumpfile: support _count -> _refcount rename in struct page
  2016-06-21 10:36         ` Vitaly Kuznetsov
@ 2016-06-22  0:15           ` Atsushi Kumagai
  0 siblings, 0 replies; 7+ messages in thread
From: Atsushi Kumagai @ 2016-06-22  0:15 UTC (permalink / raw)
  To: Vitaly Kuznetsov, Pratyush Anand
  Cc: Masaki Tachibana, Minoru Usui, kexec, David Anderson

>Pratyush Anand <panand@redhat.com> writes:
>
>> On 21/06/2016:10:05:42 AM, Vitaly Kuznetsov wrote:
>>> Pratyush Anand <panand@redhat.com> writes:
>>>
>>> > On 21/06/2016:05:43:29 AM, Atsushi Kumagai wrote:
>>> >> Hello Vitaly,
>>> >>
>>> >> >_count member was renamed to _refcount in linux commit 0139aa7b7fa12
>>> >> >("mm: rename _count, field of the struct page, to _refcount") and this
>>> >> >broke makedumpfile. The reason for making the change was to find all users
>>> >> >accessing it directly and not through the recommended API. I tried
>>> >> >suggesting to revert the change but failed, I see no other choice than to
>>> >> >start supporting both _count and _refcount in makedumpfile.
>>> >> >
>>> >> >Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
>>> >> >---
>>> >> >Changes since 'v1':
>>> >> >- Make '_refcount' the default [Atsushi Kumagai]
>>> >
>>> > Sorry, I missed this conversation earlier.
>>> >
>>> >>
>>> >> Good fix, I'll merge this into v1.6.1.
>>> >>
>>> >> Thanks,
>>> >> Atsushi Kumagai
>>> >>
>>> >> >---
>>> >> > makedumpfile.c | 26 +++++++++++++++++++++-----
>>> >> > makedumpfile.h |  3 ++-
>>> >> > 2 files changed, 23 insertions(+), 6 deletions(-)
>>> >> >
>>> >> >diff --git a/makedumpfile.c b/makedumpfile.c
>>> >> >index 853b999..fd884d3 100644
>>> >> >--- a/makedumpfile.c
>>> >> >+++ b/makedumpfile.c
>>> >> >@@ -1579,7 +1579,14 @@ get_structure_info(void)
>>> >> > 	 */
>>> >> > 	SIZE_INIT(page, "page");
>>> >> > 	OFFSET_INIT(page.flags, "page", "flags");
>>> >> >-	OFFSET_INIT(page._count, "page", "_count");
>>> >> >+	OFFSET_INIT(page._refcount, "page", "_refcount");
>>> >> >+	if (OFFSET(page._refcount) == NOT_FOUND_STRUCTURE) {
>>> >> >+		info->flag_use_count = TRUE;
>>> >> >+		OFFSET_INIT(page._refcount, "page", "_count");
>>> >> >+	} else {
>>> >> >+		info->flag_use_count = FALSE;
>>> >
>>> > Probably we could have avoided this flag and could have solved it same way as we
>>> > have solved for other such kernel changes, like that of "module.module_core".
>>> > Infact, we do not need to add "_refcount" in makedumpfile's page struct.
>>> >
>>>
>>> Yes, the flag is only needed to do the write. Why do you think we don't
>>> need it?
>>
>> IMHO, even if we write with "page._count", we would be able to get correct
>> value in next read, because we prefer reading with "page._count" over
>> "page._refcount". I think "crash utility" also gives priority for reading with
>> _count, so things should work without any issue.
>
>While things will probably work now while everyone still remembers the
>old "_count" name it may happen that some tool will drop this support in
>future and this will lead to an awkward situation when everythin works
>with /proc/vmcore but doesn't work with makedumpfile's output. Said that
>I think it makes sense for makedumpfile to be precise and write what we
>read. IMO, of course, I'm not a regular makedumpfile contributor.

Yeah, I think it would be better to write down the correct current name
even if the code gets a bit more complicated, thus I accepted the Vitaly's patch.


Thanks,
Atsushi Kumagai

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

^ permalink raw reply	[flat|nested] 7+ messages in thread
end of thread, other threads:[~2016-06-22  0:17 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-06-17  9:41 [PATCH v2] makedumpfile: support _count -> _refcount rename in struct page Vitaly Kuznetsov
2016-06-21  5:43 ` Atsushi Kumagai
2016-06-21  6:45   ` Pratyush Anand
2016-06-21  8:05     ` Vitaly Kuznetsov
2016-06-21 10:03       ` Pratyush Anand
2016-06-21 10:36         ` Vitaly Kuznetsov
2016-06-22  0:15           ` Atsushi Kumagai

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.