| Message ID | 20190614100114.311-2-david@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | mm: Further memory block device cleanups | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | warning | Failed to apply on branch next (a3bf9fbdad600b1e4335dd90979f8d6072e4f602) |
| snowpatch_ozlabs/apply_patch | fail | Failed to apply to any branch |
On Fri, 14 Jun 2019 12:01:09 +0200 David Hildenbrand <david@redhat.com> wrote: > We are using a mixture of "int" and "unsigned long". Let's make this > consistent by using "unsigned long" everywhere. We'll do the same with > memory block ids next. > > ... > > - int i, ret, section_count = 0; > + unsigned long i; > > ... > > - unsigned int i; > + unsigned long i; Maybe I did too much fortran back in the day, but I think the expectation is that a variable called "i" has type "int". This? s/unsigned long i/unsigned long section_nr/ --- a/drivers/base/memory.c~mm-section-numbers-use-the-type-unsigned-long-fix +++ a/drivers/base/memory.c @@ -131,17 +131,17 @@ static ssize_t phys_index_show(struct de static ssize_t removable_show(struct device *dev, struct device_attribute *attr, char *buf) { - unsigned long i, pfn; + unsigned long section_nr, pfn; int ret = 1; struct memory_block *mem = to_memory_block(dev); if (mem->state != MEM_ONLINE) goto out; - for (i = 0; i < sections_per_block; i++) { - if (!present_section_nr(mem->start_section_nr + i)) + for (section_nr = 0; section_nr < sections_per_block; section_nr++) { + if (!present_section_nr(mem->start_section_nr + section_nr)) continue; - pfn = section_nr_to_pfn(mem->start_section_nr + i); + pfn = section_nr_to_pfn(mem->start_section_nr + section_nr); ret &= is_mem_section_removable(pfn, PAGES_PER_SECTION); } @@ -695,12 +695,12 @@ static int add_memory_block(unsigned lon { int ret, section_count = 0; struct memory_block *mem; - unsigned long i; + unsigned long section_nr; - for (i = base_section_nr; - i < base_section_nr + sections_per_block; - i++) - if (present_section_nr(i)) + for (section_nr = base_section_nr; + section_nr < base_section_nr + sections_per_block; + section_nr++) + if (present_section_nr(section_nr)) section_count++; if (section_count == 0) @@ -823,7 +823,7 @@ static const struct attribute_group *mem */ int __init memory_dev_init(void) { - unsigned long i; + unsigned long section_nr; int ret; int err; unsigned long block_sz; @@ -840,9 +840,9 @@ int __init memory_dev_init(void) * during boot and have been initialized */ mutex_lock(&mem_sysfs_mutex); - for (i = 0; i <= __highest_present_section_nr; - i += sections_per_block) { - err = add_memory_block(i); + for (section_nr = 0; section_nr <= __highest_present_section_nr; + section_nr += sections_per_block) { + err = add_memory_block(section_nr); if (!ret) ret = err; }
On 14.06.19 21:00, Andrew Morton wrote: > On Fri, 14 Jun 2019 12:01:09 +0200 David Hildenbrand <david@redhat.com> wrote: > >> We are using a mixture of "int" and "unsigned long". Let's make this >> consistent by using "unsigned long" everywhere. We'll do the same with >> memory block ids next. >> >> ... >> >> - int i, ret, section_count = 0; >> + unsigned long i; >> >> ... >> >> - unsigned int i; >> + unsigned long i; > > Maybe I did too much fortran back in the day, but I think the > expectation is that a variable called "i" has type "int". > > This? t460s: ~/git/linux memory_block_devices2 $ git grep "unsigned long i;" | wc -l 245 t460s: ~/git/linux memory_block_devices2 $ git grep "int i;" | wc -l 26827 Yes ;) While it makes sense for the second and third occurrence, I think for the first one it could be confusing (it's not actually a section number but a counter for sections_per_block). I see just now that we can avoid converting the first occurrence completely. So maybe we should drop changing removable_show() from this patch. Cheers! > > > > s/unsigned long i/unsigned long section_nr/ > > --- a/drivers/base/memory.c~mm-section-numbers-use-the-type-unsigned-long-fix > +++ a/drivers/base/memory.c > @@ -131,17 +131,17 @@ static ssize_t phys_index_show(struct de > static ssize_t removable_show(struct device *dev, struct device_attribute *attr, > char *buf) > { > - unsigned long i, pfn; > + unsigned long section_nr, pfn; > int ret = 1; > struct memory_block *mem = to_memory_block(dev); > > if (mem->state != MEM_ONLINE) > goto out; > > - for (i = 0; i < sections_per_block; i++) { > - if (!present_section_nr(mem->start_section_nr + i)) > + for (section_nr = 0; section_nr < sections_per_block; section_nr++) { > + if (!present_section_nr(mem->start_section_nr + section_nr)) > continue; > - pfn = section_nr_to_pfn(mem->start_section_nr + i); > + pfn = section_nr_to_pfn(mem->start_section_nr + section_nr); > ret &= is_mem_section_removable(pfn, PAGES_PER_SECTION); > } > > @@ -695,12 +695,12 @@ static int add_memory_block(unsigned lon > { > int ret, section_count = 0; > struct memory_block *mem; > - unsigned long i; > + unsigned long section_nr; > > - for (i = base_section_nr; > - i < base_section_nr + sections_per_block; > - i++) > - if (present_section_nr(i)) > + for (section_nr = base_section_nr; > + section_nr < base_section_nr + sections_per_block; > + section_nr++) > + if (present_section_nr(section_nr)) > section_count++; > > if (section_count == 0) > @@ -823,7 +823,7 @@ static const struct attribute_group *mem > */ > int __init memory_dev_init(void) > { > - unsigned long i; > + unsigned long section_nr; > int ret; > int err; > unsigned long block_sz; > @@ -840,9 +840,9 @@ int __init memory_dev_init(void) > * during boot and have been initialized > */ > mutex_lock(&mem_sysfs_mutex); > - for (i = 0; i <= __highest_present_section_nr; > - i += sections_per_block) { > - err = add_memory_block(i); > + for (section_nr = 0; section_nr <= __highest_present_section_nr; > + section_nr += sections_per_block) { > + err = add_memory_block(section_nr); > if (!ret) > ret = err; > } > _ >
Le 14/06/2019 à 21:00, Andrew Morton a écrit : > On Fri, 14 Jun 2019 12:01:09 +0200 David Hildenbrand <david@redhat.com> wrote: > >> We are using a mixture of "int" and "unsigned long". Let's make this >> consistent by using "unsigned long" everywhere. We'll do the same with >> memory block ids next. >> >> ... >> >> - int i, ret, section_count = 0; >> + unsigned long i; >> >> ... >> >> - unsigned int i; >> + unsigned long i; > > Maybe I did too much fortran back in the day, but I think the > expectation is that a variable called "i" has type "int". > > This? > > > > s/unsigned long i/unsigned long section_nr/ From my point of view you degrade readability by doing that. section_nr_to_pfn(mem->start_section_nr + section_nr); Three times the word 'section_nr' in one line, is that worth it ? Gives me headache. Codying style says the following, which makes full sense in my opinion: LOCAL variable names should be short, and to the point. If you have some random integer loop counter, it should probably be called ``i``. Calling it ``loop_counter`` is non-productive, if there is no chance of it being mis-understood. What about just naming it 'nr' if we want to use something else than 'i' ? Christophe > > --- a/drivers/base/memory.c~mm-section-numbers-use-the-type-unsigned-long-fix > +++ a/drivers/base/memory.c > @@ -131,17 +131,17 @@ static ssize_t phys_index_show(struct de > static ssize_t removable_show(struct device *dev, struct device_attribute *attr, > char *buf) > { > - unsigned long i, pfn; > + unsigned long section_nr, pfn; > int ret = 1; > struct memory_block *mem = to_memory_block(dev); > > if (mem->state != MEM_ONLINE) > goto out; > > - for (i = 0; i < sections_per_block; i++) { > - if (!present_section_nr(mem->start_section_nr + i)) > + for (section_nr = 0; section_nr < sections_per_block; section_nr++) { > + if (!present_section_nr(mem->start_section_nr + section_nr)) > continue; > - pfn = section_nr_to_pfn(mem->start_section_nr + i); > + pfn = section_nr_to_pfn(mem->start_section_nr + section_nr); > ret &= is_mem_section_removable(pfn, PAGES_PER_SECTION); > } > > @@ -695,12 +695,12 @@ static int add_memory_block(unsigned lon > { > int ret, section_count = 0; > struct memory_block *mem; > - unsigned long i; > + unsigned long section_nr; > > - for (i = base_section_nr; > - i < base_section_nr + sections_per_block; > - i++) > - if (present_section_nr(i)) > + for (section_nr = base_section_nr; > + section_nr < base_section_nr + sections_per_block; > + section_nr++) > + if (present_section_nr(section_nr)) > section_count++; > > if (section_count == 0) > @@ -823,7 +823,7 @@ static const struct attribute_group *mem > */ > int __init memory_dev_init(void) > { > - unsigned long i; > + unsigned long section_nr; > int ret; > int err; > unsigned long block_sz; > @@ -840,9 +840,9 @@ int __init memory_dev_init(void) > * during boot and have been initialized > */ > mutex_lock(&mem_sysfs_mutex); > - for (i = 0; i <= __highest_present_section_nr; > - i += sections_per_block) { > - err = add_memory_block(i); > + for (section_nr = 0; section_nr <= __highest_present_section_nr; > + section_nr += sections_per_block) { > + err = add_memory_block(section_nr); > if (!ret) > ret = err; > } > _ >
On Sat, 15 Jun 2019 10:06:54 +0200 Christophe Leroy <christophe.leroy@c-s.fr> wrote: > > > Le 14/06/2019 à 21:00, Andrew Morton a écrit : > > On Fri, 14 Jun 2019 12:01:09 +0200 David Hildenbrand <david@redhat.com> wrote: > > > >> We are using a mixture of "int" and "unsigned long". Let's make this > >> consistent by using "unsigned long" everywhere. We'll do the same with > >> memory block ids next. > >> > >> ... > >> > >> - int i, ret, section_count = 0; > >> + unsigned long i; > >> > >> ... > >> > >> - unsigned int i; > >> + unsigned long i; > > > > Maybe I did too much fortran back in the day, but I think the > > expectation is that a variable called "i" has type "int". > > > > This? > > > > > > > > s/unsigned long i/unsigned long section_nr/ > > From my point of view you degrade readability by doing that. > > section_nr_to_pfn(mem->start_section_nr + section_nr); > > Three times the word 'section_nr' in one line, is that worth it ? Gives > me headache. > > Codying style says the following, which makes full sense in my opinion: > > LOCAL variable names should be short, and to the point. If you have > some random integer loop counter, it should probably be called ``i``. > Calling it ``loop_counter`` is non-productive, if there is no chance of it > being mis-understood. Well. It did say "integer". Calling an unsigned long `i' is flat out misleading. > What about just naming it 'nr' if we want to use something else than 'i' ? Sure, that works.
Andrew Morton <akpm@linux-foundation.org> writes: > On Sat, 15 Jun 2019 10:06:54 +0200 Christophe Leroy <christophe.leroy@c-s.fr> wrote: >> Le 14/06/2019 à 21:00, Andrew Morton a écrit : >> > On Fri, 14 Jun 2019 12:01:09 +0200 David Hildenbrand <david@redhat.com> wrote: >> > >> >> We are using a mixture of "int" and "unsigned long". Let's make this >> >> consistent by using "unsigned long" everywhere. We'll do the same with >> >> memory block ids next. >> >> >> >> ... >> >> >> >> - int i, ret, section_count = 0; >> >> + unsigned long i; >> >> >> >> ... >> >> >> >> - unsigned int i; >> >> + unsigned long i; >> > >> > Maybe I did too much fortran back in the day, but I think the >> > expectation is that a variable called "i" has type "int". ... >> Codying style says the following, which makes full sense in my opinion: >> >> LOCAL variable names should be short, and to the point. If you have >> some random integer loop counter, it should probably be called ``i``. >> Calling it ``loop_counter`` is non-productive, if there is no chance of it >> being mis-understood. > > Well. It did say "integer". Calling an unsigned long `i' is flat out > misleading. I always thought `i` was for loop `index` not `integer`. But I've never written any Fortran :) cheers
diff --git a/drivers/base/memory.c b/drivers/base/memory.c index 826dd76f662e..5b3a2fd250ba 100644 --- a/drivers/base/memory.c +++ b/drivers/base/memory.c @@ -34,7 +34,7 @@ static DEFINE_MUTEX(mem_sysfs_mutex); static int sections_per_block; -static inline int base_memory_block_id(int section_nr) +static inline int base_memory_block_id(unsigned long section_nr) { return section_nr / sections_per_block; } @@ -691,10 +691,11 @@ static int init_memory_block(struct memory_block **memory, int block_id, return ret; } -static int add_memory_block(int base_section_nr) +static int add_memory_block(unsigned long base_section_nr) { + int ret, section_count = 0; struct memory_block *mem; - int i, ret, section_count = 0; + unsigned long i; for (i = base_section_nr; i < base_section_nr + sections_per_block; @@ -822,7 +823,7 @@ static const struct attribute_group *memory_root_attr_groups[] = { */ int __init memory_dev_init(void) { - unsigned int i; + unsigned long i; int ret; int err; unsigned long block_sz; diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h index 427b79c39b3c..83b6aae16f13 100644 --- a/include/linux/mmzone.h +++ b/include/linux/mmzone.h @@ -1220,7 +1220,7 @@ static inline struct mem_section *__nr_to_section(unsigned long nr) return NULL; return &mem_section[SECTION_NR_TO_ROOT(nr)][nr & SECTION_ROOT_MASK]; } -extern int __section_nr(struct mem_section* ms); +extern unsigned long __section_nr(struct mem_section *ms); extern unsigned long usemap_size(void); /* @@ -1292,7 +1292,7 @@ static inline struct mem_section *__pfn_to_section(unsigned long pfn) return __nr_to_section(pfn_to_section_nr(pfn)); } -extern int __highest_present_section_nr; +extern unsigned long __highest_present_section_nr; #ifndef CONFIG_HAVE_ARCH_PFN_VALID static inline int pfn_valid(unsigned long pfn) diff --git a/mm/sparse.c b/mm/sparse.c index 1552c855d62a..e8c57e039be8 100644 --- a/mm/sparse.c +++ b/mm/sparse.c @@ -102,7 +102,7 @@ static inline int sparse_index_init(unsigned long section_nr, int nid) #endif #ifdef CONFIG_SPARSEMEM_EXTREME -int __section_nr(struct mem_section* ms) +unsigned long __section_nr(struct mem_section *ms) { unsigned long root_nr; struct mem_section *root = NULL; @@ -121,9 +121,9 @@ int __section_nr(struct mem_section* ms) return (root_nr * SECTIONS_PER_ROOT) + (ms - root); } #else -int __section_nr(struct mem_section* ms) +unsigned long __section_nr(struct mem_section *ms) { - return (int)(ms - mem_section[0]); + return (unsigned long)(ms - mem_section[0]); } #endif @@ -178,10 +178,10 @@ void __meminit mminit_validate_memmodel_limits(unsigned long *start_pfn, * Keeping track of this gives us an easy way to break out of * those loops early. */ -int __highest_present_section_nr; +unsigned long __highest_present_section_nr; static void section_mark_present(struct mem_section *ms) { - int section_nr = __section_nr(ms); + unsigned long section_nr = __section_nr(ms); if (section_nr > __highest_present_section_nr) __highest_present_section_nr = section_nr; @@ -189,7 +189,7 @@ static void section_mark_present(struct mem_section *ms) ms->section_mem_map |= SECTION_MARKED_PRESENT; } -static inline int next_present_section_nr(int section_nr) +static inline unsigned long next_present_section_nr(unsigned long section_nr) { do { section_nr++;
We are using a mixture of "int" and "unsigned long". Let's make this consistent by using "unsigned long" everywhere. We'll do the same with memory block ids next. Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Cc: "Rafael J. Wysocki" <rafael@kernel.org> Cc: Andrew Morton <akpm@linux-foundation.org> Cc: Vlastimil Babka <vbabka@suse.cz> Cc: Michal Hocko <mhocko@suse.com> Cc: Dan Williams <dan.j.williams@intel.com> Cc: Mel Gorman <mgorman@techsingularity.net> Cc: Wei Yang <richard.weiyang@gmail.com> Cc: Johannes Weiner <hannes@cmpxchg.org> Cc: Arun KS <arunks@codeaurora.org> Cc: Pavel Tatashin <pasha.tatashin@oracle.com> Cc: Oscar Salvador <osalvador@suse.de> Cc: Stephen Rothwell <sfr@canb.auug.org.au> Cc: Mike Rapoport <rppt@linux.vnet.ibm.com> Cc: Baoquan He <bhe@redhat.com> Signed-off-by: David Hildenbrand <david@redhat.com> --- drivers/base/memory.c | 9 +++++---- include/linux/mmzone.h | 4 ++-- mm/sparse.c | 12 ++++++------ 3 files changed, 13 insertions(+), 12 deletions(-)