mm/sparse: Clean up the obsolete code comment

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 <[email protected]>

---
 mm/sparse.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

mm/sparse.c +0 -3

index 0a0f82c5d969..054b99f74181 100644
@@ -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)

mm/sparse.c +1 -0

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

Signature · 1 line

2.17.2

Quoted 16 lines

mm/sparse.c +1 -6

index 0a0f82c5d969..054b99f74181 100644
@@ -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);

mm/sparse.c int __meminit sparse_add_one_section(int nid, unsigned long start_pfn, · line 706

Quoted 5 lines

mm/sparse.c +5 -0

+	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?

Quoted 11 lines

mm/sparse.c +1 -0

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

Signature · 1 line

2.17.2

Signature · 2 lines

Sincerely yours,
Mike.

(quoted diff) +0 -0

@@ -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;

Quoted 4 lines

(quoted diff) +1 -2

+
 	usemap = __kmalloc_section_usemap();
-	if (!usemap) {
-		__kfree_section_memmap(memmap, altmap);

Review

Quoted 5 lines

(quoted diff) +5 -0

+	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?

Hi Mike,

Quoted 12 lines · Mike Rapoport

(quoted diff) +0 -0

@@ -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;

Quoted 4 lines

mm/sparse.c +1 -2

+
 	usemap = __kmalloc_section_usemap();
-	if (!usemap) {
-		__kfree_section_memmap(memmap, altmap);

Quoted 8 lines

mm/sparse.c int __meminit sparse_add_one_section(int nid, unsigned long start_pfn,

Quoted 5 lines

mm/sparse.c +5 -0

+	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 <[email protected]>
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 <[email protected]>

---
 mm/sparse.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

mm/sparse.c +0 -4

index a99e0b253927..0e842b924be6 100644
@@ -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;

mm/sparse.c +4 -4

 	/*

@@ -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;
 }
 

Signature · 1 line

2.17.2

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]>

---
 mm/sparse.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

mm/sparse.c +0 -0

index 77a0554fa5bd..0a0f82c5d969 100644

mm/sparse.c +0 -0

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

Signature · 1 line

2.17.2

Hi,

Quoted 15 lines · 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 <[email protected]>

---
 mm/sparse.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

mm/sparse.c +0 -0

index 77a0554fa5bd..0a0f82c5d969 100644
@@ -674,9 +674,12 @@ static void free_map_bootmem(struct page *memmap)
 #endif /* CONFIG_SPARSEMEM_VMEMMAP */
 

mm/sparse.c static void free_map_bootmem(struct page *memmap) · line 677

Quoted 5 lines

mm/sparse.c +1 -3

 /*
- * 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

mm/sparse.c static void free_map_bootmem(struct page *memmap) · line 682

Quoted 6 lines

mm/sparse.c +5 -0

+ * @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

Quoted 7 lines

mm/sparse.c +0 -0

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

Signature · 1 line

2.17.2

Signature · 2 lines

Sincerely yours,
Mike.

/*
- * 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. */

Quoted 11 lines · Baoquan He

/*
- * 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

Quoted 11 lines · Baoquan He

/*
- * 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

Quoted 19 lines · Matthew Wilcox

Quoted 11 lines · Baoquan He

/*
- * 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:

Quoted 8 lines

$ 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.

Quoted 33 lines · Oscar Salvador

Quoted 19 lines · Matthew Wilcox

Quoted 11 lines · Baoquan He

/*
- * 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:

Quoted 8 lines

$ 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.

Quoted 2 lines

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" ;-)

Quoted 2 lines · Mike Rapoport

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".

Hi all,

Quoted 7 lines · Matthew Wilcox

Quoted 2 lines · Mike Rapoport

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

Quoted 3 lines · Baoquan He

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()).

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?

Quoted 7 lines · Michal Hocko

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.

Quoted 33 lines · Oscar Salvador

Quoted 19 lines · Matthew Wilcox

Quoted 11 lines · Baoquan He

/*
- * 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:

Quoted 8 lines

$ 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.

Quoted 2 lines

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.