* Re: [PATCH kernel] cxl: Remove unused include
From: Frederic Barrat @ 2018-10-03 8:29 UTC (permalink / raw)
To: linuxppc-dev
In-Reply-To: <548a710f-b542-b361-b056-74a8d58c5ceb@ozlabs.ru>
Le 03/10/2018 à 09:46, Alexey Kardashevskiy a écrit :
>
>
> On 28/09/2018 22:46, Michael Ellerman wrote:
>> Alexey Kardashevskiy <aik@ozlabs.ru> writes:
>>> The included opal.h gives a wrong idea that CXL makes PPC OPAL calls
>>> while it does not so let's remote it.
>>
>> But it does use eg.
>>
>> OPAL_PHB_CAPI_MODE_SNOOP_ON
>> OPAL_PHB_CAPI_MODE_CAPI
>>
>> Which come from opal-api.h via opal.h.
>>
>> So you should at least include opal-api.h.
>
>
> I'd say that since it includes pnv-pci.h (and this is why this patch
> compiles), it should be a different set of values for powernv (which
> would map 1:1 to OPAL_PHB_xxx). The powernv platform knowledge is
> already intimate enough for a driver to have. Dunno, I found this opal.h
> inclusion confusing, that's all.
I agree, it would be the cleaner way of doing it. It's going to be
pretty low on my list though...
Fred
>
>
>>
>> cheers
>>
>>> diff --git a/drivers/misc/cxl/pci.c b/drivers/misc/cxl/pci.c
>>> index b66d832..8cbcbb7 100644
>>> --- a/drivers/misc/cxl/pci.c
>>> +++ b/drivers/misc/cxl/pci.c
>>> @@ -17,7 +17,6 @@
>>> #include <linux/pci.h>
>>> #include <linux/of.h>
>>> #include <linux/delay.h>
>>> -#include <asm/opal.h>
>>> #include <asm/msi_bitmap.h>
>>> #include <asm/pnv-pci.h>
>>> #include <asm/io.h>
>>> --
>>> 2.11.0
>
^ permalink raw reply
* Re: [RFC PATCH v3 4/7] powerpc: regain entire stack space
From: Christophe LEROY @ 2018-10-03 9:30 UTC (permalink / raw)
To: Nicholas Piggin; +Cc: linux-kernel, Paul Mackerras, aneesh.kumar, linuxppc-dev
In-Reply-To: <20181003170736.34082f4d@roar.ozlabs.ibm.com>
Le 03/10/2018 à 09:07, Nicholas Piggin a écrit :
> On Wed, 3 Oct 2018 08:45:25 +0200
> Christophe LEROY <christophe.leroy@c-s.fr> wrote:
>
>> Le 03/10/2018 à 08:30, Nicholas Piggin a écrit :
>>> On Wed, 3 Oct 2018 07:52:59 +0200
>>> Christophe LEROY <christophe.leroy@c-s.fr> wrote:
>>>
>>>> Le 03/10/2018 à 07:34, Nicholas Piggin a écrit :
>>>>> On Mon, 1 Oct 2018 12:30:25 +0000 (UTC)
>>>>> Christophe Leroy <christophe.leroy@c-s.fr> wrote:
>>>>>
>>>>>> thread_info is not anymore in the stack, so the entire stack
>>>>>> can now be used.
>>>>>
>>>>> Nice.
>>>>>
>>>>>>
>>>>>> In the meantime, all pointers to the stacks are not anymore
>>>>>> pointers to thread_info so this patch changes them to void*
>>>>>
>>>>> Wasn't this previously effectively already the case with patch
>>>>> 3/7? You had thread_info sized space left there, but it was not
>>>>> used or initialized right? Does it make sense to move this part
>>>>> of it to the previous patch?
>>>>
>>>> Not really.
>>>>
>>>> In 3/7 I changed the prototypes of two functions that really used the
>>>> pointer as a task pointer only.
>>
>> I meant 2/7 here sorry.
>>
>>>>
>>>> Here it change things that before 4/7 were really used as both stack
>>>> pointers and thread_info pointers.
>>
>> And here I meant 3/7
>>
>>>
>>> What uses it as a thread_info pointer? It seems more like a stack
>>> with some amount of unused space in it but that's all.
>>
>> Before 3/7, we have
>>
>> void do_softirq_own_stack(void)
>> {
>> struct thread_info *curtp, *irqtp;
>>
>> curtp = current_thread_info();
>> irqtp = softirq_ctx[smp_processor_id()];
>> irqtp->task = curtp->task;
>> irqtp->flags = 0;
>> call_do_softirq(irqtp);
>> irqtp->task = NULL;
>>
>> /* Set any flag that may have been set on the
>> * alternate stack
>> */
>> if (irqtp->flags)
>> set_bits(irqtp->flags, &curtp->flags);
>> }
>>
>> After 3/7, we have
>>
>> void do_softirq_own_stack(void)
>> {
>> struct thread_info *irqtp;
>>
>> irqtp = softirq_ctx[smp_processor_id()];
>> call_do_softirq(irqtp);
>> }
>>
>>
>> So now only we can change irqtp to void* can't we ?
>
> In patch 3 we can, right? That's what I mean by moving from
> thread_info * to void * in patch 3 rather than 4.
Ah ok, that's what you meant. Sorry.
>
> But if you prefer not to, it's fine. Maybe it keeps patch 3
> a little smaller.
Yes indeed, that's the idea, keep patch 3 to the strict minimum and do
cleanups afterwards.
Christophe
>
> Thanks,
> Nick
>
^ permalink raw reply
* Re: [PATCH] powerpc: use PTRRELOC during early init
From: Andreas Schwab @ 2018-10-03 11:18 UTC (permalink / raw)
To: Christophe LEROY
Cc: Michael Neuling, Michal Suchánek, linuxppc-dev, Haren Myneni,
Nicholas Piggin
In-Reply-To: <acfeead1-90f9-d13c-41f2-e4bdd38e3702@c-s.fr>
On Okt 03 2018, Christophe LEROY <christophe.leroy@c-s.fr> wrote:
> Did you try my proposed fix https://patchwork.ozlabs.org/patch/977195/ ?
That works as well.
Andreas.
--
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510 2552 DF73 E780 A9DA AEC1
"And now for something completely different."
^ permalink raw reply
* Re: [PATCH v9 0/3] powerpc: Detection and scheduler optimization for POWER9 bigcore
From: Gautham R Shenoy @ 2018-10-03 11:19 UTC (permalink / raw)
To: Dave Hansen
Cc: Gautham R. Shenoy, Michael Neuling, Srikar Dronamraju,
Murilo Opsfelder Araujo, linux-kernel, Nicholas Piggin,
Oliver O'Halloran, Aneesh Kumar K.V, Shilpasri G Bhat,
linuxppc-dev, Akshay Adiga, Anton Blanchard
In-Reply-To: <994106db-7f79-7d22-0d4e-f458bd85bb64@intel.com>
Hello Dave,
On Mon, Oct 01, 2018 at 07:05:11AM -0700, Dave Hansen wrote:
> On 10/01/2018 06:16 AM, Gautham R. Shenoy wrote:
> >
> > Patch 3: Creates a pair of sysfs attributes named
> > /sys/devices/system/cpu/cpuN/topology/smallcore_thread_siblings
> > and
> > /sys/devices/system/cpu/cpuN/topology/smallcore_thread_siblings_list
> > exposing the small-core siblings that share the L1 cache
> > to the userspace.
>
> I really don't think you've justified the existence of a new user/kernel
> interface here. We already have information about threads share L1
> caches in here:
>
> /sys/devices/system/cpu/cpu0/cache/index0/shared_cpu_list
Hmmm. My bad, I wasn't aware of this sysfs interface. We can use this
to share information about L1 cache. And currently on the upstream
kernel we report incorrect value on POWER9 SMT8 core. I will fix
this. Thanks for calling this out.
>
> The only question would be if anything would break because it assumes
> that all SMT siblings share all caches. But, it breaks if your new
> interface is there or not; it's old software that we care about.
>
Up until POWER9 SMT8 cores, this assumption was true that all SMT
siblings share all caches. POWER9 SMT8 cores are the only exception.
--
Thanks and Regards
gautham.
^ permalink raw reply
* Re: [PATCH v03 1/5] powerpc/drmem: Export 'dynamic-memory' loader
From: Michael Bringmann @ 2018-10-03 13:22 UTC (permalink / raw)
To: linuxppc-dev
In-Reply-To: <d876a9a2-548e-1da3-f72f-211760c5c29a@linux.vnet.ibm.com>
On 10/02/2018 03:56 PM, Tyrel Datwyler wrote:
> On 10/01/2018 05:59 AM, Michael Bringmann wrote:
>> powerpc/drmem: Export many of the functions of DRMEM to parse
>> "ibm,dynamic-memory" and "ibm,dynamic-memory-v2" during hotplug
>> operations and for Post Migration events.
>>
>> Also modify the DRMEM initialization code to allow it to,
>>
>> * Be called after system initialization
>> * Provide a separate user copy of the LMB array that is produces
>> * Free the user copy upon request
>>
>> In addition, a couple of changes were made to make the creation
>> of additional copies of the LMB array more useful including,
>>
>> * Add new iterator to work through a pair of drmem_info arrays.
>> * Modify DRMEM code to replace usages of dt_root_addr_cells, and
>> dt_mem_next_cell, as these are only available at first boot.
>>
>> Signed-off-by: Michael Bringmann <mwb@linux.vnet.ibm.com>
>> ---
>> arch/powerpc/include/asm/drmem.h | 15 ++++++++
>> arch/powerpc/mm/drmem.c | 75 ++++++++++++++++++++++++++++----------
>> 2 files changed, 70 insertions(+), 20 deletions(-)
>>
>> diff --git a/arch/powerpc/include/asm/drmem.h b/arch/powerpc/include/asm/drmem.h
>> index ce242b9..b0e70fd 100644
>> --- a/arch/powerpc/include/asm/drmem.h
>> +++ b/arch/powerpc/include/asm/drmem.h
>> @@ -35,6 +35,18 @@ struct drmem_lmb_info {
>> &drmem_info->lmbs[0], \
>> &drmem_info->lmbs[drmem_info->n_lmbs - 1])
>>
>> +#define for_each_dinfo_lmb(dinfo, lmb) \
>> + for_each_drmem_lmb_in_range((lmb), \
>> + &dinfo->lmbs[0], \
>> + &dinfo->lmbs[dinfo->n_lmbs - 1])
>> +
>> +#define for_each_pair_dinfo_lmb(dinfo1, lmb1, dinfo2, lmb2) \
>> + for ((lmb1) = (&dinfo1->lmbs[0]), \
>> + (lmb2) = (&dinfo2->lmbs[0]); \
>> + ((lmb1) <= (&dinfo1->lmbs[dinfo1->n_lmbs - 1])) && \
>> + ((lmb2) <= (&dinfo2->lmbs[dinfo2->n_lmbs - 1])); \
>> + (lmb1)++, (lmb2)++)
>> +
>> /*
>> * The of_drconf_cell_v1 struct defines the layout of the LMB data
>> * specified in the ibm,dynamic-memory device tree property.
>> @@ -94,6 +106,9 @@ void __init walk_drmem_lmbs(struct device_node *dn,
>> void (*func)(struct drmem_lmb *, const __be32 **));
>> int drmem_update_dt(void);
>>
>> +struct drmem_lmb_info *drmem_lmbs_init(struct property *prop);
>> +void drmem_lmbs_free(struct drmem_lmb_info *dinfo);
>> +
>> #ifdef CONFIG_PPC_PSERIES
>> void __init walk_drmem_lmbs_early(unsigned long node,
>> void (*func)(struct drmem_lmb *, const __be32 **));
>> diff --git a/arch/powerpc/mm/drmem.c b/arch/powerpc/mm/drmem.c
>> index 3f18036..13d2abb 100644
>> --- a/arch/powerpc/mm/drmem.c
>> +++ b/arch/powerpc/mm/drmem.c
>> @@ -20,6 +20,7 @@
>>
>> static struct drmem_lmb_info __drmem_info;
>> struct drmem_lmb_info *drmem_info = &__drmem_info;
>> +static int n_root_addr_cells;
>
> What is the point of this new global? I see two places were it gets initialized if it is null, and both those initializers simply set it to "dt_root_addr_cells". I also checked the rest of the patches in the series and none of those even reference this variable.
The point of these changes is to introduce code that can be used after
an LPAR migration. The variable dt_root_addr_cells was observed to be
zero (0) after migration on powerpc. As it is defined as,
./drivers/of/fdt.c:int __initdata dt_root_addr_cells;
its definition may be replaced by other code/variables after initial boot.
We needed the value to persist, and it was cheaper to cache it.
>
>>
>> u64 drmem_lmb_memory_max(void)
>> {
>> @@ -193,12 +194,13 @@ int drmem_update_dt(void)
>> return rc;
>> }
>>
>> -static void __init read_drconf_v1_cell(struct drmem_lmb *lmb,
>> +static void read_drconf_v1_cell(struct drmem_lmb *lmb,
>> const __be32 **prop)
>> {
>> const __be32 *p = *prop;
>>
>> - lmb->base_addr = dt_mem_next_cell(dt_root_addr_cells, &p);
>> + lmb->base_addr = of_read_number(p, n_root_addr_cells);
>> + p += n_root_addr_cells;
>
> Unnecessary code churn do to the introduction of "n_root_addr_cells".'
See note above.
>
>> lmb->drc_index = of_read_number(p++, 1);
>>
>> p++; /* skip reserved field */
>> @@ -209,7 +211,7 @@ static void __init read_drconf_v1_cell(struct drmem_lmb *lmb,
>> *prop = p;
>> }
>>
>> -static void __init __walk_drmem_v1_lmbs(const __be32 *prop, const __be32 *usm,
>> +static void __walk_drmem_v1_lmbs(const __be32 *prop, const __be32 *usm,
>> void (*func)(struct drmem_lmb *, const __be32 **))
>> {
>> struct drmem_lmb lmb;
>> @@ -225,13 +227,14 @@ static void __init __walk_drmem_v1_lmbs(const __be32 *prop, const __be32 *usm,
>> }
>> }
>>
>> -static void __init read_drconf_v2_cell(struct of_drconf_cell_v2 *dr_cell,
>> +static void read_drconf_v2_cell(struct of_drconf_cell_v2 *dr_cell,
>> const __be32 **prop)
>> {
>> const __be32 *p = *prop;
>>
>> dr_cell->seq_lmbs = of_read_number(p++, 1);
>> - dr_cell->base_addr = dt_mem_next_cell(dt_root_addr_cells, &p);
>> + dr_cell->base_addr = of_read_number(p, n_root_addr_cells);
>> + p += n_root_addr_cells;
>
> Same comment as above.
See note above.
>
>> dr_cell->drc_index = of_read_number(p++, 1);
>> dr_cell->aa_index = of_read_number(p++, 1);
>> dr_cell->flags = of_read_number(p++, 1);
>> @@ -239,7 +242,7 @@ static void __init read_drconf_v2_cell(struct of_drconf_cell_v2 *dr_cell,
>> *prop = p;
>> }
>>
>> -static void __init __walk_drmem_v2_lmbs(const __be32 *prop, const __be32 *usm,
>> +static void __walk_drmem_v2_lmbs(const __be32 *prop, const __be32 *usm,
>> void (*func)(struct drmem_lmb *, const __be32 **))
>> {
>> struct of_drconf_cell_v2 dr_cell;
>> @@ -275,6 +278,9 @@ void __init walk_drmem_lmbs_early(unsigned long node,
>> const __be32 *prop, *usm;
>> int len;
>>
>> + if (n_root_addr_cells == 0)
>> + n_root_addr_cells = dt_root_addr_cells;
>> +
>
> As I mentioned initially whats the point? Why not just use "dt_root_addr_cells"?
See note above.
>
>> prop = of_get_flat_dt_prop(node, "ibm,lmb-size", &len);
>> if (!prop || len < dt_root_size_cells * sizeof(__be32))
>> return;
>> @@ -353,24 +359,26 @@ void __init walk_drmem_lmbs(struct device_node *dn,
>> }
>> }
>>
>> -static void __init init_drmem_v1_lmbs(const __be32 *prop)
>> +static void init_drmem_v1_lmbs(const __be32 *prop,
>> + struct drmem_lmb_info *dinfo)
>> {
>> struct drmem_lmb *lmb;
>>
>> - drmem_info->n_lmbs = of_read_number(prop++, 1);
>> - if (drmem_info->n_lmbs == 0)
>> + dinfo->n_lmbs = of_read_number(prop++, 1);
>> + if (dinfo->n_lmbs == 0)
>> return;
>>
>> - drmem_info->lmbs = kcalloc(drmem_info->n_lmbs, sizeof(*lmb),
>> + dinfo->lmbs = kcalloc(dinfo->n_lmbs, sizeof(*lmb),
>> GFP_KERNEL);
>> - if (!drmem_info->lmbs)
>> + if (!dinfo->lmbs)
>> return;
>>
>> - for_each_drmem_lmb(lmb)
>> + for_each_dinfo_lmb(dinfo, lmb)
>> read_drconf_v1_cell(lmb, &prop);
>> }
>>
>> -static void __init init_drmem_v2_lmbs(const __be32 *prop)
>> +static void init_drmem_v2_lmbs(const __be32 *prop,
>> + struct drmem_lmb_info *dinfo)
>> {
>> struct drmem_lmb *lmb;
>> struct of_drconf_cell_v2 dr_cell;
>> @@ -386,12 +394,12 @@ static void __init init_drmem_v2_lmbs(const __be32 *prop)
>> p = prop;
>> for (i = 0; i < lmb_sets; i++) {
>> read_drconf_v2_cell(&dr_cell, &p);
>> - drmem_info->n_lmbs += dr_cell.seq_lmbs;
>> + dinfo->n_lmbs += dr_cell.seq_lmbs;
>> }
>>
>> - drmem_info->lmbs = kcalloc(drmem_info->n_lmbs, sizeof(*lmb),
>> + dinfo->lmbs = kcalloc(dinfo->n_lmbs, sizeof(*lmb),
>> GFP_KERNEL);
>> - if (!drmem_info->lmbs)
>> + if (!dinfo->lmbs)
>> return;
>>
>> /* second pass, read in the LMB information */
>> @@ -402,10 +410,10 @@ static void __init init_drmem_v2_lmbs(const __be32 *prop)
>> read_drconf_v2_cell(&dr_cell, &p);
>>
>> for (j = 0; j < dr_cell.seq_lmbs; j++) {
>> - lmb = &drmem_info->lmbs[lmb_index++];
>> + lmb = &dinfo->lmbs[lmb_index++];
>>
>> lmb->base_addr = dr_cell.base_addr;
>> - dr_cell.base_addr += drmem_info->lmb_size;
>> + dr_cell.base_addr += dinfo->lmb_size;
>>
>> lmb->drc_index = dr_cell.drc_index;
>> dr_cell.drc_index++;
>> @@ -416,11 +424,38 @@ static void __init init_drmem_v2_lmbs(const __be32 *prop)
>> }
>> }
>>
>> +void drmem_lmbs_free(struct drmem_lmb_info *dinfo)
>> +{
>> + if (dinfo) {
>> + kfree(dinfo->lmbs);
>> + kfree(dinfo);
>> + }
>> +}
>> +
>> +struct drmem_lmb_info *drmem_lmbs_init(struct property *prop)
>> +{
>> + struct drmem_lmb_info *dinfo;
>> +
>> + dinfo = kzalloc(sizeof(*dinfo), GFP_KERNEL);
>> + if (!dinfo)
>> + return NULL;
>> +
>> + if (!strcmp("ibm,dynamic-memory", prop->name))
>> + init_drmem_v1_lmbs(prop->value, dinfo);
>> + else if (!strcmp("ibm,dynamic-memory-v2", prop->name))
>> + init_drmem_v2_lmbs(prop->value, dinfo);
>> +
>> + return dinfo;
>> +}
>> +
>> static int __init drmem_init(void)
>> {
>> struct device_node *dn;
>> const __be32 *prop;
>>
>> + if (n_root_addr_cells == 0)
>> + n_root_addr_cells = dt_root_addr_cells;
>> +
>
> See previous comment.
See note above.
>
> -Tyrel
Michael
>
>> dn = of_find_node_by_path("/ibm,dynamic-reconfiguration-memory");
>> if (!dn) {
>> pr_info("No dynamic reconfiguration memory found\n");
>> @@ -434,11 +469,11 @@ static int __init drmem_init(void)
>>
>> prop = of_get_property(dn, "ibm,dynamic-memory", NULL);
>> if (prop) {
>> - init_drmem_v1_lmbs(prop);
>> + init_drmem_v1_lmbs(prop, drmem_info);
>> } else {
>> prop = of_get_property(dn, "ibm,dynamic-memory-v2", NULL);
>> if (prop)
>> - init_drmem_v2_lmbs(prop);
>> + init_drmem_v2_lmbs(prop, drmem_info);
>> }
>>
>> of_node_put(dn);
>>
>
>
--
Michael W. Bringmann
Linux Technology Center
IBM Corporation
Tie-Line 363-5196
External: (512) 286-5196
Cell: (512) 466-0650
mwb@linux.vnet.ibm.com
^ permalink raw reply
* Re: [PATCH] migration/mm: Add WARN_ON to try_offline_node
From: Michael Bringmann @ 2018-10-03 13:27 UTC (permalink / raw)
To: Tyrel Datwyler, Michal Hocko
Cc: Thomas Falcon, Kees Cook, Mathieu Malaterre, linux-kernel,
Nicholas Piggin, Pavel Tatashin, linux-mm,
Mauricio Faria de Oliveira, Juliet Kim, Thiago Jung Bauermann,
Nathan Fontenot, Andrew Morton, YASUAKI ISHIMATSU, linuxppc-dev,
Dan Williams, Oscar Salvador
In-Reply-To: <bbc5f219-614f-b024-0888-8ad216c5eaf8@linux.vnet.ibm.com>
On 10/02/2018 02:45 PM, Tyrel Datwyler wrote:
> On 10/02/2018 11:13 AM, Michael Bringmann wrote:
>>
>>
>> On 10/02/2018 11:04 AM, Michal Hocko wrote:
>>> On Tue 02-10-18 10:14:49, Michael Bringmann wrote:
>>>> On 10/02/2018 09:59 AM, Michal Hocko wrote:
>>>>> On Tue 02-10-18 09:51:40, Michael Bringmann wrote:
>>>>> [...]
>>>>>> When the device-tree affinity attributes have changed for memory,
>>>>>> the 'nid' affinity calculated points to a different node for the
>>>>>> memory block than the one used to install it, previously on the
>>>>>> source system. The newly calculated 'nid' affinity may not yet
>>>>>> be initialized on the target system. The current memory tracking
>>>>>> mechanisms do not record the node to which a memory block was
>>>>>> associated when it was added. Nathan is looking at adding this
>>>>>> feature to the new implementation of LMBs, but it is not there
>>>>>> yet, and won't be present in earlier kernels without backporting a
>>>>>> significant number of changes.
>>>>>
>>>>> Then the patch you have proposed here just papers over a real issue, no?
>>>>> IIUC then you simply do not remove the memory if you lose the race.
>>>>
>>>> The problem occurs when removing memory after an affinity change
>>>> references a node that was previously unreferenced. Other code
>>>> in 'kernel/mm/memory_hotplug.c' deals with initializing an empty
>>>> node when adding memory to a system. The 'removing memory' case is
>>>> specific to systems that perform LPM and allow device-tree changes.
>>>> The powerpc kernel does not have the option of accepting some PRRN
>>>> requests and accepting others. It must perform them all.
>>>
>>> I am sorry, but you are still too cryptic for me. Either there is a
>>> correctness issue and the the patch doesn't really fix anything or the
>>> final race doesn't make any difference and then the ppc code should be
>>> explicit about that. Checking the node inside the hotplug core code just
>>> looks as a wrong layer to mitigate an arch specific problem. I am not
>>> saying the patch is a no-go but if anything we want a big fat comment
>>> explaining how this is possible because right now it just points to an
>>> incorrect API usage.
>>>
>>> That being said, this sounds pretty much ppc specific problem and I
>>> would _prefer_ it to be handled there (along with a big fat comment of
>>> course).
>>
>> Let me try again. Regardless of the path to which we get to this condition,
>> we currently crash the kernel. This patch changes that to a WARN_ON notice
>> and continues executing the kernel without shutting down the system. I saw
>> the problem during powerpc testing, because that is the focus of my work.
>> There are other paths to this function besides powerpc. I feel that the
>> kernel should keep running instead of halting.
>
> This is still basically a hack to get around a known race. In itself this patch is still worth while in that we shouldn't crash the kernel on a null pointer dereference. However, I think the actual problem still needs to be addressed. We shouldn't run any PRRN events for the source system on the target after a migration. The device tree update should have taken care of telling us about new affinities and what not. Can we just throw out any queued PRRN events when we wake up on the target?
We are not talking about queued events provided on the source system, but about
new PRRN events sent by phyp to the kernel on the target system to update the
kernel state after migration. No way to predict the content.
>
> -Tyrel
>>
>> Regards,
>>
Michael
--
Michael W. Bringmann
Linux Technology Center
IBM Corporation
Tie-Line 363-5196
External: (512) 286-5196
Cell: (512) 466-0650
mwb@linux.vnet.ibm.com
^ permalink raw reply
* Re: [PATCH v03 3/5] migration/memory: Add hotplug READD_MULTIPLE
From: Michael Bringmann @ 2018-10-03 13:31 UTC (permalink / raw)
To: Tyrel Datwyler, linuxppc-dev; +Cc: Nathan Fontenot, Juliet Kim, Thomas Falcon
In-Reply-To: <460b921e-269b-c7d2-9a31-522a1674faec@linux.vnet.ibm.com>
On 10/02/2018 04:03 PM, Tyrel Datwyler wrote:
> On 10/01/2018 05:59 AM, Michael Bringmann wrote:
>> migration/memory: This patch adds a new pseries hotplug action
>> for CPU and memory operations, PSERIES_HP_ELOG_ACTION_READD_MULTIPLE.
>> This is a variant of the READD operation which performs the action
>> upon multiple instances of the resource at one time. The operation
>> is to be triggered by device-tree analysis of updates by RTAS events
>> analyzed by 'migation_store' during post-migration processing. It
>> will be used for memory updates, initially.
>>
>> Signed-off-by: Michael Bringmann <mwb@linux.vnet.ibm.com>
>> ---
>> arch/powerpc/include/asm/rtas.h | 1 +
>> arch/powerpc/mm/drmem.c | 1 +
>> 2 files changed, 2 insertions(+)
>>
>> diff --git a/arch/powerpc/include/asm/rtas.h b/arch/powerpc/include/asm/rtas.h
>> index 71e393c..e510d82 100644
>> --- a/arch/powerpc/include/asm/rtas.h
>> +++ b/arch/powerpc/include/asm/rtas.h
>> @@ -320,6 +320,7 @@ struct pseries_hp_errorlog {
>> #define PSERIES_HP_ELOG_ACTION_ADD 1
>> #define PSERIES_HP_ELOG_ACTION_REMOVE 2
>> #define PSERIES_HP_ELOG_ACTION_READD 3
>> +#define PSERIES_HP_ELOG_ACTION_READD_MULTIPLE 4
>
> I'm confused, you have only added a define and not the actual implementation. I really think this should be squashed into your 4th patch where the operation is actually implemented.
Okay.
>
>>
>> #define PSERIES_HP_ELOG_ID_DRC_NAME 1
>> #define PSERIES_HP_ELOG_ID_DRC_INDEX 2
>> diff --git a/arch/powerpc/mm/drmem.c b/arch/powerpc/mm/drmem.c
>> index fd2cae92..2228586 100644
>> --- a/arch/powerpc/mm/drmem.c
>> +++ b/arch/powerpc/mm/drmem.c
>> @@ -422,6 +422,7 @@ static void init_drmem_v2_lmbs(const __be32 *prop,
>>
>> lmb->aa_index = dr_cell.aa_index;
>> lmb->flags = dr_cell.flags;
>> + lmb->internal_flags = 0;
>
> And this should have been squashed into the previous patch where you added the internal_flags field to the lmb struct.
Okay.
>
> -Tyrel
>
>> }
>> }
>> }
>>
>
>
--
Michael W. Bringmann
Linux Technology Center
IBM Corporation
Tie-Line 363-5196
External: (512) 286-5196
Cell: (512) 466-0650
mwb@linux.vnet.ibm.com
^ permalink raw reply
* Re: [PATCH RFC] mm/memory_hotplug: Introduce memory block types
From: Vitaly Kuznetsov @ 2018-10-03 13:38 UTC (permalink / raw)
To: David Hildenbrand, Michal Hocko
Cc: Kate Stewart, Rich Felker, linux-ia64, linux-sh, Peter Zijlstra,
Dave Hansen, Heiko Carstens, linux-mm, Paul Mackerras,
H. Peter Anvin, Stephen Rothwell, Rashmica Gupta, Dan Williams,
linux-s390, Michael Neuling, Stephen Hemminger, Yoshinori Sato,
linux-acpi, Ingo Molnar, xen-devel, Len Brown, Pavel Tatashin,
Rob Herring, mike.travis@hpe.com, Haiyang Zhang,
Jonathan Neuschäfer, Nicholas Piggin, Martin Schwidefsky,
Jérôme Glisse, Mike Rapoport, Borislav Petkov,
Andy Lutomirski, Boris Ostrovsky, Andrew Morton, Oscar Salvador,
Juergen Gross, Tony Luck, Mathieu Malaterre, Greg Kroah-Hartman,
Rafael J. Wysocki, linux-kernel, Fenghua Yu,
Mauricio Faria de Oliveira, Thomas Gleixner, Philippe Ombredanne,
Joe Perches, devel, Joonsoo Kim, linuxppc-dev, Kirill A. Shutemov
In-Reply-To: <98fb8d65-b641-2225-f842-8804c6f79a06@redhat.com>
David Hildenbrand <david@redhat.com> writes:
> On 02/10/2018 15:47, Michal Hocko wrote:
...
>>
>> Why do you need a generic hotplug rule in the first place? Why don't you
>> simply provide different set of rules for different usecases? Let users
>> decide which usecase they prefer rather than try to be clever which
>> almost always hits weird corner cases.
>>
>
> Memory hotplug has to work as reliable as we can out of the box. Letting
> the user make simple decisions like "oh, I am on hyper-V, I want to
> online memory to the normal zone" does not feel right. But yes, we
> should definitely allow to make modifications.
Last time I was thinking about the imperfectness of the auto-online
solution we have and any other solution we're able to suggest an idea
came to my mind - what if we add an eBPF attach point to the
auto-onlining mechanism effecively offloading decision-making to
userspace. We'll of couse need to provide all required data (e.g. how
memory blocks are aligned with physical DIMMs as it makes no sense to
online part of DIMM as normal and the rest as movable as it's going to
be impossible to unplug such DIMM anyways).
--
Vitaly
^ permalink raw reply
* Re: [PATCH RFC] mm/memory_hotplug: Introduce memory block types
From: Michal Hocko @ 2018-10-03 13:44 UTC (permalink / raw)
To: Vitaly Kuznetsov
Cc: Kate Stewart, Rich Felker, linux-ia64, linux-sh, Peter Zijlstra,
Dave Hansen, Heiko Carstens, linux-mm, Paul Mackerras,
H. Peter Anvin, Stephen Rothwell, Rashmica Gupta, Dan Williams,
linux-s390, Michael Neuling, Stephen Hemminger, Yoshinori Sato,
David Hildenbrand, linux-acpi, Ingo Molnar, xen-devel, Len Brown,
Pavel Tatashin, Rob Herring, mike.travis@hpe.com, Haiyang Zhang,
Jonathan Neuschäfer, Nicholas Piggin, Martin Schwidefsky,
Jérôme Glisse, Mike Rapoport, Borislav Petkov,
Andy Lutomirski, Boris Ostrovsky, Andrew Morton, Oscar Salvador,
Juergen Gross, Tony Luck, Mathieu Malaterre, Greg Kroah-Hartman,
Rafael J. Wysocki, linux-kernel, Fenghua Yu,
Mauricio Faria de Oliveira, Thomas Gleixner, Philippe Ombredanne,
Joe Perches, devel, Joonsoo Kim, linuxppc-dev, Kirill A. Shutemov
In-Reply-To: <8736tndubn.fsf@vitty.brq.redhat.com>
On Wed 03-10-18 15:38:04, Vitaly Kuznetsov wrote:
> David Hildenbrand <david@redhat.com> writes:
>
> > On 02/10/2018 15:47, Michal Hocko wrote:
> ...
> >>
> >> Why do you need a generic hotplug rule in the first place? Why don't you
> >> simply provide different set of rules for different usecases? Let users
> >> decide which usecase they prefer rather than try to be clever which
> >> almost always hits weird corner cases.
> >>
> >
> > Memory hotplug has to work as reliable as we can out of the box. Letting
> > the user make simple decisions like "oh, I am on hyper-V, I want to
> > online memory to the normal zone" does not feel right. But yes, we
> > should definitely allow to make modifications.
>
> Last time I was thinking about the imperfectness of the auto-online
> solution we have and any other solution we're able to suggest an idea
> came to my mind - what if we add an eBPF attach point to the
> auto-onlining mechanism effecively offloading decision-making to
> userspace. We'll of couse need to provide all required data (e.g. how
> memory blocks are aligned with physical DIMMs as it makes no sense to
> online part of DIMM as normal and the rest as movable as it's going to
> be impossible to unplug such DIMM anyways).
And how does that differ from the notification mechanism we have? Just
by not relying on the process scheduling? If yes then this revolves
around the implementation detail that you care about time-to-hot-add
vs. time-to-online. And that is a solveable problem - just allocate
memmaps from the hot-added memory.
As David said some of the memory cannot be onlined without further steps
(e.g. when it is standby as David called it) and then I fail to see how
eBPF help in any way.
--
Michal Hocko
SUSE Labs
^ permalink raw reply
* Re: [PATCH RFC] mm/memory_hotplug: Introduce memory block types
From: Vitaly Kuznetsov @ 2018-10-03 13:52 UTC (permalink / raw)
To: Michal Hocko
Cc: Kate Stewart, Rich Felker, linux-ia64, linux-sh, Peter Zijlstra,
Dave Hansen, Heiko Carstens, linux-mm, Paul Mackerras,
H. Peter Anvin, Stephen Rothwell, Rashmica Gupta, Dan Williams,
linux-s390, Michael Neuling, Stephen Hemminger, Yoshinori Sato,
David Hildenbrand, linux-acpi, Ingo Molnar, xen-devel, Len Brown,
Pavel Tatashin, Rob Herring, mike.travis@hpe.com, Haiyang Zhang,
Jonathan Neuschäfer, Nicholas Piggin, Martin Schwidefsky,
Jérôme Glisse, Mike Rapoport, Borislav Petkov,
Andy Lutomirski, Boris Ostrovsky, Andrew Morton, Oscar Salvador,
Juergen Gross, Tony Luck, Mathieu Malaterre, Greg Kroah-Hartman,
Rafael J. Wysocki, linux-kernel, Fenghua Yu,
Mauricio Faria de Oliveira, Thomas Gleixner, Philippe Ombredanne,
Joe Perches, devel, Joonsoo Kim, linuxppc-dev, Kirill A. Shutemov
In-Reply-To: <20181003134444.GH4714@dhcp22.suse.cz>
Michal Hocko <mhocko@kernel.org> writes:
> On Wed 03-10-18 15:38:04, Vitaly Kuznetsov wrote:
>> David Hildenbrand <david@redhat.com> writes:
>>
>> > On 02/10/2018 15:47, Michal Hocko wrote:
>> ...
>> >>
>> >> Why do you need a generic hotplug rule in the first place? Why don't you
>> >> simply provide different set of rules for different usecases? Let users
>> >> decide which usecase they prefer rather than try to be clever which
>> >> almost always hits weird corner cases.
>> >>
>> >
>> > Memory hotplug has to work as reliable as we can out of the box. Letting
>> > the user make simple decisions like "oh, I am on hyper-V, I want to
>> > online memory to the normal zone" does not feel right. But yes, we
>> > should definitely allow to make modifications.
>>
>> Last time I was thinking about the imperfectness of the auto-online
>> solution we have and any other solution we're able to suggest an idea
>> came to my mind - what if we add an eBPF attach point to the
>> auto-onlining mechanism effecively offloading decision-making to
>> userspace. We'll of couse need to provide all required data (e.g. how
>> memory blocks are aligned with physical DIMMs as it makes no sense to
>> online part of DIMM as normal and the rest as movable as it's going to
>> be impossible to unplug such DIMM anyways).
>
> And how does that differ from the notification mechanism we have? Just
> by not relying on the process scheduling? If yes then this revolves
> around the implementation detail that you care about time-to-hot-add
> vs. time-to-online. And that is a solveable problem - just allocate
> memmaps from the hot-added memory.
It is more than just memmaps (e.g. forking udev process doing memory
onlining also needs memory) but yes, the main idea is to make the
onlining synchronous with hotplug.
>
> As David said some of the memory cannot be onlined without further steps
> (e.g. when it is standby as David called it) and then I fail to see how
> eBPF help in any way.
and also, we can fight till the end of days here trying to come up with
an onlining solution which would work for everyone and eBPF would move
this decision to distro level.
--
Vitaly
^ permalink raw reply
* Re: [PATCH RFC] mm/memory_hotplug: Introduce memory block types
From: Michal Hocko @ 2018-10-03 13:54 UTC (permalink / raw)
To: David Hildenbrand
Cc: Kate Stewart, Rich Felker, linux-ia64, linux-sh, Peter Zijlstra,
Dave Hansen, Heiko Carstens, linux-mm, Pavel Tatashin,
Paul Mackerras, H. Peter Anvin, Rashmica Gupta, K. Y. Srinivasan,
Boris Ostrovsky, linux-s390, Michael Neuling, Stephen Hemminger,
Yoshinori Sato, linux-acpi, Ingo Molnar, xen-devel, Rob Herring,
Len Brown, Fenghua Yu, Stephen Rothwell, mike.travis@hpe.com,
Haiyang Zhang, Dan Williams, Jonathan Neuschäfer,
Nicholas Piggin, Joe Perches, Jérôme Glisse,
Mike Rapoport, Borislav Petkov, Andy Lutomirski, Thomas Gleixner,
Joonsoo Kim, Oscar Salvador, Juergen Gross, Tony Luck,
Mathieu Malaterre, Greg Kroah-Hartman, Rafael J. Wysocki,
linux-kernel, Mauricio Faria de Oliveira, Philippe Ombredanne,
Martin Schwidefsky, devel, Andrew Morton, linuxppc-dev,
Kirill A. Shutemov
In-Reply-To: <98fb8d65-b641-2225-f842-8804c6f79a06@redhat.com>
On Tue 02-10-18 17:25:19, David Hildenbrand wrote:
> On 02/10/2018 15:47, Michal Hocko wrote:
[...]
> > Zone imbalance is an inherent problem of the highmem zone. It is
> > essentially the highmem zone we all loved so much back in 32b days.
> > Yes the movable zone doesn't have any addressing limitations so it is a
> > bit more relaxed but considering the hotplug scenarios I have seen so
> > far people just want to have full NUMA nodes movable to allow replacing
> > DIMMs. And then we are back to square one and the zone imbalance issue.
> > You have those regardless where memmaps are allocated from.
>
> Unfortunately yes. And things get more complicated as you are adding a
> whole DIMMs and get notifications in the granularity of memory blocks.
> Usually you are not interested in onlining any memory block of that DIMM
> as MOVABLE as soon as you would have to online one memory block of that
> DIMM as NORMAL - because that can already block the whole DIMM.
For the purpose of the hotremove, yes. But as Dave has noted people are
(ab)using zone movable for other purposes - e.g. large pages.
[...]
> > Then the immediate question would be why to use memory hotplug for that
> > at all? Why don't you simply start with a huge pre-allocated physical
> > address space and balloon memory in an out per demand. Why do you want
> > to inject new memory during the runtime?
>
> Let's assume you have a guest with 20GB size and eventually want to
> allow to grow it to 4TB. You would have to allocate metadata for 4TB
> right from the beginning. That's definitely now what we want. That is
> why memory hotplug is used by e.g. XEN or Hyper-V. With Hyper-V, the
> hypervisor even tells you at which places additional memory has been
> made available.
Then you have to live with the fact that your hot added memory will be
self hosted and find a way for ballooning to work with that. The price
would be that some part of the memory is not really balloonable in the
end.
> >> 1. is a reason why distributions usually don't configure
> >> "MEMORY_HOTPLUG_DEFAULT_ONLINE", because you really want the option for
> >> MOVABLE zone. That however implies, that e.g. for x86, you have to
> >> handle all new memory in user space, especially also HyperV memory.
> >> There, you then have to check for things like "isHyperV()" to decide
> >> "oh, yes, this should definitely not go to the MOVABLE zone".
> >
> > Why do you need a generic hotplug rule in the first place? Why don't you
> > simply provide different set of rules for different usecases? Let users
> > decide which usecase they prefer rather than try to be clever which
> > almost always hits weird corner cases.
> >
>
> Memory hotplug has to work as reliable as we can out of the box. Letting
> the user make simple decisions like "oh, I am on hyper-V, I want to
> online memory to the normal zone" does not feel right.
Users usually know what is their usecase and then it is just a matter of
plumbing (e.g. distribution can provide proper tools to deploy those
usecases) to chose the right and for user obscure way to make it work.
> But yes, we
> should definitely allow to make modifications. So some sane default rule
> + possible modification is usually a good idea.
>
> I think Dave has a point with using MOVABLE for huge page use cases. And
> there might be other corner cases as you correctly state.
>
> I wonder if this patch itself minus modifying online/offline might make
> sense. We can then implement simple rules in user space
>
> if (normal) {
> /* customers expect hotplugged DIMMs to be unpluggable */
> online_movable();
> } else if (paravirt) {
> /* paravirt memory should as default always go to the NORMAL */
> online();
> } else {
> /* standby memory will never get onlined automatically */
> }
>
> Compared to having to guess what is to be done (isKVM(), isHyperV,
> isS390 ...) and failing once this is no longer unique (e.g. virtio-mem
> and ACPI support for x86 KVM).
I am worried that exporing a type will just push us even further to the
corner. The current design is really simple and 2 stage and that is good
because it allows for very different usecases. The more specific the API
be the more likely we are going to hit "I haven't even dreamed somebody
would be using hotplug for this thing". And I would bet this will happen
sooner or later.
Just look at how the whole auto onlining screwed the API to workaround
an implementation detail. It has created a one purpose behavior that
doesn't suite many usecases. Yet we have to live with that because
somebody really relies on it. Let's not repeat same errors.
--
Michal Hocko
SUSE Labs
^ permalink raw reply
* Re: [PATCH RFC] mm/memory_hotplug: Introduce memory block types
From: Dave Hansen @ 2018-10-03 14:07 UTC (permalink / raw)
To: Vitaly Kuznetsov, Michal Hocko
Cc: Kate Stewart, Rich Felker, linux-ia64, linux-sh, Peter Zijlstra,
Heiko Carstens, linux-mm, Paul Mackerras, H. Peter Anvin,
Stephen Rothwell, Rashmica Gupta, Dan Williams, linux-s390,
Michael Neuling, Stephen Hemminger, Yoshinori Sato,
David Hildenbrand, linux-acpi, Ingo Molnar, xen-devel, Len Brown,
Pavel Tatashin, Rob Herring, mike.travis@hpe.com, Haiyang Zhang,
Jonathan Neuschäfer, Nicholas Piggin, Martin Schwidefsky,
Jérôme Glisse, Mike Rapoport, Borislav Petkov,
Andy Lutomirski, Boris Ostrovsky, Andrew Morton, Oscar Salvador,
Juergen Gross, Tony Luck, Mathieu Malaterre, Greg Kroah-Hartman,
Rafael J. Wysocki, linux-kernel, Fenghua Yu,
Mauricio Faria de Oliveira, Thomas Gleixner, Philippe Ombredanne,
Joe Perches, devel, Joonsoo Kim, linuxppc-dev, Kirill A. Shutemov
In-Reply-To: <87zhvvcf3b.fsf@vitty.brq.redhat.com>
On 10/03/2018 06:52 AM, Vitaly Kuznetsov wrote:
> It is more than just memmaps (e.g. forking udev process doing memory
> onlining also needs memory) but yes, the main idea is to make the
> onlining synchronous with hotplug.
That's a good theoretical concern.
But, is it a problem we need to solve in practice?
^ permalink raw reply
* Re: [PATCH v03 4/5] migration/memory: Evaluate LMB assoc changes
From: Michael Bringmann @ 2018-10-03 14:21 UTC (permalink / raw)
To: Tyrel Datwyler, linuxppc-dev; +Cc: Nathan Fontenot, Juliet Kim, Thomas Falcon
In-Reply-To: <e5e6c3a5-5bc6-4f64-a045-312b9892e59f@linux.vnet.ibm.com>
On 10/02/2018 04:08 PM, Tyrel Datwyler wrote:
> On 10/01/2018 06:00 AM, Michael Bringmann wrote:
>> migration/memory: This patch adds code that recognizes changes to
>> the associativity of memory blocks described by the device-tree
>> properties in order to drive equivalent 'hotplug' operations to
>> update local and general kernel data structures to reflect those
>> changes. These differences may include:
>>
>> * Evaluate 'ibm,dynamic-memory' properties when processing the
>> updated device-tree properties of the system during Post Migration
>> events (migration_store). The new functionality looks for changes
>> to the aa_index values for each drc_index/LMB to identify any memory
>> blocks that should be readded.
>>
>> * In an LPAR migration scenario, the "ibm,associativity-lookup-arrays"
>> property may change. In the event that a row of the array differs,
>> locate all assigned memory blocks with that 'aa_index' and 're-add'
>> them to the system memory block data structures. In the process of
>> the 're-add', the system routines will update the corresponding entry
>> for the memory in the LMB structures and any other relevant kernel
>> data structures.
>>
>> A number of previous extensions made to the DRMEM code for scanning
>> device-tree properties and creating LMB arrays are used here to
>> ensure that the resulting code is simpler and more usable:
>>
>> * Use new paired list iterator for the DRMEM LMB info arrays to find
>> differences in old and new versions of properties.
>> * Use new iterator for copies of the DRMEM info arrays to evaluate
>> completely new structures.
>> * Combine common code for parsing and evaluating memory description
>> properties based on the DRMEM LMB array model to greatly simplify
>> extension from the older property 'ibm,dynamic-memory' to the new
>> property model of 'ibm,dynamic-memory-v2'.
>>
>> Signed-off-by: Michael Bringmann <mwb@linux.vnet.ibm.com>
>> ---
>> Changes in v03:
>> -- Modify the code that parses the memory affinity attributes to
>> mark relevant DRMEM LMB array entries using the internal_flags
>> mechanism instead of generate unique hotplug actions for each
>> memory block to be readded. The change is intended to both
>> simplify the code, and to require fewer resources on systems
>> with huge amounts of memory.
>> -- Save up notice about any all LMB entries until the end of the
>> 'migration_store' operation at which point a single action is
>> queued to scan the entire DRMEM array.
>> -- Add READD_MULTIPLE function for memory that scans the DRMEM
>> array to identify multiple entries that were marked previously.
>> The corresponding memory blocks are to be readded to the system
>> to update relevant data structures outside of the powerpc-
>> specific code.
>> -- Change dlpar_memory_pmt_changes_action to directly queue worker
>> to pseries work queue.
>> ---
>> arch/powerpc/platforms/pseries/hotplug-memory.c | 220 +++++++++++++++++++----
>> arch/powerpc/platforms/pseries/mobility.c | 4
>> arch/powerpc/platforms/pseries/pseries.h | 4
>> 3 files changed, 194 insertions(+), 34 deletions(-)
>>
>> diff --git a/arch/powerpc/platforms/pseries/hotplug-memory.c b/arch/powerpc/platforms/pseries/hotplug-memory.c
>> index c1578f5..68bde2e 100644
>> --- a/arch/powerpc/platforms/pseries/hotplug-memory.c
>> +++ b/arch/powerpc/platforms/pseries/hotplug-memory.c
>> @@ -561,8 +561,11 @@ static int dlpar_memory_readd_by_index(u32 drc_index)
>> }
>> }
>>
>> - if (!lmb_found)
>> - rc = -EINVAL;
>> + if (!lmb_found) {
>> + pr_info("Failed to update memory for drc index %lx\n",
>> + (unsigned long) drc_index);
>> + return -EINVAL;
>> + }
>>
>> if (rc)
>> pr_info("Failed to update memory at %llx\n",
>> @@ -573,6 +576,30 @@ static int dlpar_memory_readd_by_index(u32 drc_index)
>> return rc;
>> }
>>
>> +static int dlpar_memory_readd_multiple(void)
>> +{
>> + struct drmem_lmb *lmb;
>> + int rc;
>> +
>> + pr_info("Attempting to update multiple LMBs\n");
>> +
>> + for_each_drmem_lmb(lmb) {
>> + if (drmem_lmb_update(lmb)) {
>> + rc = dlpar_remove_lmb(lmb);
>> +
>> + if (!rc) {
>> + rc = dlpar_add_lmb(lmb);
>> + if (rc)
>> + dlpar_release_drc(lmb->drc_index);
>> + }
>> +
>> + drmem_remove_lmb_update(lmb);
>> + }
>> + }
>> +
>> + return rc;
>> +}
>> +
>
> Actually, in retrospect this function should have been independently implemented in your 3rd patch where you define the flag. So, disregard moving the define into this patch and instead move the function implementation into patch 3 where the commit message talks about adding this operation.
Okay. It won't hurt to decouple these changes.
>
>> static int dlpar_memory_remove_by_ic(u32 lmbs_to_remove, u32 drc_index)
>> {
>> struct drmem_lmb *lmb, *start_lmb, *end_lmb;
>> @@ -673,6 +700,10 @@ static int dlpar_memory_readd_by_index(u32 drc_index)
>> {
>> return -EOPNOTSUPP;
>> }
>> +static int dlpar_memory_readd_multiple(void)
>> +{
>> + return -EOPNOTSUPP;
>> +}
>
> Same as above commit obviously.
Okay.
>
>>
>> static int dlpar_memory_remove_by_ic(u32 lmbs_to_remove, u32 drc_index)
>> {
>> @@ -952,6 +983,9 @@ int dlpar_memory(struct pseries_hp_errorlog *hp_elog)
>> drc_index = hp_elog->_drc_u.drc_index;
>> rc = dlpar_memory_readd_by_index(drc_index);
>> break;
>> + case PSERIES_HP_ELOG_ACTION_READD_MULTIPLE:
>> + rc = dlpar_memory_readd_multiple();
>> + break;
>
> And this as well.
Okay.
>
> -Tyrel
Michael
>
>> default:
>> pr_err("Invalid action (%d) specified\n", hp_elog->action);
>> rc = -EINVAL;
>> @@ -994,13 +1028,43 @@ static int pseries_add_mem_node(struct device_node *np)
>> return (ret < 0) ? -EINVAL : 0;
>> }
>>
>> -static int pseries_update_drconf_memory(struct of_reconfig_data *pr)
>> +static int pmt_changes = 0;
>> +
>> +void dlpar_memory_pmt_changes_set(void)
>> +{
>> + pmt_changes = 1;
>> +}
>> +
>> +void dlpar_memory_pmt_changes_clear(void)
>> +{
>> + pmt_changes = 0;
>> +}
>> +
>> +int dlpar_memory_pmt_changes(void)
>> +{
>> + return pmt_changes;
>> +}
>> +
>> +void dlpar_memory_pmt_changes_action(void)
>> +{
>> + if (dlpar_memory_pmt_changes()) {
>> + struct pseries_hp_errorlog hp_errlog;
>> +
>> + hp_errlog.resource = PSERIES_HP_ELOG_RESOURCE_MEM;
>> + hp_errlog.action = PSERIES_HP_ELOG_ACTION_READD_MULTIPLE;
>> + hp_errlog.id_type = 0;
>> +
>> + queue_hotplug_event(&hp_errlog, NULL, NULL);
>> +
>> + dlpar_memory_pmt_changes_clear();
>> + }
>> +}
>> +
>> +static int pseries_update_drconf_memory(struct drmem_lmb_info *new_dinfo)
>> {
>> - struct of_drconf_cell_v1 *new_drmem, *old_drmem;
>> + struct drmem_lmb *old_lmb, *new_lmb;
>> unsigned long memblock_size;
>> - u32 entries;
>> - __be32 *p;
>> - int i, rc = -EINVAL;
>> + int rc = 0;
>>
>> if (rtas_hp_event)
>> return 0;
>> @@ -1009,42 +1073,122 @@ static int pseries_update_drconf_memory(struct of_reconfig_data *pr)
>> if (!memblock_size)
>> return -EINVAL;
>>
>> - p = (__be32 *) pr->old_prop->value;
>> - if (!p)
>> - return -EINVAL;
>> -
>> - /* The first int of the property is the number of lmb's described
>> - * by the property. This is followed by an array of of_drconf_cell
>> - * entries. Get the number of entries and skip to the array of
>> - * of_drconf_cell's.
>> - */
>> - entries = be32_to_cpu(*p++);
>> - old_drmem = (struct of_drconf_cell_v1 *)p;
>> + /* Arrays should have the same size and DRC indexes */
>> + for_each_pair_dinfo_lmb(drmem_info, old_lmb, new_dinfo, new_lmb) {
>>
>> - p = (__be32 *)pr->prop->value;
>> - p++;
>> - new_drmem = (struct of_drconf_cell_v1 *)p;
>> + if (new_lmb->drc_index != old_lmb->drc_index)
>> + continue;
>>
>> - for (i = 0; i < entries; i++) {
>> - if ((be32_to_cpu(old_drmem[i].flags) & DRCONF_MEM_ASSIGNED) &&
>> - (!(be32_to_cpu(new_drmem[i].flags) & DRCONF_MEM_ASSIGNED))) {
>> + if ((old_lmb->flags & DRCONF_MEM_ASSIGNED) &&
>> + (!(new_lmb->flags & DRCONF_MEM_ASSIGNED))) {
>> rc = pseries_remove_memblock(
>> - be64_to_cpu(old_drmem[i].base_addr),
>> - memblock_size);
>> + old_lmb->base_addr, memblock_size);
>> break;
>> - } else if ((!(be32_to_cpu(old_drmem[i].flags) &
>> - DRCONF_MEM_ASSIGNED)) &&
>> - (be32_to_cpu(new_drmem[i].flags) &
>> - DRCONF_MEM_ASSIGNED)) {
>> - rc = memblock_add(be64_to_cpu(old_drmem[i].base_addr),
>> - memblock_size);
>> + } else if ((!(old_lmb->flags & DRCONF_MEM_ASSIGNED)) &&
>> + (new_lmb->flags & DRCONF_MEM_ASSIGNED)) {
>> + rc = memblock_add(old_lmb->base_addr,
>> + memblock_size);
>> rc = (rc < 0) ? -EINVAL : 0;
>> break;
>> + } else if ((old_lmb->aa_index != new_lmb->aa_index) &&
>> + (new_lmb->flags & DRCONF_MEM_ASSIGNED)) {
>> + drmem_mark_lmb_update(old_lmb);
>> + dlpar_memory_pmt_changes_set();
>> }
>> }
>> return rc;
>> }
>>
>> +static void pseries_update_ala_memory_aai(int aa_index)
>> +{
>> + struct drmem_lmb *lmb;
>> +
>> + /* Readd all LMBs which were previously using the
>> + * specified aa_index value.
>> + */
>> + for_each_drmem_lmb(lmb) {
>> + if ((lmb->aa_index == aa_index) &&
>> + (lmb->flags & DRCONF_MEM_ASSIGNED)) {
>> + drmem_mark_lmb_update(lmb);
>> + dlpar_memory_pmt_changes_set();
>> + }
>> + }
>> +}
>> +
>> +struct assoc_arrays {
>> + u32 n_arrays;
>> + u32 array_sz;
>> + const __be32 *arrays;
>> +};
>> +
>> +static int pseries_update_ala_memory(struct of_reconfig_data *pr)
>> +{
>> + struct assoc_arrays new_ala, old_ala;
>> + __be32 *p;
>> + int i, lim;
>> +
>> + if (rtas_hp_event)
>> + return 0;
>> +
>> + /*
>> + * The layout of the ibm,associativity-lookup-arrays
>> + * property is a number N indicating the number of
>> + * associativity arrays, followed by a number M
>> + * indicating the size of each associativity array,
>> + * followed by a list of N associativity arrays.
>> + */
>> +
>> + p = (__be32 *) pr->old_prop->value;
>> + if (!p)
>> + return -EINVAL;
>> + old_ala.n_arrays = of_read_number(p++, 1);
>> + old_ala.array_sz = of_read_number(p++, 1);
>> + old_ala.arrays = p;
>> +
>> + p = (__be32 *) pr->prop->value;
>> + if (!p)
>> + return -EINVAL;
>> + new_ala.n_arrays = of_read_number(p++, 1);
>> + new_ala.array_sz = of_read_number(p++, 1);
>> + new_ala.arrays = p;
>> +
>> + lim = (new_ala.n_arrays > old_ala.n_arrays) ? old_ala.n_arrays :
>> + new_ala.n_arrays;
>> +
>> + if (old_ala.array_sz == new_ala.array_sz) {
>> +
>> + /* Reset any entries where the old and new rows
>> + * the array have changed.
>> + */
>> + for (i = 0; i < lim; i++) {
>> + int index = (i * new_ala.array_sz);
>> +
>> + if (!memcmp(&old_ala.arrays[index],
>> + &new_ala.arrays[index],
>> + new_ala.array_sz))
>> + continue;
>> +
>> + pseries_update_ala_memory_aai(i);
>> + }
>> +
>> + /* Reset any entries representing the extra rows.
>> + * There shouldn't be any, but just in case ...
>> + */
>> + for (i = lim; i < new_ala.n_arrays; i++)
>> + pseries_update_ala_memory_aai(i);
>> +
>> + } else {
>> + /* Update all entries representing these rows;
>> + * as all rows have different sizes, none can
>> + * have equivalent values.
>> + */
>> + for (i = 0; i < lim; i++)
>> + pseries_update_ala_memory_aai(i);
>> + }
>> +
>> + return 0;
>> +}
>> +
>> static int pseries_memory_notifier(struct notifier_block *nb,
>> unsigned long action, void *data)
>> {
>> @@ -1059,8 +1203,16 @@ static int pseries_memory_notifier(struct notifier_block *nb,
>> err = pseries_remove_mem_node(rd->dn);
>> break;
>> case OF_RECONFIG_UPDATE_PROPERTY:
>> - if (!strcmp(rd->prop->name, "ibm,dynamic-memory"))
>> - err = pseries_update_drconf_memory(rd);
>> + if (!strcmp(rd->prop->name, "ibm,dynamic-memory")) {
>> + struct drmem_lmb_info *dinfo =
>> + drmem_lmbs_init(rd->prop);
>> + if (!dinfo)
>> + return -EINVAL;
>> + err = pseries_update_drconf_memory(dinfo);
>> + drmem_lmbs_free(dinfo);
>> + } else if (!strcmp(rd->prop->name,
>> + "ibm,associativity-lookup-arrays"))
>> + err = pseries_update_ala_memory(rd);
>> break;
>> }
>> return notifier_from_errno(err);
>> diff --git a/arch/powerpc/platforms/pseries/mobility.c b/arch/powerpc/platforms/pseries/mobility.c
>> index 23fb9ac..6c0456f 100644
>> --- a/arch/powerpc/platforms/pseries/mobility.c
>> +++ b/arch/powerpc/platforms/pseries/mobility.c
>> @@ -383,6 +383,10 @@ static ssize_t migration_store(struct class *class,
>> return rc;
>>
>> post_mobility_fixup();
>> +
>> + /* Apply any necessary changes identified during fixup */
>> + dlpar_memory_pmt_changes_action();
>> +
>> return count;
>> }
>>
>> diff --git a/arch/powerpc/platforms/pseries/pseries.h b/arch/powerpc/platforms/pseries/pseries.h
>> index 60db2ee..b238634 100644
>> --- a/arch/powerpc/platforms/pseries/pseries.h
>> +++ b/arch/powerpc/platforms/pseries/pseries.h
>> @@ -69,6 +69,10 @@ static inline int dlpar_memory(struct pseries_hp_errorlog *hp_elog)
>> return -EOPNOTSUPP;
>> }
>> #endif
>> +void dlpar_memory_pmt_changes_set(void);
>> +void dlpar_memory_pmt_changes_clear(void);
>> +int dlpar_memory_pmt_changes(void);
>> +void dlpar_memory_pmt_changes_action(void);
>>
>> #ifdef CONFIG_HOTPLUG_CPU
>> int dlpar_cpu(struct pseries_hp_errorlog *hp_elog);
>>
>
>
--
Michael W. Bringmann
Linux Technology Center
IBM Corporation
Tie-Line 363-5196
External: (512) 286-5196
Cell: (512) 466-0650
mwb@linux.vnet.ibm.com
^ permalink raw reply
* Re: [PATCH RFC] mm/memory_hotplug: Introduce memory block types
From: Michal Hocko @ 2018-10-03 14:24 UTC (permalink / raw)
To: Vitaly Kuznetsov
Cc: Kate Stewart, Rich Felker, linux-ia64, linux-sh, Peter Zijlstra,
Dave Hansen, Heiko Carstens, linux-mm, Paul Mackerras,
H. Peter Anvin, Stephen Rothwell, Rashmica Gupta, Dan Williams,
linux-s390, Michael Neuling, Stephen Hemminger, Yoshinori Sato,
David Hildenbrand, linux-acpi, Ingo Molnar, xen-devel, Len Brown,
Pavel Tatashin, Rob Herring, mike.travis@hpe.com, Haiyang Zhang,
Jonathan Neuschäfer, Nicholas Piggin, Martin Schwidefsky,
Jérôme Glisse, Mike Rapoport, Borislav Petkov,
Andy Lutomirski, Boris Ostrovsky, Andrew Morton, Oscar Salvador,
Juergen Gross, Tony Luck, Mathieu Malaterre, Greg Kroah-Hartman,
Rafael J. Wysocki, linux-kernel, Fenghua Yu,
Mauricio Faria de Oliveira, Thomas Gleixner, Philippe Ombredanne,
Joe Perches, devel, Joonsoo Kim, linuxppc-dev, Kirill A. Shutemov
In-Reply-To: <87zhvvcf3b.fsf@vitty.brq.redhat.com>
On Wed 03-10-18 15:52:24, Vitaly Kuznetsov wrote:
[...]
> > As David said some of the memory cannot be onlined without further steps
> > (e.g. when it is standby as David called it) and then I fail to see how
> > eBPF help in any way.
>
> and also, we can fight till the end of days here trying to come up with
> an onlining solution which would work for everyone and eBPF would move
> this decision to distro level.
The point is that there is _no_ general onlining solution. This is
basically policy which belongs to the userspace.
--
Michal Hocko
SUSE Labs
^ permalink raw reply
* RE: [PATCH v3 3/6] drivers: clk-qoriq: increase array size of cmux_to_group
From: Vabhav Sharma @ 2018-10-03 14:25 UTC (permalink / raw)
To: Stephen Boyd, arnd@arndb.de, catalin.marinas@arm.com,
devicetree@vger.kernel.org, gregkh@linuxfoundation.org,
kstewart@linuxfoundation.org,
linux-arm-kernel@lists.infradead.org, linux-clk@vger.kernel.org,
linux-kernel-owner@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-pm@vger.kernel.org, linuxppc-dev@lists.ozlabs.org,
mark.rutland@arm.com, mturquette@baylibre.com, oss@buserror.net,
rjw@rjwysocki.net, robh+dt@kernel.org, sudeep.holla@arm.com,
viresh.kumar@linaro.org, will.deacon@arm.com,
yamada.masahiro@socionext.com
Cc: Udit Kumar, Yogesh Narayan Gaur, linux@armlinux.org.uk,
Varun Sethi
In-Reply-To: <153843144041.119890.5558230475983572666@swboyd.mtv.corp.google.com>
> -----Original Message-----
> From: Stephen Boyd <sboyd@kernel.org>
> Sent: Tuesday, October 2, 2018 3:34 AM
> To: Vabhav Sharma <vabhav.sharma@nxp.com>; arnd@arndb.de;
> catalin.marinas@arm.com; devicetree@vger.kernel.org;
> gregkh@linuxfoundation.org; kstewart@linuxfoundation.org; linux-arm-
> kernel@lists.infradead.org; linux-clk@vger.kernel.org; linux-kernel-
> owner@vger.kernel.org; linux-kernel@vger.kernel.org; linux-
> pm@vger.kernel.org; linuxppc-dev@lists.ozlabs.org; mark.rutland@arm.com;
> mturquette@baylibre.com; oss@buserror.net; rjw@rjwysocki.net;
> robh+dt@kernel.org; sudeep.holla@arm.com; viresh.kumar@linaro.org;
> will.deacon@arm.com; yamada.masahiro@socionext.com
> Cc: linux@armlinux.org.uk; Varun Sethi <V.Sethi@nxp.com>; Udit Kumar
> <udit.kumar@nxp.com>; Yogesh Narayan Gaur
> <yogeshnarayan.gaur@nxp.com>; Vabhav Sharma <vabhav.sharma@nxp.com>
> Subject: Re: [PATCH v3 3/6] drivers: clk-qoriq: increase array size of
> cmux_to_group
>
> Subject should be "clk: qoriq: increase array size ..."
Ok
^ permalink raw reply
* RE: [PATCH v3 4/6] drivers: clk-qoriq: Add clockgen support for lx2160a
From: Vabhav Sharma @ 2018-10-03 14:25 UTC (permalink / raw)
To: Stephen Boyd, arnd@arndb.de, catalin.marinas@arm.com,
devicetree@vger.kernel.org, gregkh@linuxfoundation.org,
kstewart@linuxfoundation.org,
linux-arm-kernel@lists.infradead.org, linux-clk@vger.kernel.org,
linux-kernel-owner@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-pm@vger.kernel.org, linuxppc-dev@lists.ozlabs.org,
mark.rutland@arm.com, mturquette@baylibre.com, oss@buserror.net,
rjw@rjwysocki.net, robh+dt@kernel.org, sudeep.holla@arm.com,
viresh.kumar@linaro.org, will.deacon@arm.com,
yamada.masahiro@socionext.com
Cc: Andy Tang, Udit Kumar, Yogesh Narayan Gaur, linux@armlinux.org.uk,
Varun Sethi
In-Reply-To: <153843146463.119890.6551025813220021124@swboyd.mtv.corp.google.com>
> -----Original Message-----
> From: devicetree-owner@vger.kernel.org <devicetree-owner@vger.kernel.org>
> On Behalf Of Stephen Boyd
> Sent: Tuesday, October 2, 2018 3:34 AM
> To: Vabhav Sharma <vabhav.sharma@nxp.com>; arnd@arndb.de;
> catalin.marinas@arm.com; devicetree@vger.kernel.org;
> gregkh@linuxfoundation.org; kstewart@linuxfoundation.org; linux-arm-
> kernel@lists.infradead.org; linux-clk@vger.kernel.org; linux-kernel-
> owner@vger.kernel.org; linux-kernel@vger.kernel.org; linux-
> pm@vger.kernel.org; linuxppc-dev@lists.ozlabs.org; mark.rutland@arm.com;
> mturquette@baylibre.com; oss@buserror.net; rjw@rjwysocki.net;
> robh+dt@kernel.org; sudeep.holla@arm.com; viresh.kumar@linaro.org;
> will.deacon@arm.com; yamada.masahiro@socionext.com
> Cc: linux@armlinux.org.uk; Varun Sethi <V.Sethi@nxp.com>; Udit Kumar
> <udit.kumar@nxp.com>; Yogesh Narayan Gaur
> <yogeshnarayan.gaur@nxp.com>; Andy Tang <andy.tang@nxp.com>; Vabhav
> Sharma <vabhav.sharma@nxp.com>
> Subject: Re: [PATCH v3 4/6] drivers: clk-qoriq: Add clockgen support for lx2160a
>
> Same subject comment.
Ok
>
> Quoting Vabhav Sharma (2018-09-23 17:08:59)
> > From: Yogesh Gaur <yogeshnarayan.gaur@nxp.com>
> >
> > Add clockgen support for lx2160a.
^ permalink raw reply
* Re: [PATCH RFC] mm/memory_hotplug: Introduce memory block types
From: Vitaly Kuznetsov @ 2018-10-03 14:34 UTC (permalink / raw)
To: Dave Hansen, Michal Hocko
Cc: Kate Stewart, Rich Felker, linux-ia64, linux-sh, Peter Zijlstra,
Heiko Carstens, linux-mm, Paul Mackerras, H. Peter Anvin,
Stephen Rothwell, Rashmica Gupta, Dan Williams, linux-s390,
Michael Neuling, Stephen Hemminger, Yoshinori Sato,
David Hildenbrand, linux-acpi, Ingo Molnar, xen-devel, Len Brown,
Pavel Tatashin, Rob Herring, mike.travis@hpe.com, Haiyang Zhang,
Jonathan Neuschäfer, Nicholas Piggin, Martin Schwidefsky,
Jérôme Glisse, Mike Rapoport, Borislav Petkov,
Andy Lutomirski, Boris Ostrovsky, Andrew Morton, Oscar Salvador,
Juergen Gross, Tony Luck, Mathieu Malaterre, Greg Kroah-Hartman,
Rafael J. Wysocki, linux-kernel, Fenghua Yu,
Mauricio Faria de Oliveira, Thomas Gleixner, Philippe Ombredanne,
Joe Perches, devel, Joonsoo Kim, linuxppc-dev, Kirill A. Shutemov
In-Reply-To: <49456818-238e-2d95-9df6-d1934e9c8b53@linux.intel.com>
Dave Hansen <dave.hansen@linux.intel.com> writes:
> On 10/03/2018 06:52 AM, Vitaly Kuznetsov wrote:
>> It is more than just memmaps (e.g. forking udev process doing memory
>> onlining also needs memory) but yes, the main idea is to make the
>> onlining synchronous with hotplug.
>
> That's a good theoretical concern.
>
> But, is it a problem we need to solve in practice?
Yes, unfortunately. It was previously discovered that when we try to
hotplug tons of memory to a low memory system (this is a common scenario
with VMs) we end up with OOM because for all new memory blocks we need
to allocate page tables, struct pages, ... and we need memory to do
that. The userspace program doing memory onlining also needs memory to
run and in case it prefers to fork to handle hundreds of notfifications
... well, it may get OOMkilled before it manages to online anything.
Allocating all kernel objects from the newly hotplugged blocks would
definitely help to manage the situation but as I said this won't solve
the 'forking udev' problem completely (it will likely remain in
'extreme' cases only. We can probably work around it by onlining with a
dedicated process which doesn't do memory allocation).
--
Vitaly
^ permalink raw reply
* Re: [PATCH RFC] mm/memory_hotplug: Introduce memory block types
From: David Hildenbrand @ 2018-10-03 17:00 UTC (permalink / raw)
To: Michal Hocko
Cc: Kate Stewart, Rich Felker, linux-ia64, linux-sh, Peter Zijlstra,
Dave Hansen, Heiko Carstens, linux-mm, Pavel Tatashin,
Paul Mackerras, H. Peter Anvin, Rashmica Gupta, K. Y. Srinivasan,
Boris Ostrovsky, linux-s390, Michael Neuling, Stephen Hemminger,
Yoshinori Sato, linux-acpi, Ingo Molnar, xen-devel, Rob Herring,
Len Brown, Fenghua Yu, Stephen Rothwell, mike.travis@hpe.com,
Haiyang Zhang, Dan Williams, Jonathan Neuschäfer,
Nicholas Piggin, Joe Perches, Jérôme Glisse,
Mike Rapoport, Borislav Petkov, Andy Lutomirski, Thomas Gleixner,
Joonsoo Kim, Oscar Salvador, Juergen Gross, Tony Luck,
Mathieu Malaterre, Greg Kroah-Hartman, Rafael J. Wysocki,
linux-kernel, Mauricio Faria de Oliveira, Philippe Ombredanne,
Martin Schwidefsky, devel, Andrew Morton, linuxppc-dev,
Kirill A. Shutemov
In-Reply-To: <20181003135407.GI4714@dhcp22.suse.cz>
On 03/10/2018 15:54, Michal Hocko wrote:
> On Tue 02-10-18 17:25:19, David Hildenbrand wrote:
>> On 02/10/2018 15:47, Michal Hocko wrote:
> [...]
>>> Zone imbalance is an inherent problem of the highmem zone. It is
>>> essentially the highmem zone we all loved so much back in 32b days.
>>> Yes the movable zone doesn't have any addressing limitations so it is a
>>> bit more relaxed but considering the hotplug scenarios I have seen so
>>> far people just want to have full NUMA nodes movable to allow replacing
>>> DIMMs. And then we are back to square one and the zone imbalance issue.
>>> You have those regardless where memmaps are allocated from.
>>
>> Unfortunately yes. And things get more complicated as you are adding a
>> whole DIMMs and get notifications in the granularity of memory blocks.
>> Usually you are not interested in onlining any memory block of that DIMM
>> as MOVABLE as soon as you would have to online one memory block of that
>> DIMM as NORMAL - because that can already block the whole DIMM.
>
> For the purpose of the hotremove, yes. But as Dave has noted people are
> (ab)using zone movable for other purposes - e.g. large pages.
That might be right for some very special use cases. For most of users
this is not the case (meaning it should be the default but if the user
wants to change it, he should be allowed to change it).
>
> [...]
>>> Then the immediate question would be why to use memory hotplug for that
>>> at all? Why don't you simply start with a huge pre-allocated physical
>>> address space and balloon memory in an out per demand. Why do you want
>>> to inject new memory during the runtime?
>>
>> Let's assume you have a guest with 20GB size and eventually want to
>> allow to grow it to 4TB. You would have to allocate metadata for 4TB
>> right from the beginning. That's definitely now what we want. That is
>> why memory hotplug is used by e.g. XEN or Hyper-V. With Hyper-V, the
>> hypervisor even tells you at which places additional memory has been
>> made available.
>
> Then you have to live with the fact that your hot added memory will be
> self hosted and find a way for ballooning to work with that. The price
> would be that some part of the memory is not really balloonable in the
> end.
>
>>>> 1. is a reason why distributions usually don't configure
>>>> "MEMORY_HOTPLUG_DEFAULT_ONLINE", because you really want the option for
>>>> MOVABLE zone. That however implies, that e.g. for x86, you have to
>>>> handle all new memory in user space, especially also HyperV memory.
>>>> There, you then have to check for things like "isHyperV()" to decide
>>>> "oh, yes, this should definitely not go to the MOVABLE zone".
>>>
>>> Why do you need a generic hotplug rule in the first place? Why don't you
>>> simply provide different set of rules for different usecases? Let users
>>> decide which usecase they prefer rather than try to be clever which
>>> almost always hits weird corner cases.
>>>
>>
>> Memory hotplug has to work as reliable as we can out of the box. Letting
>> the user make simple decisions like "oh, I am on hyper-V, I want to
>> online memory to the normal zone" does not feel right.
>
> Users usually know what is their usecase and then it is just a matter of
> plumbing (e.g. distribution can provide proper tools to deploy those
> usecases) to chose the right and for user obscure way to make it work.
I disagree. If we can ship sane defaults, we should do that and allow to
make changes later on. This is how distributions have been working for
ever. But yes, allowing to make modifications is always a good idea to
tailor it to some special case user scenarios. (tuned or whatever we
have in place).
>
>> But yes, we
>> should definitely allow to make modifications. So some sane default rule
>> + possible modification is usually a good idea.
>>
>> I think Dave has a point with using MOVABLE for huge page use cases. And
>> there might be other corner cases as you correctly state.
>>
>> I wonder if this patch itself minus modifying online/offline might make
>> sense. We can then implement simple rules in user space
>>
>> if (normal) {
>> /* customers expect hotplugged DIMMs to be unpluggable */
>> online_movable();
>> } else if (paravirt) {
>> /* paravirt memory should as default always go to the NORMAL */
>> online();
>> } else {
>> /* standby memory will never get onlined automatically */
>> }
>>
>> Compared to having to guess what is to be done (isKVM(), isHyperV,
>> isS390 ...) and failing once this is no longer unique (e.g. virtio-mem
>> and ACPI support for x86 KVM).
>
> I am worried that exporing a type will just push us even further to the
> corner. The current design is really simple and 2 stage and that is good
> because it allows for very different usecases. The more specific the API
> be the more likely we are going to hit "I haven't even dreamed somebody
> would be using hotplug for this thing". And I would bet this will happen
> sooner or later.
Exposing the type of memory is in my point of view just forwarding facts
to user space. We should not export arbitrary information, that is true.
>
> Just look at how the whole auto onlining screwed the API to workaround
> an implementation detail. It has created a one purpose behavior that
> doesn't suite many usecases. Yet we have to live with that because
> somebody really relies on it. Let's not repeat same errors.
>
Let me rephrase: You state that user space has to make the decision and
that user should be able to set/reconfigure rules. That is perfectly fine.
But then we should give user space access to sufficient information to
make a decision. This might be the type of memory as we learned (what
some part of this patch proposes), but maybe later more, e.g. to which
physical device memory belongs (e.g. to hotplug it all movable or all
normal) ...
--
Thanks,
David / dhildenb
^ permalink raw reply
* Re: [PATCH RFC] mm/memory_hotplug: Introduce memory block types
From: David Hildenbrand @ 2018-10-03 17:06 UTC (permalink / raw)
To: Michal Hocko, Vitaly Kuznetsov
Cc: Kate Stewart, Rich Felker, linux-ia64, linux-sh, Peter Zijlstra,
Dave Hansen, Heiko Carstens, linux-mm, Paul Mackerras,
H. Peter Anvin, Stephen Rothwell, Rashmica Gupta, Dan Williams,
linux-s390, Michael Neuling, Stephen Hemminger, Yoshinori Sato,
linux-acpi, Ingo Molnar, xen-devel, Len Brown, Pavel Tatashin,
Rob Herring, mike.travis@hpe.com, Haiyang Zhang,
Jonathan Neuschäfer, Nicholas Piggin, Martin Schwidefsky,
Jérôme Glisse, Mike Rapoport, Borislav Petkov,
Andy Lutomirski, Boris Ostrovsky, Andrew Morton, Oscar Salvador,
Juergen Gross, Tony Luck, Mathieu Malaterre, Greg Kroah-Hartman,
Rafael J. Wysocki, linux-kernel, Fenghua Yu,
Mauricio Faria de Oliveira, Thomas Gleixner, Philippe Ombredanne,
Joe Perches, devel, Joonsoo Kim, linuxppc-dev, Kirill A. Shutemov
In-Reply-To: <20181003142444.GJ4714@dhcp22.suse.cz>
On 03/10/2018 16:24, Michal Hocko wrote:
> On Wed 03-10-18 15:52:24, Vitaly Kuznetsov wrote:
> [...]
>>> As David said some of the memory cannot be onlined without further steps
>>> (e.g. when it is standby as David called it) and then I fail to see how
>>> eBPF help in any way.
>>
>> and also, we can fight till the end of days here trying to come up with
>> an onlining solution which would work for everyone and eBPF would move
>> this decision to distro level.
>
> The point is that there is _no_ general onlining solution. This is
> basically policy which belongs to the userspace.
>
As already stated, I guess we should then provide user space with
sufficient information to make a good decision (to implement rules).
The eBPF is basically the same idea, only the rules are formulated
differently and directly handle in the kernel. Still it might be e.e.
relevant if memory is standby memory (that's what I remember the
official s390x name), or something else.
Right now, the (udev) rules we have make assumptions based on general
system properties (s390x, HyperV ...).
--
Thanks,
David / dhildenb
^ permalink raw reply
* Re: [PATCH RFC] mm/memory_hotplug: Introduce memory block types
From: David Hildenbrand @ 2018-10-03 17:14 UTC (permalink / raw)
To: Vitaly Kuznetsov, Dave Hansen, Michal Hocko
Cc: Kate Stewart, Rich Felker, linux-ia64, linux-sh, Peter Zijlstra,
Heiko Carstens, linux-mm, Paul Mackerras, H. Peter Anvin,
Stephen Rothwell, Rashmica Gupta, Dan Williams, linux-s390,
Michael Neuling, Stephen Hemminger, Yoshinori Sato, linux-acpi,
Ingo Molnar, xen-devel, Len Brown, Pavel Tatashin, Rob Herring,
mike.travis@hpe.com, Haiyang Zhang, Jonathan Neuschäfer,
Nicholas Piggin, Martin Schwidefsky, Jérôme Glisse,
Mike Rapoport, Borislav Petkov, Andy Lutomirski, Boris Ostrovsky,
Andrew Morton, Oscar Salvador, Juergen Gross, Tony Luck,
Mathieu Malaterre, Greg Kroah-Hartman, Rafael J. Wysocki,
linux-kernel, Fenghua Yu, Mauricio Faria de Oliveira,
Thomas Gleixner, Philippe Ombredanne, Joe Perches, devel,
Joonsoo Kim, linuxppc-dev, Kirill A. Shutemov
In-Reply-To: <87tvm3cd5w.fsf@vitty.brq.redhat.com>
On 03/10/2018 16:34, Vitaly Kuznetsov wrote:
> Dave Hansen <dave.hansen@linux.intel.com> writes:
>
>> On 10/03/2018 06:52 AM, Vitaly Kuznetsov wrote:
>>> It is more than just memmaps (e.g. forking udev process doing memory
>>> onlining also needs memory) but yes, the main idea is to make the
>>> onlining synchronous with hotplug.
>>
>> That's a good theoretical concern.
>>
>> But, is it a problem we need to solve in practice?
>
> Yes, unfortunately. It was previously discovered that when we try to
> hotplug tons of memory to a low memory system (this is a common scenario
> with VMs) we end up with OOM because for all new memory blocks we need
> to allocate page tables, struct pages, ... and we need memory to do
> that. The userspace program doing memory onlining also needs memory to
> run and in case it prefers to fork to handle hundreds of notfifications
> ... well, it may get OOMkilled before it manages to online anything.
>
> Allocating all kernel objects from the newly hotplugged blocks would
> definitely help to manage the situation but as I said this won't solve
> the 'forking udev' problem completely (it will likely remain in
> 'extreme' cases only. We can probably work around it by onlining with a
> dedicated process which doesn't do memory allocation).
>
I guess the problem is even worse. We always have two phases
1. add memory - requires memory allocation
2. online memory - might require memory allocations e.g. for slab/slub
So if we just added memory but don't have sufficient memory to start a
user space process to trigger onlining, then we most likely also don't
have sufficient memory to online the memory right away (in some scenarios).
We would have to allocate all new memory for 1 and 2 from the memory to
be onlined. I guess the latter part is less trivial.
So while onlining the memory from the kernel might make things a little
more robust, we would still have the chance for OOM / onlining failing.
--
Thanks,
David / dhildenb
^ permalink raw reply
* Re: [RFC PATCH kernel] vfio/spapr_tce: Get rid of possible infinite loop
From: Alex Williamson @ 2018-10-03 18:17 UTC (permalink / raw)
To: Alexey Kardashevskiy; +Cc: linuxppc-dev, kvm-ppc, David Gibson
In-Reply-To: <20181002032231.7494-1-aik@ozlabs.ru>
On Tue, 2 Oct 2018 13:22:31 +1000
Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
> As a part of cleanup, the SPAPR TCE IOMMU subdriver releases preregistered
> memory. If there is a bug in memory release, the loop in
> tce_iommu_release() becomes infinite; this actually happened to me.
>
> This makes the loop finite and prints a warning on every failure to make
> the code more bug prone.
>
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
> drivers/vfio/vfio_iommu_spapr_tce.c | 10 +++-------
> 1 file changed, 3 insertions(+), 7 deletions(-)
Should this have a stable/fixes tag? Looks like it's relevant to:
4b6fad7097f8 powerpc/mm/iommu, vfio/spapr: Put pages on VFIO container shutdown
Also, not sure who you're wanting to take this since it was sent to ppc
lists. If it's me, let me know, otherwise
Acked-by: Alex Williamson <alex.williamson@redhat.com>
Thanks,
Alex
> diff --git a/drivers/vfio/vfio_iommu_spapr_tce.c b/drivers/vfio/vfio_iommu_spapr_tce.c
> index b1a8ab3..ece0651 100644
> --- a/drivers/vfio/vfio_iommu_spapr_tce.c
> +++ b/drivers/vfio/vfio_iommu_spapr_tce.c
> @@ -371,6 +371,7 @@ static void tce_iommu_release(void *iommu_data)
> {
> struct tce_container *container = iommu_data;
> struct tce_iommu_group *tcegrp;
> + struct tce_iommu_prereg *tcemem, *tmtmp;
> long i;
>
> while (tce_groups_attached(container)) {
> @@ -393,13 +394,8 @@ static void tce_iommu_release(void *iommu_data)
> tce_iommu_free_table(container, tbl);
> }
>
> - while (!list_empty(&container->prereg_list)) {
> - struct tce_iommu_prereg *tcemem;
> -
> - tcemem = list_first_entry(&container->prereg_list,
> - struct tce_iommu_prereg, next);
> - WARN_ON_ONCE(tce_iommu_prereg_free(container, tcemem));
> - }
> + list_for_each_entry_safe(tcemem, tmtmp, &container->prereg_list, next)
> + WARN_ON(tce_iommu_prereg_free(container, tcemem));
>
> tce_iommu_disable(container);
> if (container->mm)
^ permalink raw reply
* Re: [PATCH 2/3] powerpc/powernv/npu: Use size-based ATSD invalidates
From: Mark Hairgrove @ 2018-10-03 18:33 UTC (permalink / raw)
To: Alistair Popple; +Cc: linuxppc-dev, Reza Arbab
In-Reply-To: <3711069.TlgilHPQgk@new-mexico>
On Wed, 3 Oct 2018, Alistair Popple wrote:
> > >
> > > We also support 4K page sizes on PPC. If I am not mistaken this means every ATSD
> > > would invalidate the entire GPU TLB for a the given PID on those systems. Could
> > > we change the above check to `if (size <= PAGE_64K)` to avoid this?
> >
> > PPC supports 4K pages but the GPU ATS implementation does not. For that
> > reason I didn't bother handling invalidates smaller than 64K. I'll add a
> > comment on that.
>
> Interesting, I was not aware of that limitation. Do you know if it is a
> SW/driver limitation or a HW limitation?
HW.
>
> > I don't know that this requirement is enforced anywhere though. I could
> > add a PAGE_SIZE == 64K check to pnv_npu2_init_context if you think it
> > would be useful.
>
> Given it's a static kernel build parameter perhaps it makes more sense to do the
> check as part of the driver build in a conftest rather than a runtime failure?
Sure, we can do it in the driver. I'll just stick with a comment for the
kernel.
^ permalink raw reply
* [PATCH v2 0/3] powerpc/powernv/npu: Improve ATSD invalidation overhead
From: Mark Hairgrove @ 2018-10-03 18:51 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Alistair Popple, Mark Hairgrove, Reza Arbab
When ATS is used in a process, all CPU TLB invalidates in that process
also trigger ATSD invalidates via mmu_notifiers. This additional overhead
is noticeable in applications which do heavy memory allocation or page
migration among nodes, particularly to and from GPUs.
This patch set reduces that overhead by rearranging how the ATSDs are
issued and by using size-based ATSD invalidates.
Mark Hairgrove (3):
powerpc/powernv/npu: Reduce eieio usage when issuing ATSD invalidates
powerpc/powernv/npu: Use size-based ATSD invalidates
powerpc/powernv/npu: Remove atsd_threshold debugfs setting
arch/powerpc/platforms/powernv/npu-dma.c | 198 ++++++++++++++----------------
1 files changed, 94 insertions(+), 104 deletions(-)
--
1.7.2.5
^ permalink raw reply
* [PATCH v2 2/3] powerpc/powernv/npu: Use size-based ATSD invalidates
From: Mark Hairgrove @ 2018-10-03 18:51 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Alistair Popple, Mark Hairgrove, Reza Arbab
In-Reply-To: <1538592694-18739-1-git-send-email-mhairgrove@nvidia.com>
Prior to this change only two types of ATSDs were issued to the NPU:
invalidates targeting a single page and invalidates targeting the whole
address space. The crossover point happened at the configurable
atsd_threshold which defaulted to 2M. Invalidates that size or smaller
would issue per-page invalidates for the whole range.
The NPU supports more invalidation sizes however: 64K, 2M, 1G, and all.
These invalidates target addresses aligned to their size. 2M is a common
invalidation size for GPU-enabled applications because that is a GPU
page size, so reducing the number of invalidates by 32x in that case is a
clear improvement.
ATSD latency is high in general so now we always issue a single invalidate
rather than multiple. This will over-invalidate in some cases, but for any
invalidation size over 2M it matches or improves the prior behavior.
There's also an improvement for single-page invalidates since the prior
version issued two invalidates for that case instead of one.
With this change all issued ATSDs now perform a flush, so the flush
parameter has been removed from all the helpers.
To show the benefit here are some performance numbers from a
microbenchmark which creates a 1G allocation then uses mprotect with
PROT_NONE to trigger invalidates in strides across the allocation.
One NPU (1 GPU):
mprotect rate (GB/s)
Stride Before After Speedup
64K 5.3 5.6 5%
1M 39.3 57.4 46%
2M 49.7 82.6 66%
4M 286.6 285.7 0%
Two NPUs (6 GPUs):
mprotect rate (GB/s)
Stride Before After Speedup
64K 6.5 7.4 13%
1M 33.4 67.9 103%
2M 38.7 93.1 141%
4M 356.7 354.6 -1%
Anything over 2M is roughly the same as before since both cases issue a
single ATSD.
Signed-off-by: Mark Hairgrove <mhairgrove@nvidia.com>
---
arch/powerpc/platforms/powernv/npu-dma.c | 103 ++++++++++++++++--------------
1 files changed, 55 insertions(+), 48 deletions(-)
diff --git a/arch/powerpc/platforms/powernv/npu-dma.c b/arch/powerpc/platforms/powernv/npu-dma.c
index c8f438a..e4c0fab 100644
--- a/arch/powerpc/platforms/powernv/npu-dma.c
+++ b/arch/powerpc/platforms/powernv/npu-dma.c
@@ -18,6 +18,7 @@
#include <linux/memblock.h>
#include <linux/iommu.h>
#include <linux/debugfs.h>
+#include <linux/sizes.h>
#include <asm/debugfs.h>
#include <asm/tlb.h>
@@ -458,8 +459,7 @@ static void put_mmio_atsd_reg(struct npu *npu, int reg)
#define XTS_ATSD_AVA 1
#define XTS_ATSD_STAT 2
-static unsigned long get_atsd_launch_val(unsigned long pid, unsigned long psize,
- bool flush)
+static unsigned long get_atsd_launch_val(unsigned long pid, unsigned long psize)
{
unsigned long launch = 0;
@@ -477,8 +477,7 @@ static unsigned long get_atsd_launch_val(unsigned long pid, unsigned long psize,
/* PID */
launch |= pid << PPC_BITLSHIFT(38);
- /* No flush */
- launch |= !flush << PPC_BITLSHIFT(39);
+ /* Leave "No flush" (bit 39) 0 so every ATSD performs a flush */
return launch;
}
@@ -501,23 +500,22 @@ static void mmio_atsd_regs_write(struct mmio_atsd_reg
}
static void mmio_invalidate_pid(struct mmio_atsd_reg mmio_atsd_reg[NV_MAX_NPUS],
- unsigned long pid, bool flush)
+ unsigned long pid)
{
- unsigned long launch = get_atsd_launch_val(pid, MMU_PAGE_COUNT, flush);
+ unsigned long launch = get_atsd_launch_val(pid, MMU_PAGE_COUNT);
/* Invalidating the entire process doesn't use a va */
mmio_atsd_regs_write(mmio_atsd_reg, XTS_ATSD_LAUNCH, launch);
}
-static void mmio_invalidate_va(struct mmio_atsd_reg mmio_atsd_reg[NV_MAX_NPUS],
- unsigned long va, unsigned long pid, bool flush)
+static void mmio_invalidate_range(struct mmio_atsd_reg
+ mmio_atsd_reg[NV_MAX_NPUS], unsigned long pid,
+ unsigned long start, unsigned long psize)
{
- unsigned long launch;
-
- launch = get_atsd_launch_val(pid, mmu_virtual_psize, flush);
+ unsigned long launch = get_atsd_launch_val(pid, psize);
/* Write all VAs first */
- mmio_atsd_regs_write(mmio_atsd_reg, XTS_ATSD_AVA, va);
+ mmio_atsd_regs_write(mmio_atsd_reg, XTS_ATSD_AVA, start);
/* Issue one barrier for all address writes */
eieio();
@@ -609,14 +607,36 @@ static void release_atsd_reg(struct mmio_atsd_reg mmio_atsd_reg[NV_MAX_NPUS])
}
/*
- * Invalidate either a single address or an entire PID depending on
- * the value of va.
+ * Invalidate a virtual address range
*/
-static void mmio_invalidate(struct npu_context *npu_context, int va,
- unsigned long address, bool flush)
+static void mmio_invalidate(struct npu_context *npu_context,
+ unsigned long start, unsigned long size)
{
struct mmio_atsd_reg mmio_atsd_reg[NV_MAX_NPUS];
unsigned long pid = npu_context->mm->context.id;
+ unsigned long atsd_start = 0;
+ unsigned long end = start + size - 1;
+ int atsd_psize = MMU_PAGE_COUNT;
+
+ /*
+ * Convert the input range into one of the supported sizes. If the range
+ * doesn't fit, use the next larger supported size. Invalidation latency
+ * is high, so over-invalidation is preferred to issuing multiple
+ * invalidates.
+ *
+ * A 4K page size isn't supported by NPU/GPU ATS, so that case is
+ * ignored.
+ */
+ if (size == SZ_64K) {
+ atsd_start = start;
+ atsd_psize = MMU_PAGE_64K;
+ } else if (ALIGN_DOWN(start, SZ_2M) == ALIGN_DOWN(end, SZ_2M)) {
+ atsd_start = ALIGN_DOWN(start, SZ_2M);
+ atsd_psize = MMU_PAGE_2M;
+ } else if (ALIGN_DOWN(start, SZ_1G) == ALIGN_DOWN(end, SZ_1G)) {
+ atsd_start = ALIGN_DOWN(start, SZ_1G);
+ atsd_psize = MMU_PAGE_1G;
+ }
if (npu_context->nmmu_flush)
/*
@@ -631,23 +651,25 @@ static void mmio_invalidate(struct npu_context *npu_context, int va,
* an invalidate.
*/
acquire_atsd_reg(npu_context, mmio_atsd_reg);
- if (va)
- mmio_invalidate_va(mmio_atsd_reg, address, pid, flush);
+
+ if (atsd_psize == MMU_PAGE_COUNT)
+ mmio_invalidate_pid(mmio_atsd_reg, pid);
else
- mmio_invalidate_pid(mmio_atsd_reg, pid, flush);
+ mmio_invalidate_range(mmio_atsd_reg, pid, atsd_start,
+ atsd_psize);
mmio_invalidate_wait(mmio_atsd_reg);
- if (flush) {
- /*
- * The GPU requires two flush ATSDs to ensure all entries have
- * been flushed. We use PID 0 as it will never be used for a
- * process on the GPU.
- */
- mmio_invalidate_pid(mmio_atsd_reg, 0, true);
- mmio_invalidate_wait(mmio_atsd_reg);
- mmio_invalidate_pid(mmio_atsd_reg, 0, true);
- mmio_invalidate_wait(mmio_atsd_reg);
- }
+
+ /*
+ * The GPU requires two flush ATSDs to ensure all entries have been
+ * flushed. We use PID 0 as it will never be used for a process on the
+ * GPU.
+ */
+ mmio_invalidate_pid(mmio_atsd_reg, 0);
+ mmio_invalidate_wait(mmio_atsd_reg);
+ mmio_invalidate_pid(mmio_atsd_reg, 0);
+ mmio_invalidate_wait(mmio_atsd_reg);
+
release_atsd_reg(mmio_atsd_reg);
}
@@ -664,7 +686,7 @@ static void pnv_npu2_mn_release(struct mmu_notifier *mn,
* There should be no more translation requests for this PID, but we
* need to ensure any entries for it are removed from the TLB.
*/
- mmio_invalidate(npu_context, 0, 0, true);
+ mmio_invalidate(npu_context, 0, ~0UL);
}
static void pnv_npu2_mn_change_pte(struct mmu_notifier *mn,
@@ -673,8 +695,7 @@ static void pnv_npu2_mn_change_pte(struct mmu_notifier *mn,
pte_t pte)
{
struct npu_context *npu_context = mn_to_npu_context(mn);
-
- mmio_invalidate(npu_context, 1, address, true);
+ mmio_invalidate(npu_context, address, PAGE_SIZE);
}
static void pnv_npu2_mn_invalidate_range(struct mmu_notifier *mn,
@@ -682,21 +703,7 @@ static void pnv_npu2_mn_invalidate_range(struct mmu_notifier *mn,
unsigned long start, unsigned long end)
{
struct npu_context *npu_context = mn_to_npu_context(mn);
- unsigned long address;
-
- if (end - start > atsd_threshold) {
- /*
- * Just invalidate the entire PID if the address range is too
- * large.
- */
- mmio_invalidate(npu_context, 0, 0, true);
- } else {
- for (address = start; address < end; address += PAGE_SIZE)
- mmio_invalidate(npu_context, 1, address, false);
-
- /* Do the flush only on the final addess == end */
- mmio_invalidate(npu_context, 1, address, true);
- }
+ mmio_invalidate(npu_context, start, end - start);
}
static const struct mmu_notifier_ops nv_nmmu_notifier_ops = {
--
1.7.2.5
^ permalink raw reply related
* [PATCH v2 1/3] powerpc/powernv/npu: Reduce eieio usage when issuing ATSD invalidates
From: Mark Hairgrove @ 2018-10-03 18:51 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Alistair Popple, Mark Hairgrove, Reza Arbab
In-Reply-To: <1538592694-18739-1-git-send-email-mhairgrove@nvidia.com>
There are two types of ATSDs issued to the NPU: invalidates targeting a
specific virtual address and invalidates targeting the whole address
space. In both cases prior to this change, the sequence was:
for each NPU
- Write the target address to the XTS_ATSD_AVA register
- EIEIO
- Write the launch value to issue the ATSD
First, a target address is not required when invalidating the whole
address space, so that write and the EIEIO have been removed. The AP
(size) field in the launch is not needed either.
Second, for per-address invalidates the above sequence is inefficient in
the common case of multiple NPUs because an EIEIO is issued per NPU. This
unnecessarily forces the launches of later ATSDs to be ordered with the
launches of earlier ones. The new sequence only issues a single EIEIO:
for each NPU
- Write the target address to the XTS_ATSD_AVA register
EIEIO
for each NPU
- Write the launch value to issue the ATSD
Performance results were gathered using a microbenchmark which creates a
1G allocation then uses mprotect with PROT_NONE to trigger invalidates in
strides across the allocation.
With only a single NPU active (one GPU) the difference is in the noise for
both types of invalidates (+/-1%).
With two NPUs active (on a 6-GPU system) the effect is more noticeable:
mprotect rate (GB/s)
Stride Before After Speedup
64K 5.9 6.5 10%
1M 31.2 33.4 7%
2M 36.3 38.7 7%
4M 322.6 356.7 11%
Signed-off-by: Mark Hairgrove <mhairgrove@nvidia.com>
---
arch/powerpc/platforms/powernv/npu-dma.c | 99 ++++++++++++++---------------
1 files changed, 48 insertions(+), 51 deletions(-)
diff --git a/arch/powerpc/platforms/powernv/npu-dma.c b/arch/powerpc/platforms/powernv/npu-dma.c
index 8006c54..c8f438a 100644
--- a/arch/powerpc/platforms/powernv/npu-dma.c
+++ b/arch/powerpc/platforms/powernv/npu-dma.c
@@ -454,79 +454,76 @@ static void put_mmio_atsd_reg(struct npu *npu, int reg)
}
/* MMIO ATSD register offsets */
-#define XTS_ATSD_AVA 1
-#define XTS_ATSD_STAT 2
+#define XTS_ATSD_LAUNCH 0
+#define XTS_ATSD_AVA 1
+#define XTS_ATSD_STAT 2
-static void mmio_launch_invalidate(struct mmio_atsd_reg *mmio_atsd_reg,
- unsigned long launch, unsigned long va)
+static unsigned long get_atsd_launch_val(unsigned long pid, unsigned long psize,
+ bool flush)
{
- struct npu *npu = mmio_atsd_reg->npu;
- int reg = mmio_atsd_reg->reg;
+ unsigned long launch = 0;
- __raw_writeq_be(va, npu->mmio_atsd_regs[reg] + XTS_ATSD_AVA);
- eieio();
- __raw_writeq_be(launch, npu->mmio_atsd_regs[reg]);
+ if (psize == MMU_PAGE_COUNT) {
+ /* IS set to invalidate entire matching PID */
+ launch |= PPC_BIT(12);
+ } else {
+ /* AP set to invalidate region of psize */
+ launch |= (u64)mmu_get_ap(psize) << PPC_BITLSHIFT(17);
+ }
+
+ /* PRS set to process-scoped */
+ launch |= PPC_BIT(13);
+
+ /* PID */
+ launch |= pid << PPC_BITLSHIFT(38);
+
+ /* No flush */
+ launch |= !flush << PPC_BITLSHIFT(39);
+
+ return launch;
}
-static void mmio_invalidate_pid(struct mmio_atsd_reg mmio_atsd_reg[NV_MAX_NPUS],
- unsigned long pid, bool flush)
+static void mmio_atsd_regs_write(struct mmio_atsd_reg
+ mmio_atsd_reg[NV_MAX_NPUS], unsigned long offset,
+ unsigned long val)
{
- int i;
- unsigned long launch;
+ struct npu *npu;
+ int i, reg;
for (i = 0; i <= max_npu2_index; i++) {
- if (mmio_atsd_reg[i].reg < 0)
+ reg = mmio_atsd_reg[i].reg;
+ if (reg < 0)
continue;
- /* IS set to invalidate matching PID */
- launch = PPC_BIT(12);
-
- /* PRS set to process-scoped */
- launch |= PPC_BIT(13);
-
- /* AP */
- launch |= (u64)
- mmu_get_ap(mmu_virtual_psize) << PPC_BITLSHIFT(17);
-
- /* PID */
- launch |= pid << PPC_BITLSHIFT(38);
+ npu = mmio_atsd_reg[i].npu;
+ __raw_writeq_be(val, npu->mmio_atsd_regs[reg] + offset);
+ }
+}
- /* No flush */
- launch |= !flush << PPC_BITLSHIFT(39);
+static void mmio_invalidate_pid(struct mmio_atsd_reg mmio_atsd_reg[NV_MAX_NPUS],
+ unsigned long pid, bool flush)
+{
+ unsigned long launch = get_atsd_launch_val(pid, MMU_PAGE_COUNT, flush);
- /* Invalidating the entire process doesn't use a va */
- mmio_launch_invalidate(&mmio_atsd_reg[i], launch, 0);
- }
+ /* Invalidating the entire process doesn't use a va */
+ mmio_atsd_regs_write(mmio_atsd_reg, XTS_ATSD_LAUNCH, launch);
}
static void mmio_invalidate_va(struct mmio_atsd_reg mmio_atsd_reg[NV_MAX_NPUS],
unsigned long va, unsigned long pid, bool flush)
{
- int i;
unsigned long launch;
- for (i = 0; i <= max_npu2_index; i++) {
- if (mmio_atsd_reg[i].reg < 0)
- continue;
-
- /* IS set to invalidate target VA */
- launch = 0;
+ launch = get_atsd_launch_val(pid, mmu_virtual_psize, flush);
- /* PRS set to process scoped */
- launch |= PPC_BIT(13);
+ /* Write all VAs first */
+ mmio_atsd_regs_write(mmio_atsd_reg, XTS_ATSD_AVA, va);
- /* AP */
- launch |= (u64)
- mmu_get_ap(mmu_virtual_psize) << PPC_BITLSHIFT(17);
-
- /* PID */
- launch |= pid << PPC_BITLSHIFT(38);
-
- /* No flush */
- launch |= !flush << PPC_BITLSHIFT(39);
+ /* Issue one barrier for all address writes */
+ eieio();
- mmio_launch_invalidate(&mmio_atsd_reg[i], launch, va);
- }
+ /* Launch */
+ mmio_atsd_regs_write(mmio_atsd_reg, XTS_ATSD_LAUNCH, launch);
}
#define mn_to_npu_context(x) container_of(x, struct npu_context, mn)
--
1.7.2.5
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox