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 <[email protected]>
---
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
> Am 01.11.2019 um 23:32 schrieb Rick Lindsley <[email protected]>:
>
> 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
On Fri, Nov 01, 2019 at 11:47:49PM +0100, David Hildenbrand wrote:
>
> > Am 01.11.2019 um 23:32 schrieb Rick Lindsley <[email protected]>:
> >
> > 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
> Am 02.11.2019 um 20:43 schrieb Scott Cheloha <[email protected]>:
>
> On Fri, Nov 01, 2019 at 11:47:49PM +0100, David Hildenbrand wrote:
>>
>>>> Am 01.11.2019 um 23:32 schrieb Rick Lindsley <[email protected]>:
>>>
>>> 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