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:   Tue, 11 Feb 2020 16:04:06 -0600
From:   Scott Cheloha <cheloha@linux.ibm.com>
To:     Nathan Lynch <nathanl@linux.ibm.com>
Cc:     Nathan Fontenont <ndfont@gmail.com>,
        Rick Lindsley <ricklind@linux.vnet.ibm.com>,
        linux-kernel@vger.kernel.org, Michael Ellerman <mpe@ellerman.id.au>
Subject: Re: [PATCH] pseries/hotplug-memory: remove
 dlpar_memory_{add,remove}_by_index() functions
Message-ID: <20200211220406.72jdndodtp23v4w2@rascal.austin.ibm.com>
References: <20200127200839.12441-1-cheloha@linux.ibm.com>
 <87ftfimbjy.fsf@linux.ibm.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <87ftfimbjy.fsf@linux.ibm.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Mon, Feb 10, 2020 at 02:28:49PM -0600, Nathan Lynch wrote:
> Scott Cheloha <cheloha@linux.ibm.com> writes:
> > The dlpar_memory_{add,remove}_by_index() functions are just special
> > cases of their dlpar_memory_{add,remove}_by_ic() counterparts where
> > the LMB count is 1.
> 
> I wish that were the case, but there are (gratuitous?) differences:
> 
> - dlpar_memory_remove_by_ic() checks DRCONF_MEM_RESERVED and
>   DRCONF_MEM_ASSIGNED flags; dlpar_memory_remove_by_index() does not.

The lack of a DRCONF_MEM_RESERVED check in add_by_index() and
remove_by_index() might be a mistake.  If I understand the PAPR
correctly, when DRCONF_MEM_RESERVED is set for an LMB the operating
system isn't allowed to touch it.  The LMB could become available for
use later if the platform clears the bit, but if it's set it's
no good.

DRCONF_MEM_ASSIGNED checks are not present in
dlpar_memory_add_by_index() and dlpar_memory_remove_by_index() but
they are done at the top of dlpar_add_lmb() and lmb_is_removable(),
so the checks do happen in those paths.

> - dlpar_memory_remove_by_ic() attempts to roll back failed removal;
>   dlpar_memory_remove_by_index() does not.

The exclusion of rollback in the remove_by_index() path makes sense,
as there are only N=1 possible elements where the operation can fail.
Doing the marking/unmarking for rollback in the N=1 case is harmless
though.  If the removal fails for the given LMB we never call
drmem_mark_reserved() to indicate that we need to re-add it.  The
rollback loop then finds no marked LMBs and does no work.

> I'm not sure how much either of these gets used in practice. AFAIK the
> usual HMC/drmgr-driven workflow tends to exercise
> dlpar_memory_remove_by_count().

drmgr eventually uses dlpar_memory_*_by_count() when you give it a
count of LMBs with the '-q' flag, e.g.:

# drmgr -c mem -a -q 10

drmgr eventually uses dlpar_memory_*_by_index() when you give it a
particular DRC, e.g.:

# drmgr -c mem -a -s <some drc number>

QEMU hotplug uses dlpar_memory_*_by_ic().

> I agree this code needs consolidation, but we should proceed a little
> carefully because it's likely going to entail changing some user-visible
> behaviors.

Sure.

Maybe there are less ambitious ways to start out.