The code comment above sparse_add_one_section() is obsolete and
incorrect, clean it up and write new one.
Signed-off-by: Baoquan He <[email protected]>
---
v2->v3:
Normalize the code comment to use '/**' at 1st line of doc
above function.
v1-v2:
Add comments to explain what the returned value means for
each error code.
mm/sparse.c | 17 +++++++++++++----
1 file changed, 13 insertions(+), 4 deletions(-)
diff --git a/mm/sparse.c b/mm/sparse.c
index 69904aa6165b..363f9d31b511 100644
--- a/mm/sparse.c
+++ b/mm/sparse.c
@@ -684,10 +684,19 @@ static void free_map_bootmem(struct page *memmap)
#endif /* CONFIG_MEMORY_HOTREMOVE */
#endif /* CONFIG_SPARSEMEM_VMEMMAP */
-/*
- * returns the number of sections whose mem_maps were properly
- * set. If this is <=0, then that means that the passed-in
- * map was not consumed and must be freed.
+/**
+ * sparse_add_one_section - add a memory section
+ * @nid: The node to add section on
+ * @start_pfn: start pfn of the memory range
+ * @altmap: device page map
+ *
+ * This is only intended for hotplug.
+ *
+ * Returns:
+ * 0 on success.
+ * Other error code on failure:
+ * - -EEXIST - section has been present.
+ * - -ENOMEM - out of memory.
*/
int __meminit sparse_add_one_section(int nid, unsigned long start_pfn,
struct vmem_altmap *altmap)
--
2.17.2
The input parameter 'phys_index' of memory_block_action() is actually
the section number, but not the phys_index of memory_block. Fix it.
Signed-off-by: Baoquan He <[email protected]>
---
v2->v3:
Rename the parameter to 'start_section_nr' from 'sec'.
drivers/base/memory.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/base/memory.c b/drivers/base/memory.c
index cb8347500ce2..9ea972b2ae79 100644
--- a/drivers/base/memory.c
+++ b/drivers/base/memory.c
@@ -231,13 +231,14 @@ static bool pages_correctly_probed(unsigned long start_pfn)
* OK to have direct references to sparsemem variables in here.
*/
static int
-memory_block_action(unsigned long phys_index, unsigned long action, int online_type)
+memory_block_action(unsigned long start_section_nr, unsigned long action,
+ int online_type)
{
unsigned long start_pfn;
unsigned long nr_pages = PAGES_PER_SECTION * sections_per_block;
int ret;
- start_pfn = section_nr_to_pfn(phys_index);
+ start_pfn = section_nr_to_pfn(start_section_nr);
switch (action) {
case MEM_ONLINE:
@@ -251,7 +252,7 @@ memory_block_action(unsigned long phys_index, unsigned long action, int online_t
break;
default:
WARN(1, KERN_WARNING "%s(%ld, %ld) unknown action: "
- "%ld\n", __func__, phys_index, action, action);
+ "%ld\n", __func__, start_section_nr, action, action);
ret = -EINVAL;
}
--
2.17.2
On Fri 29-03-19 16:29:15, Baoquan He wrote:
> The input parameter 'phys_index' of memory_block_action() is actually
> the section number, but not the phys_index of memory_block. Fix it.
I have tried to explain that the naming is mostly a relict from the past
than really a misleading name http://lkml.kernel.org/r/[email protected]
Maybe it would be good to reflect that in the changelog
> Signed-off-by: Baoquan He <[email protected]>
btw. I've acked the previous version as well.
> ---
> v2->v3:
> Rename the parameter to 'start_section_nr' from 'sec'.
>
> drivers/base/memory.c | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/base/memory.c b/drivers/base/memory.c
> index cb8347500ce2..9ea972b2ae79 100644
> --- a/drivers/base/memory.c
> +++ b/drivers/base/memory.c
> @@ -231,13 +231,14 @@ static bool pages_correctly_probed(unsigned long start_pfn)
> * OK to have direct references to sparsemem variables in here.
> */
> static int
> -memory_block_action(unsigned long phys_index, unsigned long action, int online_type)
> +memory_block_action(unsigned long start_section_nr, unsigned long action,
> + int online_type)
> {
> unsigned long start_pfn;
> unsigned long nr_pages = PAGES_PER_SECTION * sections_per_block;
> int ret;
>
> - start_pfn = section_nr_to_pfn(phys_index);
> + start_pfn = section_nr_to_pfn(start_section_nr);
>
> switch (action) {
> case MEM_ONLINE:
> @@ -251,7 +252,7 @@ memory_block_action(unsigned long phys_index, unsigned long action, int online_t
> break;
> default:
> WARN(1, KERN_WARNING "%s(%ld, %ld) unknown action: "
> - "%ld\n", __func__, phys_index, action, action);
> + "%ld\n", __func__, start_section_nr, action, action);
> ret = -EINVAL;
> }
>
> --
> 2.17.2
>
--
Michal Hocko
SUSE Labs
On Fri 29-03-19 16:29:14, Baoquan He wrote:
> The code comment above sparse_add_one_section() is obsolete and
> incorrect, clean it up and write new one.
>
> Signed-off-by: Baoquan He <[email protected]>
Acked-by: Michal Hocko <[email protected]>
> ---
> v2->v3:
> Normalize the code comment to use '/**' at 1st line of doc
> above function.
> v1-v2:
> Add comments to explain what the returned value means for
> each error code.
> mm/sparse.c | 17 +++++++++++++----
> 1 file changed, 13 insertions(+), 4 deletions(-)
>
> diff --git a/mm/sparse.c b/mm/sparse.c
> index 69904aa6165b..363f9d31b511 100644
> --- a/mm/sparse.c
> +++ b/mm/sparse.c
> @@ -684,10 +684,19 @@ static void free_map_bootmem(struct page *memmap)
> #endif /* CONFIG_MEMORY_HOTREMOVE */
> #endif /* CONFIG_SPARSEMEM_VMEMMAP */
>
> -/*
> - * returns the number of sections whose mem_maps were properly
> - * set. If this is <=0, then that means that the passed-in
> - * map was not consumed and must be freed.
> +/**
> + * sparse_add_one_section - add a memory section
> + * @nid: The node to add section on
> + * @start_pfn: start pfn of the memory range
> + * @altmap: device page map
> + *
> + * This is only intended for hotplug.
> + *
> + * Returns:
> + * 0 on success.
> + * Other error code on failure:
> + * - -EEXIST - section has been present.
> + * - -ENOMEM - out of memory.
> */
> int __meminit sparse_add_one_section(int nid, unsigned long start_pfn,
> struct vmem_altmap *altmap)
> --
> 2.17.2
>
--
Michal Hocko
SUSE Labs
On 03/29/19 at 10:13am, Michal Hocko wrote:
> On Fri 29-03-19 16:29:15, Baoquan He wrote:
> > The input parameter 'phys_index' of memory_block_action() is actually
> > the section number, but not the phys_index of memory_block. Fix it.
>
> I have tried to explain that the naming is mostly a relict from the past
> than really a misleading name http://lkml.kernel.org/r/[email protected]
> Maybe it would be good to reflect that in the changelog
>
> > Signed-off-by: Baoquan He <[email protected]>
>
> btw. I've acked the previous version as well.
Sure, will rewrite the log and add people's Acked-by tag. Thanks.
>
> > ---
> > v2->v3:
> > Rename the parameter to 'start_section_nr' from 'sec'.
> >
> > drivers/base/memory.c | 7 ++++---
> > 1 file changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/base/memory.c b/drivers/base/memory.c
> > index cb8347500ce2..9ea972b2ae79 100644
> > --- a/drivers/base/memory.c
> > +++ b/drivers/base/memory.c
> > @@ -231,13 +231,14 @@ static bool pages_correctly_probed(unsigned long start_pfn)
> > * OK to have direct references to sparsemem variables in here.
> > */
> > static int
> > -memory_block_action(unsigned long phys_index, unsigned long action, int online_type)
> > +memory_block_action(unsigned long start_section_nr, unsigned long action,
> > + int online_type)
> > {
> > unsigned long start_pfn;
> > unsigned long nr_pages = PAGES_PER_SECTION * sections_per_block;
> > int ret;
> >
> > - start_pfn = section_nr_to_pfn(phys_index);
> > + start_pfn = section_nr_to_pfn(start_section_nr);
> >
> > switch (action) {
> > case MEM_ONLINE:
> > @@ -251,7 +252,7 @@ memory_block_action(unsigned long phys_index, unsigned long action, int online_t
> > break;
> > default:
> > WARN(1, KERN_WARNING "%s(%ld, %ld) unknown action: "
> > - "%ld\n", __func__, phys_index, action, action);
> > + "%ld\n", __func__, start_section_nr, action, action);
> > ret = -EINVAL;
> > }
> >
> > --
> > 2.17.2
> >
>
> --
> Michal Hocko
> SUSE Labs
The input parameter 'phys_index' of memory_block_action() is actually
the section number, but not the phys_index of memory_block. This is
a relict from the past when one memory block could only contain one
section.
Rename it to start_section_nr.
Signed-off-by: Baoquan He <[email protected]>
Acked-by: Michal Hocko <[email protected]>
Reviewed-by: Rafael J. Wysocki <[email protected]>
---
drivers/base/memory.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/base/memory.c b/drivers/base/memory.c
index cb8347500ce2..9ea972b2ae79 100644
--- a/drivers/base/memory.c
+++ b/drivers/base/memory.c
@@ -231,13 +231,14 @@ static bool pages_correctly_probed(unsigned long start_pfn)
* OK to have direct references to sparsemem variables in here.
*/
static int
-memory_block_action(unsigned long phys_index, unsigned long action, int online_type)
+memory_block_action(unsigned long start_section_nr, unsigned long action,
+ int online_type)
{
unsigned long start_pfn;
unsigned long nr_pages = PAGES_PER_SECTION * sections_per_block;
int ret;
- start_pfn = section_nr_to_pfn(phys_index);
+ start_pfn = section_nr_to_pfn(start_section_nr);
switch (action) {
case MEM_ONLINE:
@@ -251,7 +252,7 @@ memory_block_action(unsigned long phys_index, unsigned long action, int online_t
break;
default:
WARN(1, KERN_WARNING "%s(%ld, %ld) unknown action: "
- "%ld\n", __func__, phys_index, action, action);
+ "%ld\n", __func__, start_section_nr, action, action);
ret = -EINVAL;
}
--
2.17.2
On Fri, Mar 29, 2019 at 10:13:25AM +0100, Michal Hocko wrote:
> On Fri 29-03-19 16:29:15, Baoquan He wrote:
> > The input parameter 'phys_index' of memory_block_action() is actually
> > the section number, but not the phys_index of memory_block. Fix it.
>
> I have tried to explain that the naming is mostly a relict from the past
> than really a misleading name http://lkml.kernel.org/r/[email protected]
> Maybe it would be good to reflect that in the changelog
I think that phys_device variable in remove_memory_section() is also a relict
from the past, and it is no longer used.
Neither node_id variable is used.
Actually, unregister_memory_section() sets those two to 0 no matter what.
Since we are cleaning up, I wonder if we can go a bit further and we can get
rid of that as well.
--
Oscar Salvador
SUSE L3
On Fri, Mar 29, 2019 at 05:36:59PM +0800, Baoquan He wrote:
> The input parameter 'phys_index' of memory_block_action() is actually
> the section number, but not the phys_index of memory_block. This is
> a relict from the past when one memory block could only contain one
> section.
>
> Rename it to start_section_nr.
>
> Signed-off-by: Baoquan He <[email protected]>
> Acked-by: Michal Hocko <[email protected]>
> Reviewed-by: Rafael J. Wysocki <[email protected]>
Reviewed-by: Oscar Salvador <[email protected]>
> ---
> drivers/base/memory.c | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/base/memory.c b/drivers/base/memory.c
> index cb8347500ce2..9ea972b2ae79 100644
> --- a/drivers/base/memory.c
> +++ b/drivers/base/memory.c
> @@ -231,13 +231,14 @@ static bool pages_correctly_probed(unsigned long start_pfn)
> * OK to have direct references to sparsemem variables in here.
> */
> static int
> -memory_block_action(unsigned long phys_index, unsigned long action, int online_type)
> +memory_block_action(unsigned long start_section_nr, unsigned long action,
> + int online_type)
> {
> unsigned long start_pfn;
> unsigned long nr_pages = PAGES_PER_SECTION * sections_per_block;
> int ret;
>
> - start_pfn = section_nr_to_pfn(phys_index);
> + start_pfn = section_nr_to_pfn(start_section_nr);
>
> switch (action) {
> case MEM_ONLINE:
> @@ -251,7 +252,7 @@ memory_block_action(unsigned long phys_index, unsigned long action, int online_t
> break;
> default:
> WARN(1, KERN_WARNING "%s(%ld, %ld) unknown action: "
> - "%ld\n", __func__, phys_index, action, action);
> + "%ld\n", __func__, start_section_nr, action, action);
> ret = -EINVAL;
> }
>
> --
> 2.17.2
>
--
Oscar Salvador
SUSE L3
On Fri, Mar 29, 2019 at 04:29:14PM +0800, Baoquan He wrote:
> The code comment above sparse_add_one_section() is obsolete and
> incorrect, clean it up and write new one.
>
> Signed-off-by: Baoquan He <[email protected]>
Reviewed-by: Oscar Salvador <[email protected]>
> ---
> v2->v3:
> Normalize the code comment to use '/**' at 1st line of doc
> above function.
> v1-v2:
> Add comments to explain what the returned value means for
> each error code.
> mm/sparse.c | 17 +++++++++++++----
> 1 file changed, 13 insertions(+), 4 deletions(-)
>
> diff --git a/mm/sparse.c b/mm/sparse.c
> index 69904aa6165b..363f9d31b511 100644
> --- a/mm/sparse.c
> +++ b/mm/sparse.c
> @@ -684,10 +684,19 @@ static void free_map_bootmem(struct page *memmap)
> #endif /* CONFIG_MEMORY_HOTREMOVE */
> #endif /* CONFIG_SPARSEMEM_VMEMMAP */
>
> -/*
> - * returns the number of sections whose mem_maps were properly
> - * set. If this is <=0, then that means that the passed-in
> - * map was not consumed and must be freed.
> +/**
> + * sparse_add_one_section - add a memory section
> + * @nid: The node to add section on
> + * @start_pfn: start pfn of the memory range
> + * @altmap: device page map
> + *
> + * This is only intended for hotplug.
> + *
> + * Returns:
> + * 0 on success.
> + * Other error code on failure:
> + * - -EEXIST - section has been present.
> + * - -ENOMEM - out of memory.
I am not really into kernel-doc format, but I thought it was something like:
<--
Return:
0: success
-EEXIST: Section is already present
-ENOMEM: Out of memory
-->
But as I said, I might very well be wrong.
--
Oscar Salvador
SUSE L3
On 3/29/2019 1:59 PM, Baoquan He wrote:
> The code comment above sparse_add_one_section() is obsolete and
> incorrect, clean it up and write new one.
>
> Signed-off-by: Baoquan He <[email protected]>
Reviewed-by: Mukesh Ojha <[email protected]>
Cheers,
-Mukesh
> ---
> v2->v3:
> Normalize the code comment to use '/**' at 1st line of doc
> above function.
> v1-v2:
> Add comments to explain what the returned value means for
> each error code.
> mm/sparse.c | 17 +++++++++++++----
> 1 file changed, 13 insertions(+), 4 deletions(-)
>
> diff --git a/mm/sparse.c b/mm/sparse.c
> index 69904aa6165b..363f9d31b511 100644
> --- a/mm/sparse.c
> +++ b/mm/sparse.c
> @@ -684,10 +684,19 @@ static void free_map_bootmem(struct page *memmap)
> #endif /* CONFIG_MEMORY_HOTREMOVE */
> #endif /* CONFIG_SPARSEMEM_VMEMMAP */
>
> -/*
> - * returns the number of sections whose mem_maps were properly
> - * set. If this is <=0, then that means that the passed-in
> - * map was not consumed and must be freed.
> +/**
> + * sparse_add_one_section - add a memory section
> + * @nid: The node to add section on
> + * @start_pfn: start pfn of the memory range
> + * @altmap: device page map
> + *
> + * This is only intended for hotplug.
> + *
> + * Returns:
> + * 0 on success.
> + * Other error code on failure:
> + * - -EEXIST - section has been present.
> + * - -ENOMEM - out of memory.
> */
> int __meminit sparse_add_one_section(int nid, unsigned long start_pfn,
> struct vmem_altmap *altmap)
On 3/29/2019 3:06 PM, Baoquan He wrote:
> The input parameter 'phys_index' of memory_block_action() is actually
> the section number, but not the phys_index of memory_block. This is
> a relict from the past when one memory block could only contain one
> section.
>
> Rename it to start_section_nr.
>
> Signed-off-by: Baoquan He <[email protected]>
> Acked-by: Michal Hocko <[email protected]>
> Reviewed-by: Rafael J. Wysocki <[email protected]>
Reviewed-by: Mukesh Ojha <[email protected]>
Cheers,
-Mukesh
> ---
> drivers/base/memory.c | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/base/memory.c b/drivers/base/memory.c
> index cb8347500ce2..9ea972b2ae79 100644
> --- a/drivers/base/memory.c
> +++ b/drivers/base/memory.c
> @@ -231,13 +231,14 @@ static bool pages_correctly_probed(unsigned long start_pfn)
> * OK to have direct references to sparsemem variables in here.
> */
> static int
> -memory_block_action(unsigned long phys_index, unsigned long action, int online_type)
> +memory_block_action(unsigned long start_section_nr, unsigned long action,
> + int online_type)
> {
> unsigned long start_pfn;
> unsigned long nr_pages = PAGES_PER_SECTION * sections_per_block;
> int ret;
>
> - start_pfn = section_nr_to_pfn(phys_index);
> + start_pfn = section_nr_to_pfn(start_section_nr);
>
> switch (action) {
> case MEM_ONLINE:
> @@ -251,7 +252,7 @@ memory_block_action(unsigned long phys_index, unsigned long action, int online_t
> break;
> default:
> WARN(1, KERN_WARNING "%s(%ld, %ld) unknown action: "
> - "%ld\n", __func__, phys_index, action, action);
> + "%ld\n", __func__, start_section_nr, action, action);
> ret = -EINVAL;
> }
>
On 03/29/19 at 10:37am, Oscar Salvador wrote:
> On Fri, Mar 29, 2019 at 10:13:25AM +0100, Michal Hocko wrote:
> > On Fri 29-03-19 16:29:15, Baoquan He wrote:
> > > The input parameter 'phys_index' of memory_block_action() is actually
> > > the section number, but not the phys_index of memory_block. Fix it.
> >
> > I have tried to explain that the naming is mostly a relict from the past
> > than really a misleading name http://lkml.kernel.org/r/[email protected]
> > Maybe it would be good to reflect that in the changelog
>
> I think that phys_device variable in remove_memory_section() is also a relict
> from the past, and it is no longer used.
> Neither node_id variable is used.
> Actually, unregister_memory_section() sets those two to 0 no matter what.
>
> Since we are cleaning up, I wonder if we can go a bit further and we can get
> rid of that as well.
Yes, certainly. I would like to post a new one to carry this.
On 03/29/19 at 11:36am, Oscar Salvador wrote:
> > +/**
> > + * sparse_add_one_section - add a memory section
> > + * @nid: The node to add section on
> > + * @start_pfn: start pfn of the memory range
> > + * @altmap: device page map
> > + *
> > + * This is only intended for hotplug.
> > + *
> > + * Returns:
> > + * 0 on success.
> > + * Other error code on failure:
> > + * - -EEXIST - section has been present.
> > + * - -ENOMEM - out of memory.
>
> I am not really into kernel-doc format, but I thought it was something like:
>
> <--
> Return:
> 0: success
> -EEXIST: Section is already present
> -ENOMEM: Out of memory
> -->
>
> But as I said, I might very well be wrong.
Below is excerpt from doc-guide/kernel-doc.rst. Seems they suggest it
like this if format returned values with multi-line style. While the
format is not strictly defined. I will use it to update.
*Return:
* * 0 - Success
* * -EEXIST - Section is already present
* * -ENOMEM - Out of memory
The return value, if any, should be described in a dedicated section
named ``Return``.
.. note::
#) The multi-line descriptive text you provide does *not* recognize
line breaks, so if you try to format some text nicely, as in::
* Return:
* 0 - OK
* -EINVAL - invalid argument
* -ENOMEM - out of memory
this will all run together and produce::
Return: 0 - OK -EINVAL - invalid argument -ENOMEM - out of memory
So, in order to produce the desired line breaks, you need to use a
ReST list, e. g.::
* Return:
* * 0 - OK to runtime suspend the device
* * -EBUSY - Device should not be runtime suspended
On Fri, Mar 29, 2019 at 04:29:14PM +0800, Baoquan He wrote:
> The code comment above sparse_add_one_section() is obsolete and
> incorrect, clean it up and write new one.
>
> Signed-off-by: Baoquan He <[email protected]>
Reviewed-by: Mike Rapoport <[email protected]>
> ---
> v2->v3:
> Normalize the code comment to use '/**' at 1st line of doc
> above function.
> v1-v2:
> Add comments to explain what the returned value means for
> each error code.
> mm/sparse.c | 17 +++++++++++++----
> 1 file changed, 13 insertions(+), 4 deletions(-)
>
> diff --git a/mm/sparse.c b/mm/sparse.c
> index 69904aa6165b..363f9d31b511 100644
> --- a/mm/sparse.c
> +++ b/mm/sparse.c
> @@ -684,10 +684,19 @@ static void free_map_bootmem(struct page *memmap)
> #endif /* CONFIG_MEMORY_HOTREMOVE */
> #endif /* CONFIG_SPARSEMEM_VMEMMAP */
>
> -/*
> - * returns the number of sections whose mem_maps were properly
> - * set. If this is <=0, then that means that the passed-in
> - * map was not consumed and must be freed.
> +/**
> + * sparse_add_one_section - add a memory section
> + * @nid: The node to add section on
> + * @start_pfn: start pfn of the memory range
> + * @altmap: device page map
> + *
> + * This is only intended for hotplug.
> + *
> + * Returns:
> + * 0 on success.
> + * Other error code on failure:
> + * - -EEXIST - section has been present.
> + * - -ENOMEM - out of memory.
> */
> int __meminit sparse_add_one_section(int nid, unsigned long start_pfn,
> struct vmem_altmap *altmap)
> --
> 2.17.2
>
--
Sincerely yours,
Mike.