Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Wed, 20 Mar 2019 05:22:43 -0700
From:   Matthew Wilcox <willy@infradead.org>
To:     Oscar Salvador <osalvador@suse.de>
Cc:     Baoquan He <bhe@redhat.com>, linux-kernel@vger.kernel.org,
        akpm@linux-foundation.org, pasha.tatashin@oracle.com,
        mhocko@suse.com, rppt@linux.vnet.ibm.com,
        richard.weiyang@gmail.com, linux-mm@kvack.org
Subject: Re: [PATCH 1/3] mm/sparse: Clean up the obsolete code comment
Message-ID: <20190320122243.GX19508@bombadil.infradead.org>
References: <20190320073540.12866-1-bhe@redhat.com>
 <20190320111959.GV19508@bombadil.infradead.org>
 <20190320122011.stuoqugpjdt3d7cd@d104.suse.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20190320122011.stuoqugpjdt3d7cd@d104.suse.de>
User-Agent: Mutt/1.9.2 (2017-12-15)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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.