[PATCH 1/3] mm/sparse: Clean up the obsolete code comment

[PATCH 1/3] mm/sparse: Clean up the obsolete code comment

[Baoquan He]

The code comment above sparse_add_one_section() is obsolete and
incorrect, clean it up and write new one.

Signed-off-by: Baoquan He <bhe@redhat.com>
---
 mm/sparse.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/mm/sparse.c b/mm/sparse.c
index 77a0554fa5bd..0a0f82c5d969 100644
--- a/mm/sparse.c
+++ b/mm/sparse.c
@@ -674,9 +674,12 @@ static void free_map_bootmem(struct page *memmap)
 #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
+ *
+ * Return 0 on success and an appropriate error code otherwise.
  */
 int __meminit sparse_add_one_section(int nid, unsigned long start_pfn,
 				     struct vmem_altmap *altmap)
-- 
2.17.2

[PATCH 2/3] mm/sparse: Optimize sparse_add_one_section()

[Baoquan He]

Reorder the allocation of usemap and memmap since usemap allocation
is much smaller and simpler. Otherwise hard work is done to make
memmap ready, then have to rollback just because of usemap allocation
failure.

Signed-off-by: Baoquan He <bhe@redhat.com>
---
 mm/sparse.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/mm/sparse.c b/mm/sparse.c
index 0a0f82c5d969..054b99f74181 100644
--- a/mm/sparse.c
+++ b/mm/sparse.c
@@ -697,16 +697,17 @@ int __meminit sparse_add_one_section(int nid, unsigned long start_pfn,
 	ret = sparse_index_init(section_nr, nid);
 	if (ret < 0 && ret != -EEXIST)
 		return ret;
-	ret = 0;
-	memmap = kmalloc_section_memmap(section_nr, nid, altmap);
-	if (!memmap)
-		return -ENOMEM;
+
 	usemap = __kmalloc_section_usemap();
-	if (!usemap) {
-		__kfree_section_memmap(memmap, altmap);
+	if (!usemap)
+		return -ENOMEM;
+	memmap = kmalloc_section_memmap(section_nr, nid, altmap);
+	if (!memmap) {
+		kfree(usemap);
 		return -ENOMEM;
 	}
 
+	ret = 0;
 	ms = __pfn_to_section(start_pfn);
 	if (ms->section_mem_map & SECTION_MARKED_PRESENT) {
 		ret = -EEXIST;
-- 
2.17.2

Re: [PATCH 2/3] mm/sparse: Optimize sparse_add_one_section()

[Mike Rapoport]

On Wed, Mar 20, 2019 at 03:35:39PM +0800, Baoquan He wrote:
> Reorder the allocation of usemap and memmap since usemap allocation
> is much smaller and simpler. Otherwise hard work is done to make
> memmap ready, then have to rollback just because of usemap allocation
> failure.
> 
> Signed-off-by: Baoquan He <bhe@redhat.com>
> ---
>  mm/sparse.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/mm/sparse.c b/mm/sparse.c
> index 0a0f82c5d969..054b99f74181 100644
> --- a/mm/sparse.c
> +++ b/mm/sparse.c
> @@ -697,16 +697,17 @@ int __meminit sparse_add_one_section(int nid, unsigned long start_pfn,
>  	ret = sparse_index_init(section_nr, nid);
>  	if (ret < 0 && ret != -EEXIST)
>  		return ret;
> -	ret = 0;
> -	memmap = kmalloc_section_memmap(section_nr, nid, altmap);
> -	if (!memmap)
> -		return -ENOMEM;
> +
>  	usemap = __kmalloc_section_usemap();
> -	if (!usemap) {
> -		__kfree_section_memmap(memmap, altmap);
> +	if (!usemap)
> +		return -ENOMEM;
> +	memmap = kmalloc_section_memmap(section_nr, nid, altmap);
> +	if (!memmap) {
> +		kfree(usemap);

If you are anyway changing this why not to switch to goto's for error
handling?

>  		return -ENOMEM;
>  	}
> 
> +	ret = 0;
>  	ms = __pfn_to_section(start_pfn);
>  	if (ms->section_mem_map & SECTION_MARKED_PRESENT) {
>  		ret = -EEXIST;
> -- 
> 2.17.2
> 

-- 
Sincerely yours,
Mike.

Re: [PATCH 2/3] mm/sparse: Optimize sparse_add_one_section()

[Baoquan He]

On 03/20/19 at 09:56am, Mike Rapoport wrote:
> > diff --git a/mm/sparse.c b/mm/sparse.c
> > index 0a0f82c5d969..054b99f74181 100644
> > --- a/mm/sparse.c
> > +++ b/mm/sparse.c
> > @@ -697,16 +697,17 @@ int __meminit sparse_add_one_section(int nid, unsigned long start_pfn,
> >  	ret = sparse_index_init(section_nr, nid);
> >  	if (ret < 0 && ret != -EEXIST)
> >  		return ret;
> > -	ret = 0;
> > -	memmap = kmalloc_section_memmap(section_nr, nid, altmap);
> > -	if (!memmap)
> > -		return -ENOMEM;
> > +
> >  	usemap = __kmalloc_section_usemap();
> > -	if (!usemap) {
> > -		__kfree_section_memmap(memmap, altmap);
> > +	if (!usemap)
> > +		return -ENOMEM;
> > +	memmap = kmalloc_section_memmap(section_nr, nid, altmap);
> > +	if (!memmap) {
> > +		kfree(usemap);
> 
> If you are anyway changing this why not to switch to goto's for error
> handling?

OK, I am fine to switch to 'goto'. Will update and repost. Thanks.

> 
> >  		return -ENOMEM;
> >  	}
> > 
> > +	ret = 0;
> >  	ms = __pfn_to_section(start_pfn);
> >  	if (ms->section_mem_map & SECTION_MARKED_PRESENT) {
> >  		ret = -EEXIST;
> > -- 
> > 2.17.2
> > 
> 
> -- 
> Sincerely yours,
> Mike.
> 

Re: [PATCH 2/3] mm/sparse: Optimize sparse_add_one_section()

[Baoquan He]

Hi Mike,

On 03/20/19 at 09:56am, Mike Rapoport wrote:
 > @@ -697,16 +697,17 @@ int __meminit sparse_add_one_section(int nid, unsigned long start_pfn,
> >  	ret = sparse_index_init(section_nr, nid);
> >  	if (ret < 0 && ret != -EEXIST)
> >  		return ret;
> > -	ret = 0;
> > -	memmap = kmalloc_section_memmap(section_nr, nid, altmap);
> > -	if (!memmap)
> > -		return -ENOMEM;
> > +
> >  	usemap = __kmalloc_section_usemap();
> > -	if (!usemap) {
> > -		__kfree_section_memmap(memmap, altmap);
> > +	if (!usemap)
> > +		return -ENOMEM;
> > +	memmap = kmalloc_section_memmap(section_nr, nid, altmap);
> > +	if (!memmap) {
> > +		kfree(usemap);
> 
> If you are anyway changing this why not to switch to goto's for error
> handling?

I update code change as below, could you check if it's OK to you?

Thanks
Baoquan

From 39b679b6f34f6acbc05351be8569d23bae3c0458 Mon Sep 17 00:00:00 2001
From: Baoquan He <bhe@redhat.com>
Date: Fri, 15 Mar 2019 16:03:52 +0800
Subject: [PATCH] mm/sparse: Optimize sparse_add_one_section()

Reorder the allocation of usemap and memmap since usemap allocation
is much smaller and simpler. Otherwise hard work is done to make
memmap ready, then have to rollback just because of usemap allocation
failure.

Meanwhile update the error handler to cover usemap allocation failure
too.

Signed-off-by: Baoquan He <bhe@redhat.com>
---
 mm/sparse.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/mm/sparse.c b/mm/sparse.c
index a99e0b253927..0e842b924be6 100644
--- a/mm/sparse.c
+++ b/mm/sparse.c
@@ -699,20 +699,21 @@ int __meminit sparse_add_one_section(int nid, unsigned long start_pfn,
 	ret = sparse_index_init(section_nr, nid);
 	if (ret < 0 && ret != -EEXIST)
 		return ret;
-	ret = 0;
-	memmap = kmalloc_section_memmap(section_nr, nid, altmap);
-	if (!memmap)
-		return -ENOMEM;
+
 	usemap = __kmalloc_section_usemap();
-	if (!usemap) {
-		__kfree_section_memmap(memmap, altmap);
+	if (!usemap)
 		return -ENOMEM;
+	memmap = kmalloc_section_memmap(section_nr, nid, altmap);
+	if (!memmap) {
+		ret = -ENOMEM;
+		goto out2;
 	}
 
+	ret = 0;
 	ms = __pfn_to_section(start_pfn);
 	if (ms->section_mem_map & SECTION_MARKED_PRESENT) {
 		ret = -EEXIST;
-		goto out;
+		goto out2;
 	}
 
 	/*
@@ -724,11 +725,11 @@ int __meminit sparse_add_one_section(int nid, unsigned long start_pfn,
 	section_mark_present(ms);
 	sparse_init_one_section(ms, section_nr, memmap, usemap);
 
+	return ret;
 out:
-	if (ret < 0) {
-		kfree(usemap);
-		__kfree_section_memmap(memmap, altmap);
-	}
+	__kfree_section_memmap(memmap, altmap);
+out2:
+	kfree(usemap);
 	return ret;
 }
 
-- 
2.17.2

Re: [PATCH 2/3] mm/sparse: Optimize sparse_add_one_section()

[Matthew Wilcox]

On Wed, Mar 20, 2019 at 06:13:18PM +0800, Baoquan He wrote:
> +	if (!memmap) {
> +		ret = -ENOMEM;
> +		goto out2;

Documentation/process/coding-style:

Choose label names which say what the goto does or why the goto exists.  An
example of a good name could be ``out_free_buffer:`` if the goto frees ``buffer``.
Avoid using GW-BASIC names like ``err1:`` and ``err2:``, as you would have to
renumber them if you ever add or remove exit paths, and they make correctness
difficult to verify anyway.

Re: [PATCH 2/3] mm/sparse: Optimize sparse_add_one_section()

[Mike Rapoport]

On Wed, Mar 20, 2019 at 06:13:18PM +0800, Baoquan He wrote:
> Hi Mike,
> 
> On 03/20/19 at 09:56am, Mike Rapoport wrote:
>  > @@ -697,16 +697,17 @@ int __meminit sparse_add_one_section(int nid, unsigned long start_pfn,
> > >  	ret = sparse_index_init(section_nr, nid);
> > >  	if (ret < 0 && ret != -EEXIST)
> > >  		return ret;
> > > -	ret = 0;
> > > -	memmap = kmalloc_section_memmap(section_nr, nid, altmap);
> > > -	if (!memmap)
> > > -		return -ENOMEM;
> > > +
> > >  	usemap = __kmalloc_section_usemap();
> > > -	if (!usemap) {
> > > -		__kfree_section_memmap(memmap, altmap);
> > > +	if (!usemap)
> > > +		return -ENOMEM;
> > > +	memmap = kmalloc_section_memmap(section_nr, nid, altmap);
> > > +	if (!memmap) {
> > > +		kfree(usemap);
> > 
> > If you are anyway changing this why not to switch to goto's for error
> > handling?
> 
> I update code change as below, could you check if it's OK to you?
> 
> Thanks
> Baoquan
> 
> From 39b679b6f34f6acbc05351be8569d23bae3c0458 Mon Sep 17 00:00:00 2001
> From: Baoquan He <bhe@redhat.com>
> Date: Fri, 15 Mar 2019 16:03:52 +0800
> Subject: [PATCH] mm/sparse: Optimize sparse_add_one_section()
> 
> Reorder the allocation of usemap and memmap since usemap allocation
> is much smaller and simpler. Otherwise hard work is done to make
> memmap ready, then have to rollback just because of usemap allocation
> failure.
> 
> Meanwhile update the error handler to cover usemap allocation failure
> too.
> 
> Signed-off-by: Baoquan He <bhe@redhat.com>
> ---
>  mm/sparse.c | 23 ++++++++++++-----------
>  1 file changed, 12 insertions(+), 11 deletions(-)
> 
> diff --git a/mm/sparse.c b/mm/sparse.c
> index a99e0b253927..0e842b924be6 100644
> --- a/mm/sparse.c
> +++ b/mm/sparse.c
> @@ -699,20 +699,21 @@ int __meminit sparse_add_one_section(int nid, unsigned long start_pfn,
>  	ret = sparse_index_init(section_nr, nid);
>  	if (ret < 0 && ret != -EEXIST)
>  		return ret;
> -	ret = 0;
> -	memmap = kmalloc_section_memmap(section_nr, nid, altmap);
> -	if (!memmap)
> -		return -ENOMEM;
> +
>  	usemap = __kmalloc_section_usemap();
> -	if (!usemap) {
> -		__kfree_section_memmap(memmap, altmap);
> +	if (!usemap)
>  		return -ENOMEM;
> +	memmap = kmalloc_section_memmap(section_nr, nid, altmap);
> +	if (!memmap) {
> +		ret = -ENOMEM;
> +		goto out2;

I'd name the label out_free_usemap.

>  	}
>  
> +	ret = 0;
>  	ms = __pfn_to_section(start_pfn);
>  	if (ms->section_mem_map & SECTION_MARKED_PRESENT) {
>  		ret = -EEXIST;
> -		goto out;
> +		goto out2;
>  	}

I've missed this previously, but it seems that this check can be moved
before the allocations, which simplifies the code a bit more.

>  
>  	/*
> @@ -724,11 +725,11 @@ int __meminit sparse_add_one_section(int nid, unsigned long start_pfn,
>  	section_mark_present(ms);
>  	sparse_init_one_section(ms, section_nr, memmap, usemap);
>  
> +	return ret;
>  out:
> -	if (ret < 0) {
> -		kfree(usemap);
> -		__kfree_section_memmap(memmap, altmap);
> -	}
> +	__kfree_section_memmap(memmap, altmap);
> +out2:
> +	kfree(usemap);
>  	return ret;
>  }
>  
> -- 
> 2.17.2
> 

-- 
Sincerely yours,
Mike.

[PATCH 3/3] mm/sparse: Rename function related to section memmap allocation/free

[Baoquan He]

These functions are used allocate/free section memmap, have nothing
to do with kmalloc/free during the handling. Rename them to remove
the confusion.

Signed-off-by: Baoquan He <bhe@redhat.com>
---
 mm/sparse.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/mm/sparse.c b/mm/sparse.c
index 054b99f74181..374206212d01 100644
--- a/mm/sparse.c
+++ b/mm/sparse.c
@@ -579,13 +579,13 @@ void offline_mem_sections(unsigned long start_pfn, unsigned long end_pfn)
 #endif
 
 #ifdef CONFIG_SPARSEMEM_VMEMMAP
-static inline struct page *kmalloc_section_memmap(unsigned long pnum, int nid,
+static inline struct page *alloc_section_memmap(unsigned long pnum, int nid,
 		struct vmem_altmap *altmap)
 {
 	/* This will make the necessary allocations eventually. */
 	return sparse_mem_map_populate(pnum, nid, altmap);
 }
-static void __kfree_section_memmap(struct page *memmap,
+static void __free_section_memmap(struct page *memmap,
 		struct vmem_altmap *altmap)
 {
 	unsigned long start = (unsigned long)memmap;
@@ -603,7 +603,7 @@ static void free_map_bootmem(struct page *memmap)
 }
 #endif /* CONFIG_MEMORY_HOTREMOVE */
 #else
-static struct page *__kmalloc_section_memmap(void)
+static struct page *__alloc_section_memmap(void)
 {
 	struct page *page, *ret;
 	unsigned long memmap_size = sizeof(struct page) * PAGES_PER_SECTION;
@@ -624,13 +624,13 @@ static struct page *__kmalloc_section_memmap(void)
 	return ret;
 }
 
-static inline struct page *kmalloc_section_memmap(unsigned long pnum, int nid,
+static inline struct page *alloc_section_memmap(unsigned long pnum, int nid,
 		struct vmem_altmap *altmap)
 {
-	return __kmalloc_section_memmap();
+	return __alloc_section_memmap();
 }
 
-static void __kfree_section_memmap(struct page *memmap,
+static void __free_section_memmap(struct page *memmap,
 		struct vmem_altmap *altmap)
 {
 	if (is_vmalloc_addr(memmap))
@@ -701,7 +701,7 @@ int __meminit sparse_add_one_section(int nid, unsigned long start_pfn,
 	usemap = __kmalloc_section_usemap();
 	if (!usemap)
 		return -ENOMEM;
-	memmap = kmalloc_section_memmap(section_nr, nid, altmap);
+	memmap = alloc_section_memmap(section_nr, nid, altmap);
 	if (!memmap) {
 		kfree(usemap);
 		return -ENOMEM;
@@ -726,7 +726,7 @@ int __meminit sparse_add_one_section(int nid, unsigned long start_pfn,
 out:
 	if (ret < 0) {
 		kfree(usemap);
-		__kfree_section_memmap(memmap, altmap);
+		__free_section_memmap(memmap, altmap);
 	}
 	return ret;
 }
@@ -777,7 +777,7 @@ static void free_section_usemap(struct page *memmap, unsigned long *usemap,
 	if (PageSlab(usemap_page) || PageCompound(usemap_page)) {
 		kfree(usemap);
 		if (memmap)
-			__kfree_section_memmap(memmap, altmap);
+			__free_section_memmap(memmap, altmap);
 		return;
 	}
 
-- 
2.17.2

Re: [PATCH 1/3] mm/sparse: Clean up the obsolete code comment

[Mike Rapoport]

Hi,

On Wed, Mar 20, 2019 at 03:35:38PM +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 <bhe@redhat.com>
> ---
>  mm/sparse.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/mm/sparse.c b/mm/sparse.c
> index 77a0554fa5bd..0a0f82c5d969 100644
> --- a/mm/sparse.c
> +++ b/mm/sparse.c
> @@ -674,9 +674,12 @@ static void free_map_bootmem(struct page *memmap)
>  #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

Please mention that this is only intended for memory hotplug

> + * @nid:	The node to add section on
> + * @start_pfn:	start pfn of the memory range
> + * @altmap:	device page map
> + *
> + * Return 0 on success and an appropriate error code otherwise.

s/Return/Return:/ please

>   */
>  int __meminit sparse_add_one_section(int nid, unsigned long start_pfn,
>  				     struct vmem_altmap *altmap)
> -- 
> 2.17.2
> 

-- 
Sincerely yours,
Mike.

Re: [PATCH 1/3] mm/sparse: Clean up the obsolete code comment

[Baoquan He]

On 03/20/19 at 09:50am, Mike Rapoport wrote:
> Hi,
> 
> On Wed, Mar 20, 2019 at 03:35:38PM +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 <bhe@redhat.com>
> > ---
> >  mm/sparse.c | 9 ++++++---
> >  1 file changed, 6 insertions(+), 3 deletions(-)
> > 
> > diff --git a/mm/sparse.c b/mm/sparse.c
> > index 77a0554fa5bd..0a0f82c5d969 100644
> > --- a/mm/sparse.c
> > +++ b/mm/sparse.c
> > @@ -674,9 +674,12 @@ static void free_map_bootmem(struct page *memmap)
> >  #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
> 
> Please mention that this is only intended for memory hotplug

Will do. Thanks for reviewing.

> 
> > + * @nid:	The node to add section on
> > + * @start_pfn:	start pfn of the memory range
> > + * @altmap:	device page map
> > + *
> > + * Return 0 on success and an appropriate error code otherwise.
> 
> s/Return/Return:/ please

Thanks, will change.

> 
> >   */
> >  int __meminit sparse_add_one_section(int nid, unsigned long start_pfn,
> >  				     struct vmem_altmap *altmap)
> > -- 
> > 2.17.2
> > 
> 
> -- 
> Sincerely yours,
> Mike.
> 

Re: [PATCH 1/3] mm/sparse: Clean up the obsolete code comment

[Matthew Wilcox]

On Wed, Mar 20, 2019 at 03:35:38PM +0800, Baoquan He wrote:
>  /*
> - * 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
> + *
> + * Return 0 on success and an appropriate error code otherwise.
>   */

I think it's worth documenting what those error codes are.  Seems to be
just -ENOMEM and -EEXIST, but it'd be nice for users to know what they
can expect under which circumstances.

Also, -EEXIST is a bad errno to return here:

$ errno EEXIST
EEXIST 17 File exists

What file?  I think we should be using -EBUSY instead in case this errno
makes it back to userspace:

$ errno EBUSY
EBUSY 16 Device or resource busy

Re: [PATCH 1/3] mm/sparse: Clean up the obsolete code comment

[Baoquan He]

On 03/20/19 at 04:19am, Matthew Wilcox wrote:
> On Wed, Mar 20, 2019 at 03:35:38PM +0800, Baoquan He wrote:
> >  /*
> > - * 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
> > + *
> > + * Return 0 on success and an appropriate error code otherwise.
> >   */
> 
> I think it's worth documenting what those error codes are.  Seems to be
> just -ENOMEM and -EEXIST, but it'd be nice for users to know what they
> can expect under which circumstances.
> 
> Also, -EEXIST is a bad errno to return here:
> 
> $ errno EEXIST
> EEXIST 17 File exists
> 
> What file?  I think we should be using -EBUSY instead in case this errno
> makes it back to userspace:
> 
> $ errno EBUSY
> EBUSY 16 Device or resource busy

OK, will update per your comments. Thanks.
> 

Re: [PATCH 1/3] mm/sparse: Clean up the obsolete code comment

[Oscar Salvador]

On Wed, Mar 20, 2019 at 04:19:59AM -0700, Matthew Wilcox wrote:
> On Wed, Mar 20, 2019 at 03:35:38PM +0800, Baoquan He wrote:
> >  /*
> > - * 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
> > + *
> > + * Return 0 on success and an appropriate error code otherwise.
> >   */
> 
> I think it's worth documenting what those error codes are.  Seems to be
> just -ENOMEM and -EEXIST, but it'd be nice for users to know what they
> can expect under which circumstances.
> 
> Also, -EEXIST is a bad errno to return here:
> 
> $ errno EEXIST
> EEXIST 17 File exists
> 
> What file?  I think we should be using -EBUSY instead in case this errno
> makes it back to userspace:
> 
> $ errno EBUSY
> EBUSY 16 Device or resource busy

We return -EEXIST in case the section we are trying to add is already
there, and that error is being caught by __add_pages(), which ignores the
error in case is -EXIST and keeps going with further sections.

Sure we can change that for -EBUSY, but I think -EEXIST makes more sense,
plus that kind of error is never handed back to userspace.

-- 
Oscar Salvador
SUSE L3

Re: [PATCH 1/3] mm/sparse: Clean up the obsolete code comment

[Matthew Wilcox]

On Wed, Mar 20, 2019 at 01:20:15PM +0100, Oscar Salvador wrote:
> On Wed, Mar 20, 2019 at 04:19:59AM -0700, Matthew Wilcox wrote:
> > On Wed, Mar 20, 2019 at 03:35:38PM +0800, Baoquan He wrote:
> > >  /*
> > > - * 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
> > > + *
> > > + * Return 0 on success and an appropriate error code otherwise.
> > >   */
> > 
> > I think it's worth documenting what those error codes are.  Seems to be
> > just -ENOMEM and -EEXIST, but it'd be nice for users to know what they
> > can expect under which circumstances.
> > 
> > Also, -EEXIST is a bad errno to return here:
> > 
> > $ errno EEXIST
> > EEXIST 17 File exists
> > 
> > What file?  I think we should be using -EBUSY instead in case this errno
> > makes it back to userspace:
> > 
> > $ errno EBUSY
> > EBUSY 16 Device or resource busy
> 
> We return -EEXIST in case the section we are trying to add is already
> there, and that error is being caught by __add_pages(), which ignores the
> error in case is -EXIST and keeps going with further sections.
> 
> Sure we can change that for -EBUSY, but I think -EEXIST makes more sense,
> plus that kind of error is never handed back to userspace.

Not returned to userspace today.  It's also bad precedent for other parts
of the kernel where errnos do get returned to userspace.

Re: [PATCH 1/3] mm/sparse: Clean up the obsolete code comment

[Mike Rapoport]

On Wed, Mar 20, 2019 at 05:22:43AM -0700, Matthew Wilcox wrote:
> On Wed, Mar 20, 2019 at 01:20:15PM +0100, Oscar Salvador wrote:
> > On Wed, Mar 20, 2019 at 04:19:59AM -0700, Matthew Wilcox wrote:
> > > On Wed, Mar 20, 2019 at 03:35:38PM +0800, Baoquan He wrote:
> > > >  /*
> > > > - * 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
> > > > + *
> > > > + * Return 0 on success and an appropriate error code otherwise.
> > > >   */
> > > 
> > > I think it's worth documenting what those error codes are.  Seems to be
> > > just -ENOMEM and -EEXIST, but it'd be nice for users to know what they
> > > can expect under which circumstances.
> > > 
> > > Also, -EEXIST is a bad errno to return here:
> > > 
> > > $ errno EEXIST
> > > EEXIST 17 File exists
> > > 
> > > What file?  I think we should be using -EBUSY instead in case this errno
> > > makes it back to userspace:
> > > 
> > > $ errno EBUSY
> > > EBUSY 16 Device or resource busy
> > 
> > We return -EEXIST in case the section we are trying to add is already
> > there, and that error is being caught by __add_pages(), which ignores the
> > error in case is -EXIST and keeps going with further sections.
> > 
> > Sure we can change that for -EBUSY, but I think -EEXIST makes more sense,
> > plus that kind of error is never handed back to userspace.
> 
> Not returned to userspace today.  It's also bad precedent for other parts
> of the kernel where errnos do get returned to userspace.

There are more than a thousand -EEXIST in the kernel, I really doubt all of
them mean "File exists" ;-)

-- 
Sincerely yours,
Mike.

Re: [PATCH 1/3] mm/sparse: Clean up the obsolete code comment

[Matthew Wilcox]

On Wed, Mar 20, 2019 at 02:36:58PM +0200, Mike Rapoport wrote:
> There are more than a thousand -EEXIST in the kernel, I really doubt all of
> them mean "File exists" ;-)

And yet that's what the user will see if it's ever printed with perror()
or similar.  We're pretty bad at choosing errnos; look how abused
ENOSPC is:

$ errno ENOSPC
ENOSPC 28 No space left on device

net/sunrpc/auth_gss/gss_rpc_xdr.c:              return -ENOSPC;

... that's an authentication failure, not "I've run out of disc space".

Re: [PATCH 1/3] mm/sparse: Clean up the obsolete code comment

[Baoquan He]

Hi all,

On 03/20/19 at 05:58am, Matthew Wilcox wrote:
> On Wed, Mar 20, 2019 at 02:36:58PM +0200, Mike Rapoport wrote:
> > There are more than a thousand -EEXIST in the kernel, I really doubt all of
> > them mean "File exists" ;-)
> 
> And yet that's what the user will see if it's ever printed with perror()
> or similar.  We're pretty bad at choosing errnos; look how abused
> ENOSPC is:

When I tried to change -EEXIST to -EBUSY, seems the returned value will
return back over the whole path. And -EEXIST is checked explicitly
several times during the path. 

acpi_memory_enable_device -> __add_pages .. -> __add_section -> sparse_add_one_section

Only look into hotplug path triggered by ACPI event, there are also
device memory and ballon memory paths I haven't checked carefully
because not familiar with them.

So from the checking, I tend to agree with Oscar and Mike. There have
been so many places to use '-EEXIST' to indicate that stuffs checked have
been existing. We can't deny it's inconsistent with term explanation
text. While the defense is that -EEXIST is more precise to indicate a
static instance has been present when we want to create it, but -EBUSY
is a little blizarre. I would rather see -EBUSY is used on a device.
When want to stop it or destroy it, need check if it's busy or not.

#define EBUSY           16      /* Device or resource busy */
#define EEXIST          17      /* File exists */

Obviously saying resource busy or not, it violates semanics in any
language. So many people use EEXIST instead, isn't it the obsolete
text's fault?

Personal opinion.

Thanks
Baoquan
> 
> $ errno ENOSPC
> ENOSPC 28 No space left on device
> 
> net/sunrpc/auth_gss/gss_rpc_xdr.c:              return -ENOSPC;
> 
> ... that's an authentication failure, not "I've run out of disc space".

Re: [PATCH 1/3] mm/sparse: Clean up the obsolete code comment

[Baoquan He]

On 03/21/19 at 02:40pm, Baoquan He wrote:
> Hi all,
> 
> On 03/20/19 at 05:58am, Matthew Wilcox wrote:
> > On Wed, Mar 20, 2019 at 02:36:58PM +0200, Mike Rapoport wrote:
> > > There are more than a thousand -EEXIST in the kernel, I really doubt all of
> > > them mean "File exists" ;-)
> > 
> > And yet that's what the user will see if it's ever printed with perror()
> > or similar.  We're pretty bad at choosing errnos; look how abused
> > ENOSPC is:
> 
> When I tried to change -EEXIST to -EBUSY, seems the returned value will
> return back over the whole path. And -EEXIST is checked explicitly
> several times during the path. 
> 
> acpi_memory_enable_device -> __add_pages .. -> __add_section -> sparse_add_one_section
> 
> Only look into hotplug path triggered by ACPI event, there are also
> device memory and ballon memory paths I haven't checked carefully
> because not familiar with them.
> 
> So from the checking, I tend to agree with Oscar and Mike. There have
> been so many places to use '-EEXIST' to indicate that stuffs checked have
> been existing. We can't deny it's inconsistent with term explanation
> text. While the defense is that -EEXIST is more precise to indicate a
> static instance has been present when we want to create it, but -EBUSY
> is a little blizarre. I would rather see -EBUSY is used on a device.
> When want to stop it or destroy it, need check if it's busy or not.
> 
> #define EBUSY           16      /* Device or resource busy */
> #define EEXIST          17      /* File exists */
> 
> Obviously saying resource busy or not, it violates semanics in any
> language. So many people use EEXIST instead, isn't it the obsolete

Surely when we require a lock which is protecting resource, we can also
return -EBUSY since someone is busy on this resource. For creating one
instance, just check if the instance exists already, no matter what the
code comment of the errno is saying, IMHO, it really should not be -EBUSY.

Thanks
Baoquan

Re: [PATCH 1/3] mm/sparse: Clean up the obsolete code comment

[William Kucharski]

> On Mar 21, 2019, at 3:21 AM, Baoquan He <bhe@redhat.com> wrote:

It appears as is so often the case that the usage has far outpaced the
documentation and -EEXIST may be the proper code to return.

The correct answer here may be to modify the documentation to note the
additional semantic, though if the usage is solely within the kernel it
may be sufficient to explain its use in the header comment for the
routine (in this case sparse_add_one_section()).

Re: [PATCH 1/3] mm/sparse: Clean up the obsolete code comment

[Michal Hocko]

On Thu 21-03-19 04:24:35, William Kucharski wrote:
> 
> 
> > On Mar 21, 2019, at 3:21 AM, Baoquan He <bhe@redhat.com> wrote:
> 
> It appears as is so often the case that the usage has far outpaced the
> documentation and -EEXIST may be the proper code to return.
> 
> The correct answer here may be to modify the documentation to note the
> additional semantic, though if the usage is solely within the kernel it
> may be sufficient to explain its use in the header comment for the
> routine (in this case sparse_add_one_section()).

Is this really worth? It is a well known problem that errno codes are
far from sufficient to describe error codes we need. Yet we are stuck
with them more or less. I really do not see any point changing this
particular path, nor spend a lot of time whether one inappropriate
code is any better than another one. The code works as intended AFAICS.

I would stick with all good rule of thumb. It works, do not touch it too
much.

I am sorry to be snarky but hasn't this generated way much more email
traffic than it really deserves? A simply and trivial clean up in the
beginning that was it, right?
-- 
Michal Hocko
SUSE Labs

Re: [PATCH 1/3] mm/sparse: Clean up the obsolete code comment

[William Kucharski]

> On Mar 21, 2019, at 4:35 AM, Michal Hocko <mhocko@kernel.org> wrote:
> 
> I am sorry to be snarky but hasn't this generated way much more email
> traffic than it really deserves? A simply and trivial clean up in the
> beginning that was it, right?

That's rather the point; that it did generate a fair amount of email
traffic indicates it's worthy of at least a passing mention in a
comment somewhere.

Re: [PATCH 1/3] mm/sparse: Clean up the obsolete code comment

[Baoquan He]

On 03/21/19 at 05:19am, William Kucharski wrote:
> 
> 
> > On Mar 21, 2019, at 4:35 AM, Michal Hocko <mhocko@kernel.org> wrote:
> > 
> > I am sorry to be snarky but hasn't this generated way much more email
> > traffic than it really deserves? A simply and trivial clean up in the
> > beginning that was it, right?

Yeah, I'd like to do like this. Will arrange patch and post a new
version. Sorry about the mail bomb to CCed people.

Yet I also would like to hear any suggestion from people who intend to
improve. Discussions make me know more the status of errno than before.

Thank you all for sharing.

> 
> That's rather the point; that it did generate a fair amount of email
> traffic indicates it's worthy of at least a passing mention in a
> comment somewhere.

We header files to put errno. Only changing in kernel may cause
difference between it and userspace. I will list each returned value in
code comment and tell what they are meaning in this function, that could
be helpful. Thanks.

usr/include/asm-generic/errno-base.h 
include/uapi/asm-generic/errno-base.h

Re: [PATCH 1/3] mm/sparse: Clean up the obsolete code comment

[Oscar Salvador]

On Wed, Mar 20, 2019 at 05:22:43AM -0700, Matthew Wilcox wrote:
> On Wed, Mar 20, 2019 at 01:20:15PM +0100, Oscar Salvador wrote:
> > On Wed, Mar 20, 2019 at 04:19:59AM -0700, Matthew Wilcox wrote:
> > > On Wed, Mar 20, 2019 at 03:35:38PM +0800, Baoquan He wrote:
> > > >  /*
> > > > - * 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
> > > > + *
> > > > + * Return 0 on success and an appropriate error code otherwise.
> > > >   */
> > > 
> > > I think it's worth documenting what those error codes are.  Seems to be
> > > just -ENOMEM and -EEXIST, but it'd be nice for users to know what they
> > > can expect under which circumstances.
> > > 
> > > Also, -EEXIST is a bad errno to return here:
> > > 
> > > $ errno EEXIST
> > > EEXIST 17 File exists
> > > 
> > > What file?  I think we should be using -EBUSY instead in case this errno
> > > makes it back to userspace:
> > > 
> > > $ errno EBUSY
> > > EBUSY 16 Device or resource busy
> > 
> > We return -EEXIST in case the section we are trying to add is already
> > there, and that error is being caught by __add_pages(), which ignores the
> > error in case is -EXIST and keeps going with further sections.
> > 
> > Sure we can change that for -EBUSY, but I think -EEXIST makes more sense,
> > plus that kind of error is never handed back to userspace.
> 
> Not returned to userspace today.  It's also bad precedent for other parts
> of the kernel where errnos do get returned to userspace.

Yes, I get your point, but I do not really see -EBUSY fitting here.
Actually, we do have the same kind of situation when dealing with resources.
We return -EEXIST in register_memory_resource() in case the resource we are
trying to add conflicts with another one.

I think that -EEXIST is more intuitive in that code path, but I am not going to
insist.

-- 
Oscar Salvador
SUSE L3