From: Christophe Leroy <christophe.leroy@c-s.fr> To: Andrew Morton <akpm@linux-foundation.org>, David Hildenbrand <david@redhat.com> 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: Sat, 15 Jun 2019 10:06:54 +0200 [thread overview] Message-ID: <701e8feb-cbf8-04c1-758c-046da9394ac1@c-s.fr> (raw) In-Reply-To: <20190614120036.00ae392e3f210e7bc9ec6960@linux-foundation.org> 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; > } > _ >
WARNING: multiple messages have this Message-ID (diff)
From: Christophe Leroy <christophe.leroy@c-s.fr> To: Andrew Morton <akpm@linux-foundation.org>, David Hildenbrand <david@redhat.com> Cc: Stephen Rothwell <sfr@canb.auug.org.au>, Michal Hocko <mhocko@suse.com>, Pavel Tatashin <pasha.tatashin@oracle.com>, linux-acpi@vger.kernel.org, Baoquan He <bhe@redhat.com>, "Rafael J. Wysocki" <rafael@kernel.org>, Greg Kroah-Hartman <gregkh@linuxfoundation.org>, linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org, Wei Yang <richard.weiyang@gmail.com>, linux-mm@kvack.org, Mike Rapoport <rppt@linux.vnet.ibm.com>, Arun KS <arunks@codeaurora.org>, Johannes Weiner <hannes@cmpxchg.org>, Dan Williams <dan.j.williams@intel.com>, Mel Gorman <mgorman@techsingularity.net>, 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: Sat, 15 Jun 2019 10:06:54 +0200 [thread overview] Message-ID: <701e8feb-cbf8-04c1-758c-046da9394ac1@c-s.fr> (raw) In-Reply-To: <20190614120036.00ae392e3f210e7bc9ec6960@linux-foundation.org> 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; > } > _ >
next prev parent reply other threads:[~2019-06-15 8:07 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 2019-06-14 19:34 ` David Hildenbrand 2019-06-15 8:06 ` Christophe Leroy [this message] 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=701e8feb-cbf8-04c1-758c-046da9394ac1@c-s.fr \ --to=christophe.leroy@c-s.fr \ --cc=akpm@linux-foundation.org \ --cc=arunks@codeaurora.org \ --cc=bhe@redhat.com \ --cc=dan.j.williams@intel.com \ --cc=david@redhat.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.