* [PATCH] drivers/base/memory.c: memory subsys init: skip search for missing blocks
@ 2019-11-01 18:10 Scott Cheloha
2019-11-01 19:00 ` David Hildenbrand
0 siblings, 1 reply; 7+ messages in thread
From: Scott Cheloha @ 2019-11-01 18:10 UTC (permalink / raw)
To: linux-kernel, Rafael J. Wysocki, Greg Kroah-Hartman, David Hildenbrand
Cc: nathanl, ricklind, Scott Cheloha
Add a flag to init_memory_block() to enable/disable searching for a
matching block before creating a device for the block and adding it to
the memory subsystem's bus.
When the memory subsystem is being initialized there is no need to check
if a given block has already been added to its bus. The bus is new, so the
block in question cannot yet have a corresponding device.
The search for a missing block is O(n) so this saves substantial time at
boot if there are many such blocks to add.
Signed-off-by: Scott Cheloha <cheloha@linux.vnet.ibm.com>
---
drivers/base/memory.c | 18 +++++++++++-------
1 file changed, 11 insertions(+), 7 deletions(-)
diff --git a/drivers/base/memory.c b/drivers/base/memory.c
index 55907c27075b..1160df4a8feb 100644
--- a/drivers/base/memory.c
+++ b/drivers/base/memory.c
@@ -643,17 +643,21 @@ int register_memory(struct memory_block *memory)
}
static int init_memory_block(struct memory_block **memory,
- unsigned long block_id, unsigned long state)
+ unsigned long block_id, unsigned long state,
+ bool may_exist)
{
struct memory_block *mem;
unsigned long start_pfn;
int ret = 0;
- mem = find_memory_block_by_id(block_id);
- if (mem) {
- put_device(&mem->dev);
- return -EEXIST;
+ if (may_exist) {
+ mem = find_memory_block_by_id(block_id);
+ if (mem) {
+ put_device(&mem->dev);
+ return -EEXIST;
+ }
}
+
mem = kzalloc(sizeof(*mem), GFP_KERNEL);
if (!mem)
return -ENOMEM;
@@ -684,7 +688,7 @@ static int add_memory_block(unsigned long base_section_nr)
if (section_count == 0)
return 0;
ret = init_memory_block(&mem, base_memory_block_id(base_section_nr),
- MEM_ONLINE);
+ MEM_ONLINE, false);
if (ret)
return ret;
mem->section_count = section_count;
@@ -720,7 +724,7 @@ int create_memory_block_devices(unsigned long start, unsigned long size)
mutex_lock(&mem_sysfs_mutex);
for (block_id = start_block_id; block_id != end_block_id; block_id++) {
- ret = init_memory_block(&mem, block_id, MEM_OFFLINE);
+ ret = init_memory_block(&mem, block_id, MEM_OFFLINE, true);
if (ret)
break;
mem->section_count = sections_per_block;
--
2.24.0.rc1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] drivers/base/memory.c: memory subsys init: skip search for missing blocks
2019-11-01 18:10 [PATCH] drivers/base/memory.c: memory subsys init: skip search for missing blocks Scott Cheloha
@ 2019-11-01 19:00 ` David Hildenbrand
2019-11-01 19:20 ` David Hildenbrand
2019-11-01 22:32 ` Rick Lindsley
0 siblings, 2 replies; 7+ messages in thread
From: David Hildenbrand @ 2019-11-01 19:00 UTC (permalink / raw)
To: Scott Cheloha, linux-kernel, Rafael J. Wysocki, Greg Kroah-Hartman
Cc: nathanl, ricklind
On 01.11.19 19:10, Scott Cheloha wrote:
> Add a flag to init_memory_block() to enable/disable searching for a
> matching block before creating a device for the block and adding it to
> the memory subsystem's bus.
>
> When the memory subsystem is being initialized there is no need to check
> if a given block has already been added to its bus. The bus is new, so the
> block in question cannot yet have a corresponding device.
>
> The search for a missing block is O(n) so this saves substantial time at
> boot if there are many such blocks to add.
>
> Signed-off-by: Scott Cheloha <cheloha@linux.vnet.ibm.com>
> ---
> drivers/base/memory.c | 18 +++++++++++-------
> 1 file changed, 11 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/base/memory.c b/drivers/base/memory.c
> index 55907c27075b..1160df4a8feb 100644
> --- a/drivers/base/memory.c
> +++ b/drivers/base/memory.c
> @@ -643,17 +643,21 @@ int register_memory(struct memory_block *memory)
> }
>
> static int init_memory_block(struct memory_block **memory,
> - unsigned long block_id, unsigned long state)
> + unsigned long block_id, unsigned long state,
> + bool may_exist)
> {
> struct memory_block *mem;
> unsigned long start_pfn;
> int ret = 0;
>
> - mem = find_memory_block_by_id(block_id);
> - if (mem) {
> - put_device(&mem->dev);
> - return -EEXIST;
> + if (may_exist) {
> + mem = find_memory_block_by_id(block_id);
> + if (mem) {
> + put_device(&mem->dev);
> + return -EEXIST;
> + }
No, I don't really like that. Can we please speed up the lookup via a
radix tree as noted in the comment of "find_memory_block()".
/*
* For now, we have a linear search to go find the appropriate
* memory_block corresponding to a particular phys_index. If
* this gets to be a real problem, we can always use a radix
* tree or something here.
*
* This could be made generic for all device subsystems.
*/
This will speed up all users of walk_memory_blocks() similarly.
Especially, it will also speed up link_mem_sections() used during boot,
which relies on walk_memory_blocks().
--
Thanks,
David / dhildenb
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] drivers/base/memory.c: memory subsys init: skip search for missing blocks
2019-11-01 19:00 ` David Hildenbrand
@ 2019-11-01 19:20 ` David Hildenbrand
2019-11-01 22:32 ` Rick Lindsley
1 sibling, 0 replies; 7+ messages in thread
From: David Hildenbrand @ 2019-11-01 19:20 UTC (permalink / raw)
To: Scott Cheloha, linux-kernel, Rafael J. Wysocki, Greg Kroah-Hartman
Cc: nathanl, ricklind
On 01.11.19 20:00, David Hildenbrand wrote:
> On 01.11.19 19:10, Scott Cheloha wrote:
>> Add a flag to init_memory_block() to enable/disable searching for a
>> matching block before creating a device for the block and adding it to
>> the memory subsystem's bus.
>>
>> When the memory subsystem is being initialized there is no need to check
>> if a given block has already been added to its bus. The bus is new, so the
>> block in question cannot yet have a corresponding device.
>>
>> The search for a missing block is O(n) so this saves substantial time at
>> boot if there are many such blocks to add.
>>
>> Signed-off-by: Scott Cheloha <cheloha@linux.vnet.ibm.com>
>> ---
>> drivers/base/memory.c | 18 +++++++++++-------
>> 1 file changed, 11 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/base/memory.c b/drivers/base/memory.c
>> index 55907c27075b..1160df4a8feb 100644
>> --- a/drivers/base/memory.c
>> +++ b/drivers/base/memory.c
>> @@ -643,17 +643,21 @@ int register_memory(struct memory_block *memory)
>> }
>>
>> static int init_memory_block(struct memory_block **memory,
>> - unsigned long block_id, unsigned long state)
>> + unsigned long block_id, unsigned long state,
>> + bool may_exist)
>> {
>> struct memory_block *mem;
>> unsigned long start_pfn;
>> int ret = 0;
>>
>> - mem = find_memory_block_by_id(block_id);
>> - if (mem) {
>> - put_device(&mem->dev);
>> - return -EEXIST;
>> + if (may_exist) {
>> + mem = find_memory_block_by_id(block_id);
>> + if (mem) {
>> + put_device(&mem->dev);
>> + return -EEXIST;
>> + }
>
> No, I don't really like that. Can we please speed up the lookup via a
> radix tree as noted in the comment of "find_memory_block()".
>
> /*
> * For now, we have a linear search to go find the appropriate
> * memory_block corresponding to a particular phys_index. If
> * this gets to be a real problem, we can always use a radix
> * tree or something here.
> *
> * This could be made generic for all device subsystems.
> */
>
> This will speed up all users of walk_memory_blocks() similarly.
> Especially, it will also speed up link_mem_sections() used during boot,
> which relies on walk_memory_blocks().
>
Also, I think this check that I added back than should not happen in the
wild. If we care, we could turn that into a
VM_BUG_ON(find_memory_block_by_id(block_id)).
--
Thanks,
David / dhildenb
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] drivers/base/memory.c: memory subsys init: skip search for missing blocks
2019-11-01 19:00 ` David Hildenbrand
2019-11-01 19:20 ` David Hildenbrand
@ 2019-11-01 22:32 ` Rick Lindsley
2019-11-01 22:47 ` David Hildenbrand
1 sibling, 1 reply; 7+ messages in thread
From: Rick Lindsley @ 2019-11-01 22:32 UTC (permalink / raw)
To: David Hildenbrand, Scott Cheloha, linux-kernel,
Rafael J. Wysocki, Greg Kroah-Hartman
Cc: nathanl
On 11/1/19 12:00 PM, David Hildenbrand wrote:
>
> No, I don't really like that. Can we please speed up the lookup via a radix tree as noted in the comment of "find_memory_block()".
I agree with the general sentiment that a redesign is the correct long term action - it has been for some time now - but implementing a new storage and retrieval method and verifying that it introduces no new problems itself is non-trivial. There's a reason it remains a comment.
I don't see any issues with the patch itself. Do we really want to forego the short term, low-hanging, low risk fruit in favor of waiting indefinitely for that well-tested long-term solution?
Rick
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] drivers/base/memory.c: memory subsys init: skip search for missing blocks
2019-11-01 22:32 ` Rick Lindsley
@ 2019-11-01 22:47 ` David Hildenbrand
2019-11-02 19:43 ` Scott Cheloha
0 siblings, 1 reply; 7+ messages in thread
From: David Hildenbrand @ 2019-11-01 22:47 UTC (permalink / raw)
To: Rick Lindsley
Cc: Scott Cheloha, linux-kernel, Rafael J. Wysocki,
Greg Kroah-Hartman, nathanl
> Am 01.11.2019 um 23:32 schrieb Rick Lindsley <ricklind@linux.vnet.ibm.com>:
>
> On 11/1/19 12:00 PM, David Hildenbrand wrote:
>> No, I don't really like that. Can we please speed up the lookup via a radix tree as noted in the comment of "find_memory_block()".
>
> I agree with the general sentiment that a redesign is the correct long term action - it has been for some time now - but implementing a new storage and retrieval method and verifying that it introduces no new problems itself is non-trivial. There's a reason it remains a comment.
>
> I don't see any issues with the patch itself. Do we really want to forego the short term, low-hanging, low risk fruit in favor of waiting indefinitely for that well-tested long-term solution?
The low hanging fruit for me is to convert it to a simple VM_BUG_ON(). As I said, this should never really happen with current code.
Also, I don‘t think adding a radix tree here is rocket science and takes indefinitely ;) feel free to prove me wrong.
>
> Rick
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] drivers/base/memory.c: memory subsys init: skip search for missing blocks
2019-11-01 22:47 ` David Hildenbrand
@ 2019-11-02 19:43 ` Scott Cheloha
2019-11-02 20:18 ` David Hildenbrand
0 siblings, 1 reply; 7+ messages in thread
From: Scott Cheloha @ 2019-11-02 19:43 UTC (permalink / raw)
To: David Hildenbrand
Cc: Rick Lindsley, linux-kernel, Rafael J. Wysocki,
Greg Kroah-Hartman, nathanl
On Fri, Nov 01, 2019 at 11:47:49PM +0100, David Hildenbrand wrote:
>
> > Am 01.11.2019 um 23:32 schrieb Rick Lindsley <ricklind@linux.vnet.ibm.com>:
> >
> > On 11/1/19 12:00 PM, David Hildenbrand wrote:
> >> No, I don't really like that. Can we please speed up the lookup via a radix tree as noted in the comment of "find_memory_block()".
> >
> > I agree with the general sentiment that a redesign is the correct long term action - it has been for some time now - but implementing a new storage and retrieval method and verifying that it introduces no new problems itself is non-trivial. There's a reason it remains a comment.
> >
> > I don't see any issues with the patch itself. Do we really want to forego the short term, low-hanging, low risk fruit in favor of waiting indefinitely for that well-tested long-term solution?
>
> The low hanging fruit for me is to convert it to a simple VM_BUG_ON(). As I said, this should never really happen with current code.
>
> Also, I don‘t think adding a radix tree here is rocket science and takes indefinitely ;) feel free to prove me wrong.
To clarify the goal here, "adding a radix tree" means changing
subsys_private's klist_devices member from a klist to a radix
tree or xarray, right?
-Scott
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] drivers/base/memory.c: memory subsys init: skip search for missing blocks
2019-11-02 19:43 ` Scott Cheloha
@ 2019-11-02 20:18 ` David Hildenbrand
0 siblings, 0 replies; 7+ messages in thread
From: David Hildenbrand @ 2019-11-02 20:18 UTC (permalink / raw)
To: Scott Cheloha
Cc: Rick Lindsley, linux-kernel, Rafael J. Wysocki,
Greg Kroah-Hartman, nathanl
> Am 02.11.2019 um 20:43 schrieb Scott Cheloha <cheloha@linux.vnet.ibm.com>:
>
> On Fri, Nov 01, 2019 at 11:47:49PM +0100, David Hildenbrand wrote:
>>
>>>> Am 01.11.2019 um 23:32 schrieb Rick Lindsley <ricklind@linux.vnet.ibm.com>:
>>>
>>> On 11/1/19 12:00 PM, David Hildenbrand wrote:
>>>> No, I don't really like that. Can we please speed up the lookup via a radix tree as noted in the comment of "find_memory_block()".
>>>
>>> I agree with the general sentiment that a redesign is the correct long term action - it has been for some time now - but implementing a new storage and retrieval method and verifying that it introduces no new problems itself is non-trivial. There's a reason it remains a comment.
>>>
>>> I don't see any issues with the patch itself. Do we really want to forego the short term, low-hanging, low risk fruit in favor of waiting indefinitely for that well-tested long-term solution?
>>
>> The low hanging fruit for me is to convert it to a simple VM_BUG_ON(). As I said, this should never really happen with current code.
>>
>> Also, I don‘t think adding a radix tree here is rocket science and takes indefinitely ;) feel free to prove me wrong.
>
> To clarify the goal here, "adding a radix tree" means changing
> subsys_private's klist_devices member from a klist to a radix
> tree or xarray, right?
I wouldn‘t go that far and only use a subsystem local data structure as a fast lookup cache. The memory subsystem is one of the rare subsystems that deals with such a big number of devices (AFAIK). Most other subsystems don‘t really need that.
I do agree that converting the klist to a radix tree would be more involved, but at least I think we can keep this subsystem-local, at least for now. Introducing a local cache should be simple.
Cheers!
>
> -Scott
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-11-02 20:18 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-01 18:10 [PATCH] drivers/base/memory.c: memory subsys init: skip search for missing blocks Scott Cheloha
2019-11-01 19:00 ` David Hildenbrand
2019-11-01 19:20 ` David Hildenbrand
2019-11-01 22:32 ` Rick Lindsley
2019-11-01 22:47 ` David Hildenbrand
2019-11-02 19:43 ` Scott Cheloha
2019-11-02 20:18 ` David Hildenbrand
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).