| Message ID | 20190320073540.12866-1-bhe@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/3] mm/sparse: Clean up the obsolete code comment | expand |
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 >
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. >
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
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. >
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.
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.
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" ;-)
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.
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".
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".
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
> 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()).
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?
> 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.
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
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)
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(-)