All of lore.kernel.org
 help / color / mirror / Atom feed
From: Nathan Lynch <nathanl@linux.ibm.com>
To: Pingfan Liu <kernelfans@gmail.com>
Cc: Michael Ellerman <mpe@ellerman.id.au>,
	cheloha@linux.ibm.com,
	Kexec Mailing List <kexec@lists.infradead.org>,
	ldufour@linux.ibm.com,
	linuxppc-dev <linuxppc-dev@lists.ozlabs.org>,
	Hari Bathini <hbathini@linux.ibm.com>
Subject: Re: [PATCHv3 2/2] powerpc/pseries: update device tree before ejecting hotplug uevents
Date: Fri, 24 Jul 2020 11:50:12 -0500	[thread overview]
Message-ID: <87d04k26yj.fsf@linux.ibm.com> (raw)
In-Reply-To: <CAFgQCTu_QO=50v2J0=aY2iV8P-oM82_Kfw9My600ZARUt01grw@mail.gmail.com>

Pingfan Liu <kernelfans@gmail.com> writes:
> On Thu, Jul 23, 2020 at 9:27 PM Nathan Lynch <nathanl@linux.ibm.com> wrote:
>> Pingfan Liu <kernelfans@gmail.com> writes:
>> > This will introduce extra dt updating payload for each involved lmb when hotplug.
>> > But it should be fine since drmem_update_dt() is memory based operation and
>> > hotplug is not a hot path.
>>
>> This is great analysis but the performance implications of the change
>> are grave. The add/remove paths here are already O(n) where n is the
>> quantity of memory assigned to the LP, this change would make it O(n^2):
>>
>> dlpar_memory_add_by_count
>>   for_each_drmem_lmb             <--
>>     dlpar_add_lmb
>>       drmem_update_dt(_v1|_v2)
>>         for_each_drmem_lmb       <--
>>
>> Memory add/remove isn't a hot path but quadratic runtime complexity
>> isn't acceptable. Its current performance is bad enough that I have
> Yes, the quadratic runtime complexity sounds terrible.
> And I am curious about the bug. Does the system have thousands of lmb?

Yes.

>> Not to mention we leak memory every time drmem_update_dt is called
>> because we can't safely free device tree properties :-(
> Do you know what block us to free it?

It's a longstanding problem. References to device tree properties aren't
counted or tracked so there's no way to safely free them unless the node
itself is released. But the ibm,dynamic-reconfiguration-memory node does
not ever go away and its properties are only subject to updates.

Maybe there's a way to address the specific case of
ibm,dynamic-reconfiguration-memory and the ibm,dynamic-memory(-v2)
properties, instead of tackling the general problem.

Regardless of all that, the drmem code needs better data structures and
lookup functions.

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

WARNING: multiple messages have this Message-ID (diff)
From: Nathan Lynch <nathanl@linux.ibm.com>
To: Pingfan Liu <kernelfans@gmail.com>
Cc: cheloha@linux.ibm.com,
	Kexec Mailing List <kexec@lists.infradead.org>,
	ldufour@linux.ibm.com,
	linuxppc-dev <linuxppc-dev@lists.ozlabs.org>,
	Hari Bathini <hbathini@linux.ibm.com>
Subject: Re: [PATCHv3 2/2] powerpc/pseries: update device tree before ejecting hotplug uevents
Date: Fri, 24 Jul 2020 11:50:12 -0500	[thread overview]
Message-ID: <87d04k26yj.fsf@linux.ibm.com> (raw)
In-Reply-To: <CAFgQCTu_QO=50v2J0=aY2iV8P-oM82_Kfw9My600ZARUt01grw@mail.gmail.com>

Pingfan Liu <kernelfans@gmail.com> writes:
> On Thu, Jul 23, 2020 at 9:27 PM Nathan Lynch <nathanl@linux.ibm.com> wrote:
>> Pingfan Liu <kernelfans@gmail.com> writes:
>> > This will introduce extra dt updating payload for each involved lmb when hotplug.
>> > But it should be fine since drmem_update_dt() is memory based operation and
>> > hotplug is not a hot path.
>>
>> This is great analysis but the performance implications of the change
>> are grave. The add/remove paths here are already O(n) where n is the
>> quantity of memory assigned to the LP, this change would make it O(n^2):
>>
>> dlpar_memory_add_by_count
>>   for_each_drmem_lmb             <--
>>     dlpar_add_lmb
>>       drmem_update_dt(_v1|_v2)
>>         for_each_drmem_lmb       <--
>>
>> Memory add/remove isn't a hot path but quadratic runtime complexity
>> isn't acceptable. Its current performance is bad enough that I have
> Yes, the quadratic runtime complexity sounds terrible.
> And I am curious about the bug. Does the system have thousands of lmb?

Yes.

>> Not to mention we leak memory every time drmem_update_dt is called
>> because we can't safely free device tree properties :-(
> Do you know what block us to free it?

It's a longstanding problem. References to device tree properties aren't
counted or tracked so there's no way to safely free them unless the node
itself is released. But the ibm,dynamic-reconfiguration-memory node does
not ever go away and its properties are only subject to updates.

Maybe there's a way to address the specific case of
ibm,dynamic-reconfiguration-memory and the ibm,dynamic-memory(-v2)
properties, instead of tackling the general problem.

Regardless of all that, the drmem code needs better data structures and
lookup functions.

  reply	other threads:[~2020-07-24 16:50 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-07-22  1:52 [PATCHv3 1/2] powerpc/pseries: group lmb operation and memblock's Pingfan Liu
2020-07-22  1:52 ` Pingfan Liu
2020-07-22  1:52 ` [PATCHv3 2/2] powerpc/pseries: update device tree before ejecting hotplug uevents Pingfan Liu
2020-07-22  1:52   ` Pingfan Liu
2020-07-22  4:57   ` Michael Ellerman
2020-07-22  4:57     ` Michael Ellerman
2020-07-23  2:27     ` Pingfan Liu
2020-07-23  2:27       ` Pingfan Liu
2020-07-23  6:41       ` Michael Ellerman
2020-07-23  6:41         ` Michael Ellerman
2020-07-23 13:27   ` Nathan Lynch
2020-07-23 13:27     ` Nathan Lynch
2020-07-24 12:24     ` Pingfan Liu
2020-07-24 12:24       ` Pingfan Liu
2020-07-24 16:50       ` Nathan Lynch [this message]
2020-07-24 16:50         ` Nathan Lynch
2020-07-23 14:41 ` [PATCHv3 1/2] powerpc/pseries: group lmb operation and memblock's Nathan Lynch
2020-07-23 14:41   ` Nathan Lynch
2020-07-28  6:39   ` Pingfan Liu
2020-07-28  6:39     ` Pingfan Liu

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87d04k26yj.fsf@linux.ibm.com \
    --to=nathanl@linux.ibm.com \
    --cc=cheloha@linux.ibm.com \
    --cc=hbathini@linux.ibm.com \
    --cc=kernelfans@gmail.com \
    --cc=kexec@lists.infradead.org \
    --cc=ldufour@linux.ibm.com \
    --cc=linuxppc-dev@lists.ozlabs.org \
    --cc=mpe@ellerman.id.au \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.