From: David Hildenbrand <david@redhat.com> To: Andrew Morton <akpm@linux-foundation.org> Cc: linux-kernel@vger.kernel.org, Dan Williams <dan.j.williams@intel.com>, linuxppc-dev@lists.ozlabs.org, linux-acpi@vger.kernel.org, linux-mm@kvack.org, Greg Kroah-Hartman <gregkh@linuxfoundation.org>, "Rafael J. Wysocki" <rafael@kernel.org>, Vlastimil Babka <vbabka@suse.cz>, Michal Hocko <mhocko@suse.com>, Mel Gorman <mgorman@techsingularity.net>, Wei Yang <richard.weiyang@gmail.com>, Johannes Weiner <hannes@cmpxchg.org>, Arun KS <arunks@codeaurora.org>, Pavel Tatashin <pasha.tatashin@oracle.com>, Oscar Salvador <osalvador@suse.de>, Stephen Rothwell <sfr@canb.auug.org.au>, Mike Rapoport <rppt@linux.vnet.ibm.com>, Baoquan He <bhe@redhat.com> Subject: Re: [PATCH v1 1/6] mm: Section numbers use the type "unsigned long" Date: Fri, 14 Jun 2019 21:34:59 +0200 [thread overview] Message-ID: <e07449fa-251f-51c7-9ee2-202635c4aef7@redhat.com> (raw) In-Reply-To: <20190614120036.00ae392e3f210e7bc9ec6960@linux-foundation.org> 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; > } > _ > -- Thanks, David / dhildenb
WARNING: multiple messages have this Message-ID (diff)
From: David Hildenbrand <david@redhat.com> To: Andrew Morton <akpm@linux-foundation.org> Cc: Stephen Rothwell <sfr@canb.auug.org.au>, Michal Hocko <mhocko@suse.com>, Mel Gorman <mgorman@techsingularity.net>, Baoquan He <bhe@redhat.com>, linux-mm@kvack.org, Greg Kroah-Hartman <gregkh@linuxfoundation.org>, "Rafael J. Wysocki" <rafael@kernel.org>, linux-kernel@vger.kernel.org, Wei Yang <richard.weiyang@gmail.com>, linux-acpi@vger.kernel.org, Mike Rapoport <rppt@linux.vnet.ibm.com>, Arun KS <arunks@codeaurora.org>, Johannes Weiner <hannes@cmpxchg.org>, Pavel Tatashin <pasha.tatashin@oracle.com>, Dan Williams <dan.j.williams@intel.com>, linuxppc-dev@lists.ozlabs.org, Vlastimil Babka <vbabka@suse.cz>, Oscar Salvador <osalvador@suse.de> Subject: Re: [PATCH v1 1/6] mm: Section numbers use the type "unsigned long" Date: Fri, 14 Jun 2019 21:34:59 +0200 [thread overview] Message-ID: <e07449fa-251f-51c7-9ee2-202635c4aef7@redhat.com> (raw) In-Reply-To: <20190614120036.00ae392e3f210e7bc9ec6960@linux-foundation.org> 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; > } > _ > -- Thanks, David / dhildenb
next prev parent reply other threads:[~2019-06-14 19:35 UTC|newest] Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-06-14 10:01 [PATCH v1 0/6] mm: Further memory block device cleanups David Hildenbrand 2019-06-14 10:01 ` David Hildenbrand 2019-06-14 10:01 ` [PATCH v1 1/6] mm: Section numbers use the type "unsigned long" David Hildenbrand 2019-06-14 10:01 ` David Hildenbrand 2019-06-14 19:00 ` Andrew Morton 2019-06-14 19:00 ` Andrew Morton 2019-06-14 19:34 ` David Hildenbrand [this message] 2019-06-14 19:34 ` David Hildenbrand 2019-06-15 8:06 ` Christophe Leroy 2019-06-15 8:06 ` Christophe Leroy 2019-06-18 1:57 ` Andrew Morton 2019-06-18 1:57 ` Andrew Morton 2019-06-18 12:17 ` Michael Ellerman 2019-06-18 12:17 ` Michael Ellerman 2019-06-14 10:01 ` [PATCH v1 2/6] drivers/base/memory: Use "unsigned long" for block ids David Hildenbrand 2019-06-14 10:01 ` David Hildenbrand 2019-06-14 10:01 ` [PATCH v1 3/6] mm: Make register_mem_sect_under_node() static David Hildenbrand 2019-06-14 10:01 ` David Hildenbrand 2019-06-14 10:01 ` [PATCH v1 4/6] mm/memory_hotplug: Rename walk_memory_range() and pass start+size instead of pfns David Hildenbrand 2019-06-14 10:01 ` David Hildenbrand 2019-06-14 10:01 ` [PATCH v1 5/6] mm/memory_hotplug: Move and simplify walk_memory_blocks() David Hildenbrand 2019-06-14 10:01 ` David Hildenbrand 2019-06-14 10:01 ` [PATCH v1 6/6] drivers/base/memory.c: Get rid of find_memory_block_hinted() David Hildenbrand 2019-06-14 10:01 ` David Hildenbrand
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=e07449fa-251f-51c7-9ee2-202635c4aef7@redhat.com \ --to=david@redhat.com \ --cc=akpm@linux-foundation.org \ --cc=arunks@codeaurora.org \ --cc=bhe@redhat.com \ --cc=dan.j.williams@intel.com \ --cc=gregkh@linuxfoundation.org \ --cc=hannes@cmpxchg.org \ --cc=linux-acpi@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-mm@kvack.org \ --cc=linuxppc-dev@lists.ozlabs.org \ --cc=mgorman@techsingularity.net \ --cc=mhocko@suse.com \ --cc=osalvador@suse.de \ --cc=pasha.tatashin@oracle.com \ --cc=rafael@kernel.org \ --cc=richard.weiyang@gmail.com \ --cc=rppt@linux.vnet.ibm.com \ --cc=sfr@canb.auug.org.au \ --cc=vbabka@suse.cz \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
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.