| Message ID | 1530637506-1256-1-git-send-email-rppt@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Jul 3, 2018 at 1:05 PM Mike Rapoport <rppt@linux.vnet.ibm.com> wrote: > > Most functions in memblock already use phys_addr_t to represent a physical > address with __memblock_free_late() being an exception. > > This patch replaces u64 with phys_addr_t in __memblock_free_late() and > switches several format strings from %llx to %pa to avoid casting from > phys_addr_t to u64. > > CC: Michal Hocko <mhocko@kernel.org> > CC: Matthew Wilcox <willy@infradead.org> > Signed-off-by: Mike Rapoport <rppt@linux.vnet.ibm.com> Looks good. Reviewed-by: Pavel Tatashin <pasha.tatashin@oracle.com> One minor thing that I would like to change in memblock.c is the useage phys_addr_t for size. I understand the reasoning behind this choice, but could we do something like: typedef phys_addr_t phys_size_t; It would be similar to resource_size_t. I just think the code and function prototypes would look better with proper typing. Thank you, Pavel
On Tue, 3 Jul 2018 20:05:06 +0300 Mike Rapoport <rppt@linux.vnet.ibm.com> wrote: > Most functions in memblock already use phys_addr_t to represent a physical > address with __memblock_free_late() being an exception. > > This patch replaces u64 with phys_addr_t in __memblock_free_late() and > switches several format strings from %llx to %pa to avoid casting from > phys_addr_t to u64. > > ... > > @@ -1343,9 +1343,9 @@ void * __init memblock_virt_alloc_try_nid_raw( > { > void *ptr; > > - memblock_dbg("%s: %llu bytes align=0x%llx nid=%d from=0x%llx max_addr=0x%llx %pF\n", > - __func__, (u64)size, (u64)align, nid, (u64)min_addr, > - (u64)max_addr, (void *)_RET_IP_); > + memblock_dbg("%s: %llu bytes align=0x%llx nid=%d from=%pa max_addr=%pa %pF\n", > + __func__, (u64)size, (u64)align, nid, &min_addr, > + &max_addr, (void *)_RET_IP_); > Did you see all this checkpatch noise? : WARNING: Deprecated vsprintf pointer extension '%pF' - use %pS instead : #54: FILE: mm/memblock.c:1348: : + memblock_dbg("%s: %llu bytes align=0x%llx nid=%d from=%pa max_addr=%pa %pF\n", : + __func__, (u64)size, (u64)align, nid, &min_addr, : + &max_addr, (void *)_RET_IP_); : ... : * - 'S' For symbolic direct pointers (or function descriptors) with offset * - 's' For symbolic direct pointers (or function descriptors) without offset * - 'F' Same as 'S' * - 'f' Same as 's' I'm not sure why or when all that happened. I suppose we should do that as a separate patch sometime.
On Tue, 2018-07-03 at 12:57 -0700, Andrew Morton wrote: > Did you see all this checkpatch noise? > > : WARNING: Deprecated vsprintf pointer extension '%pF' - use %pS instead > : #54: FILE: mm/memblock.c:1348: > : + memblock_dbg("%s: %llu bytes align=0x%llx nid=%d from=%pa max_addr=%pa %pF\n", > : + __func__, (u64)size, (u64)align, nid, &min_addr, > : + &max_addr, (void *)_RET_IP_); > : ... %p[Ff] got deprecated by commit 04b8eb7a4ccd9ef9343e2720ccf2a5db8cfe2f67 I think it'd be simplest to just convert all the %pF and %pf uses all at once. $ git grep --name-only "%p[Ff]" | \ xargs sed -i -e 's/%pF/%pS/' -e 's/%pf/%ps/' and remove the appropriate Documentation bit.
On Tue, Jul 03, 2018 at 12:57:22PM -0700, Andrew Morton wrote: > On Tue, 3 Jul 2018 20:05:06 +0300 Mike Rapoport <rppt@linux.vnet.ibm.com> wrote: > > > Most functions in memblock already use phys_addr_t to represent a physical > > address with __memblock_free_late() being an exception. > > > > This patch replaces u64 with phys_addr_t in __memblock_free_late() and > > switches several format strings from %llx to %pa to avoid casting from > > phys_addr_t to u64. > > > > ... > > > > @@ -1343,9 +1343,9 @@ void * __init memblock_virt_alloc_try_nid_raw( > > { > > void *ptr; > > > > - memblock_dbg("%s: %llu bytes align=0x%llx nid=%d from=0x%llx max_addr=0x%llx %pF\n", > > - __func__, (u64)size, (u64)align, nid, (u64)min_addr, > > - (u64)max_addr, (void *)_RET_IP_); > > + memblock_dbg("%s: %llu bytes align=0x%llx nid=%d from=%pa max_addr=%pa %pF\n", > > + __func__, (u64)size, (u64)align, nid, &min_addr, > > + &max_addr, (void *)_RET_IP_); > > > > Did you see all this checkpatch noise? > > : WARNING: Deprecated vsprintf pointer extension '%pF' - use %pS instead > : #54: FILE: mm/memblock.c:1348: > : + memblock_dbg("%s: %llu bytes align=0x%llx nid=%d from=%pa max_addr=%pa %pF\n", > : + __func__, (u64)size, (u64)align, nid, &min_addr, > : + &max_addr, (void *)_RET_IP_); > : ... > : Sorry, my bad... > * - 'S' For symbolic direct pointers (or function descriptors) with offset > * - 's' For symbolic direct pointers (or function descriptors) without offset > * - 'F' Same as 'S' > * - 'f' Same as 's' > > I'm not sure why or when all that happened. > > I suppose we should do that as a separate patch sometime. >
On Tue 03-07-18 20:05:06, Mike Rapoport wrote: > Most functions in memblock already use phys_addr_t to represent a physical > address with __memblock_free_late() being an exception. > > This patch replaces u64 with phys_addr_t in __memblock_free_late() and > switches several format strings from %llx to %pa to avoid casting from > phys_addr_t to u64. > > CC: Michal Hocko <mhocko@kernel.org> > CC: Matthew Wilcox <willy@infradead.org> > Signed-off-by: Mike Rapoport <rppt@linux.vnet.ibm.com> > --- > mm/memblock.c | 46 +++++++++++++++++++++++----------------------- > 1 file changed, 23 insertions(+), 23 deletions(-) > > diff --git a/mm/memblock.c b/mm/memblock.c > index 03d48d8..20ad8e9 100644 > --- a/mm/memblock.c > +++ b/mm/memblock.c > @@ -330,7 +330,7 @@ static int __init_memblock memblock_double_array(struct memblock_type *type, > { > struct memblock_region *new_array, *old_array; > phys_addr_t old_alloc_size, new_alloc_size; > - phys_addr_t old_size, new_size, addr; > + phys_addr_t old_size, new_size, addr, new_end; > int use_slab = slab_is_available(); > int *in_slab; > > @@ -391,9 +391,9 @@ static int __init_memblock memblock_double_array(struct memblock_type *type, > return -1; > } > > - memblock_dbg("memblock: %s is doubled to %ld at [%#010llx-%#010llx]", > - type->name, type->max * 2, (u64)addr, > - (u64)addr + new_size - 1); > + new_end = addr + new_size - 1; > + memblock_dbg("memblock: %s is doubled to %ld at [%pa-%pa]", > + type->name, type->max * 2, &addr, &new_end); I didn't get to check this carefully but this surely looks suspicious. I am pretty sure you wanted to print the value here rather than address of the local variable, right?
On Wed, Jul 04, 2018 at 03:05:00PM +0200, Michal Hocko wrote: > On Tue 03-07-18 20:05:06, Mike Rapoport wrote: > > Most functions in memblock already use phys_addr_t to represent a physical > > address with __memblock_free_late() being an exception. > > > > This patch replaces u64 with phys_addr_t in __memblock_free_late() and > > switches several format strings from %llx to %pa to avoid casting from > > phys_addr_t to u64. > > > > CC: Michal Hocko <mhocko@kernel.org> > > CC: Matthew Wilcox <willy@infradead.org> > > Signed-off-by: Mike Rapoport <rppt@linux.vnet.ibm.com> > > --- > > mm/memblock.c | 46 +++++++++++++++++++++++----------------------- > > 1 file changed, 23 insertions(+), 23 deletions(-) > > > > diff --git a/mm/memblock.c b/mm/memblock.c > > index 03d48d8..20ad8e9 100644 > > --- a/mm/memblock.c > > +++ b/mm/memblock.c > > @@ -330,7 +330,7 @@ static int __init_memblock memblock_double_array(struct memblock_type *type, > > { > > struct memblock_region *new_array, *old_array; > > phys_addr_t old_alloc_size, new_alloc_size; > > - phys_addr_t old_size, new_size, addr; > > + phys_addr_t old_size, new_size, addr, new_end; > > int use_slab = slab_is_available(); > > int *in_slab; > > > > @@ -391,9 +391,9 @@ static int __init_memblock memblock_double_array(struct memblock_type *type, > > return -1; > > } > > > > - memblock_dbg("memblock: %s is doubled to %ld at [%#010llx-%#010llx]", > > - type->name, type->max * 2, (u64)addr, > > - (u64)addr + new_size - 1); > > + new_end = addr + new_size - 1; > > + memblock_dbg("memblock: %s is doubled to %ld at [%pa-%pa]", > > + type->name, type->max * 2, &addr, &new_end); > > I didn't get to check this carefully but this surely looks suspicious. I > am pretty sure you wanted to print the value here rather than address of > the local variable, right? It's the semantics of %pa: Physical address types phys_addr_t ---------------------------------- :: %pa[p] 0x01234567 or 0x0123456789abcdef For printing a phys_addr_t type (and its derivatives, such as resource_size_t) which can vary based on build options, regardless of the width of the CPU data path. Passed by reference. > -- > Michal Hocko > SUSE Labs >
On Wed 04-07-18 16:24:11, Mike Rapoport wrote: > On Wed, Jul 04, 2018 at 03:05:00PM +0200, Michal Hocko wrote: > > On Tue 03-07-18 20:05:06, Mike Rapoport wrote: > > > Most functions in memblock already use phys_addr_t to represent a physical > > > address with __memblock_free_late() being an exception. > > > > > > This patch replaces u64 with phys_addr_t in __memblock_free_late() and > > > switches several format strings from %llx to %pa to avoid casting from > > > phys_addr_t to u64. > > > > > > CC: Michal Hocko <mhocko@kernel.org> > > > CC: Matthew Wilcox <willy@infradead.org> > > > Signed-off-by: Mike Rapoport <rppt@linux.vnet.ibm.com> > > > --- > > > mm/memblock.c | 46 +++++++++++++++++++++++----------------------- > > > 1 file changed, 23 insertions(+), 23 deletions(-) > > > > > > diff --git a/mm/memblock.c b/mm/memblock.c > > > index 03d48d8..20ad8e9 100644 > > > --- a/mm/memblock.c > > > +++ b/mm/memblock.c > > > @@ -330,7 +330,7 @@ static int __init_memblock memblock_double_array(struct memblock_type *type, > > > { > > > struct memblock_region *new_array, *old_array; > > > phys_addr_t old_alloc_size, new_alloc_size; > > > - phys_addr_t old_size, new_size, addr; > > > + phys_addr_t old_size, new_size, addr, new_end; > > > int use_slab = slab_is_available(); > > > int *in_slab; > > > > > > @@ -391,9 +391,9 @@ static int __init_memblock memblock_double_array(struct memblock_type *type, > > > return -1; > > > } > > > > > > - memblock_dbg("memblock: %s is doubled to %ld at [%#010llx-%#010llx]", > > > - type->name, type->max * 2, (u64)addr, > > > - (u64)addr + new_size - 1); > > > + new_end = addr + new_size - 1; > > > + memblock_dbg("memblock: %s is doubled to %ld at [%pa-%pa]", > > > + type->name, type->max * 2, &addr, &new_end); > > > > I didn't get to check this carefully but this surely looks suspicious. I > > am pretty sure you wanted to print the value here rather than address of > > the local variable, right? > > It's the semantics of %pa: > > Physical address types phys_addr_t > ---------------------------------- > > :: > > %pa[p] 0x01234567 or 0x0123456789abcdef > > For printing a phys_addr_t type (and its derivatives, such as > resource_size_t) which can vary based on build options, regardless of the > width of the CPU data path. > > Passed by reference. I wasn't aware of that. Thanks for the clarification!
On Tue 03-07-18 20:05:06, Mike Rapoport wrote: > Most functions in memblock already use phys_addr_t to represent a physical > address with __memblock_free_late() being an exception. > > This patch replaces u64 with phys_addr_t in __memblock_free_late() and > switches several format strings from %llx to %pa to avoid casting from > phys_addr_t to u64. > > CC: Michal Hocko <mhocko@kernel.org> > CC: Matthew Wilcox <willy@infradead.org> > Signed-off-by: Mike Rapoport <rppt@linux.vnet.ibm.com> Acked-by: Michal Hocko <mhocko@suse.com> > --- > mm/memblock.c | 46 +++++++++++++++++++++++----------------------- > 1 file changed, 23 insertions(+), 23 deletions(-) > > diff --git a/mm/memblock.c b/mm/memblock.c > index 03d48d8..20ad8e9 100644 > --- a/mm/memblock.c > +++ b/mm/memblock.c > @@ -330,7 +330,7 @@ static int __init_memblock memblock_double_array(struct memblock_type *type, > { > struct memblock_region *new_array, *old_array; > phys_addr_t old_alloc_size, new_alloc_size; > - phys_addr_t old_size, new_size, addr; > + phys_addr_t old_size, new_size, addr, new_end; > int use_slab = slab_is_available(); > int *in_slab; > > @@ -391,9 +391,9 @@ static int __init_memblock memblock_double_array(struct memblock_type *type, > return -1; > } > > - memblock_dbg("memblock: %s is doubled to %ld at [%#010llx-%#010llx]", > - type->name, type->max * 2, (u64)addr, > - (u64)addr + new_size - 1); > + new_end = addr + new_size - 1; > + memblock_dbg("memblock: %s is doubled to %ld at [%pa-%pa]", > + type->name, type->max * 2, &addr, &new_end); > > /* > * Found space, we now need to move the array over before we add the > @@ -1343,9 +1343,9 @@ void * __init memblock_virt_alloc_try_nid_raw( > { > void *ptr; > > - memblock_dbg("%s: %llu bytes align=0x%llx nid=%d from=0x%llx max_addr=0x%llx %pF\n", > - __func__, (u64)size, (u64)align, nid, (u64)min_addr, > - (u64)max_addr, (void *)_RET_IP_); > + memblock_dbg("%s: %llu bytes align=0x%llx nid=%d from=%pa max_addr=%pa %pF\n", > + __func__, (u64)size, (u64)align, nid, &min_addr, > + &max_addr, (void *)_RET_IP_); > > ptr = memblock_virt_alloc_internal(size, align, > min_addr, max_addr, nid); > @@ -1380,9 +1380,9 @@ void * __init memblock_virt_alloc_try_nid_nopanic( > { > void *ptr; > > - memblock_dbg("%s: %llu bytes align=0x%llx nid=%d from=0x%llx max_addr=0x%llx %pF\n", > - __func__, (u64)size, (u64)align, nid, (u64)min_addr, > - (u64)max_addr, (void *)_RET_IP_); > + memblock_dbg("%s: %llu bytes align=0x%llx nid=%d from=%pa max_addr=%pa %pF\n", > + __func__, (u64)size, (u64)align, nid, &min_addr, > + &max_addr, (void *)_RET_IP_); > > ptr = memblock_virt_alloc_internal(size, align, > min_addr, max_addr, nid); > @@ -1416,9 +1416,9 @@ void * __init memblock_virt_alloc_try_nid( > { > void *ptr; > > - memblock_dbg("%s: %llu bytes align=0x%llx nid=%d from=0x%llx max_addr=0x%llx %pF\n", > - __func__, (u64)size, (u64)align, nid, (u64)min_addr, > - (u64)max_addr, (void *)_RET_IP_); > + memblock_dbg("%s: %llu bytes align=0x%llx nid=%d from=%pa max_addr=%pa %pF\n", > + __func__, (u64)size, (u64)align, nid, &min_addr, > + &max_addr, (void *)_RET_IP_); > ptr = memblock_virt_alloc_internal(size, align, > min_addr, max_addr, nid); > if (ptr) { > @@ -1426,9 +1426,8 @@ void * __init memblock_virt_alloc_try_nid( > return ptr; > } > > - panic("%s: Failed to allocate %llu bytes align=0x%llx nid=%d from=0x%llx max_addr=0x%llx\n", > - __func__, (u64)size, (u64)align, nid, (u64)min_addr, > - (u64)max_addr); > + panic("%s: Failed to allocate %llu bytes align=0x%llx nid=%d from=%pa max_addr=%pa\n", > + __func__, (u64)size, (u64)align, nid, &min_addr, &max_addr); > return NULL; > } > > @@ -1442,9 +1441,10 @@ void * __init memblock_virt_alloc_try_nid( > */ > void __init __memblock_free_early(phys_addr_t base, phys_addr_t size) > { > - memblock_dbg("%s: [%#016llx-%#016llx] %pF\n", > - __func__, (u64)base, (u64)base + size - 1, > - (void *)_RET_IP_); > + phys_addr_t end = base + size - 1; > + > + memblock_dbg("%s: [%pa-%pa] %pF\n", > + __func__, &base, &end, (void *)_RET_IP_); > kmemleak_free_part_phys(base, size); > memblock_remove_range(&memblock.reserved, base, size); > } > @@ -1460,11 +1460,11 @@ void __init __memblock_free_early(phys_addr_t base, phys_addr_t size) > */ > void __init __memblock_free_late(phys_addr_t base, phys_addr_t size) > { > - u64 cursor, end; > + phys_addr_t cursor, end; > > - memblock_dbg("%s: [%#016llx-%#016llx] %pF\n", > - __func__, (u64)base, (u64)base + size - 1, > - (void *)_RET_IP_); > + end = base + size - 1; > + memblock_dbg("%s: [%pa-%pa] %pF\n", > + __func__, &base, &end, (void *)_RET_IP_); > kmemleak_free_part_phys(base, size); > cursor = PFN_UP(base); > end = PFN_DOWN(base + size); > -- > 2.7.4
diff --git a/mm/memblock.c b/mm/memblock.c index 03d48d8..20ad8e9 100644 --- a/mm/memblock.c +++ b/mm/memblock.c @@ -330,7 +330,7 @@ static int __init_memblock memblock_double_array(struct memblock_type *type, { struct memblock_region *new_array, *old_array; phys_addr_t old_alloc_size, new_alloc_size; - phys_addr_t old_size, new_size, addr; + phys_addr_t old_size, new_size, addr, new_end; int use_slab = slab_is_available(); int *in_slab; @@ -391,9 +391,9 @@ static int __init_memblock memblock_double_array(struct memblock_type *type, return -1; } - memblock_dbg("memblock: %s is doubled to %ld at [%#010llx-%#010llx]", - type->name, type->max * 2, (u64)addr, - (u64)addr + new_size - 1); + new_end = addr + new_size - 1; + memblock_dbg("memblock: %s is doubled to %ld at [%pa-%pa]", + type->name, type->max * 2, &addr, &new_end); /* * Found space, we now need to move the array over before we add the @@ -1343,9 +1343,9 @@ void * __init memblock_virt_alloc_try_nid_raw( { void *ptr; - memblock_dbg("%s: %llu bytes align=0x%llx nid=%d from=0x%llx max_addr=0x%llx %pF\n", - __func__, (u64)size, (u64)align, nid, (u64)min_addr, - (u64)max_addr, (void *)_RET_IP_); + memblock_dbg("%s: %llu bytes align=0x%llx nid=%d from=%pa max_addr=%pa %pF\n", + __func__, (u64)size, (u64)align, nid, &min_addr, + &max_addr, (void *)_RET_IP_); ptr = memblock_virt_alloc_internal(size, align, min_addr, max_addr, nid); @@ -1380,9 +1380,9 @@ void * __init memblock_virt_alloc_try_nid_nopanic( { void *ptr; - memblock_dbg("%s: %llu bytes align=0x%llx nid=%d from=0x%llx max_addr=0x%llx %pF\n", - __func__, (u64)size, (u64)align, nid, (u64)min_addr, - (u64)max_addr, (void *)_RET_IP_); + memblock_dbg("%s: %llu bytes align=0x%llx nid=%d from=%pa max_addr=%pa %pF\n", + __func__, (u64)size, (u64)align, nid, &min_addr, + &max_addr, (void *)_RET_IP_); ptr = memblock_virt_alloc_internal(size, align, min_addr, max_addr, nid); @@ -1416,9 +1416,9 @@ void * __init memblock_virt_alloc_try_nid( { void *ptr; - memblock_dbg("%s: %llu bytes align=0x%llx nid=%d from=0x%llx max_addr=0x%llx %pF\n", - __func__, (u64)size, (u64)align, nid, (u64)min_addr, - (u64)max_addr, (void *)_RET_IP_); + memblock_dbg("%s: %llu bytes align=0x%llx nid=%d from=%pa max_addr=%pa %pF\n", + __func__, (u64)size, (u64)align, nid, &min_addr, + &max_addr, (void *)_RET_IP_); ptr = memblock_virt_alloc_internal(size, align, min_addr, max_addr, nid); if (ptr) { @@ -1426,9 +1426,8 @@ void * __init memblock_virt_alloc_try_nid( return ptr; } - panic("%s: Failed to allocate %llu bytes align=0x%llx nid=%d from=0x%llx max_addr=0x%llx\n", - __func__, (u64)size, (u64)align, nid, (u64)min_addr, - (u64)max_addr); + panic("%s: Failed to allocate %llu bytes align=0x%llx nid=%d from=%pa max_addr=%pa\n", + __func__, (u64)size, (u64)align, nid, &min_addr, &max_addr); return NULL; } @@ -1442,9 +1441,10 @@ void * __init memblock_virt_alloc_try_nid( */ void __init __memblock_free_early(phys_addr_t base, phys_addr_t size) { - memblock_dbg("%s: [%#016llx-%#016llx] %pF\n", - __func__, (u64)base, (u64)base + size - 1, - (void *)_RET_IP_); + phys_addr_t end = base + size - 1; + + memblock_dbg("%s: [%pa-%pa] %pF\n", + __func__, &base, &end, (void *)_RET_IP_); kmemleak_free_part_phys(base, size); memblock_remove_range(&memblock.reserved, base, size); } @@ -1460,11 +1460,11 @@ void __init __memblock_free_early(phys_addr_t base, phys_addr_t size) */ void __init __memblock_free_late(phys_addr_t base, phys_addr_t size) { - u64 cursor, end; + phys_addr_t cursor, end; - memblock_dbg("%s: [%#016llx-%#016llx] %pF\n", - __func__, (u64)base, (u64)base + size - 1, - (void *)_RET_IP_); + end = base + size - 1; + memblock_dbg("%s: [%pa-%pa] %pF\n", + __func__, &base, &end, (void *)_RET_IP_); kmemleak_free_part_phys(base, size); cursor = PFN_UP(base); end = PFN_DOWN(base + size);
Most functions in memblock already use phys_addr_t to represent a physical address with __memblock_free_late() being an exception. This patch replaces u64 with phys_addr_t in __memblock_free_late() and switches several format strings from %llx to %pa to avoid casting from phys_addr_t to u64. CC: Michal Hocko <mhocko@kernel.org> CC: Matthew Wilcox <willy@infradead.org> Signed-off-by: Mike Rapoport <rppt@linux.vnet.ibm.com> --- mm/memblock.c | 46 +++++++++++++++++++++++----------------------- 1 file changed, 23 insertions(+), 23 deletions(-)