Re: [PATCH v4] drivers/base/memory.c: cache blocks in radix tree to accelerate lookup

David Hildenbrand
2020-01-20T09:15:44Z

On 17.01.20 10:35, Michal Hocko wrote:
> On Thu 16-01-20 17:17:54, David Hildenbrand wrote:
> [...]
>> diff --git a/drivers/base/memory.c b/drivers/base/memory.c
>> index c6d288fad493..c75dec35de43 100644
>> --- a/drivers/base/memory.c
>> +++ b/drivers/base/memory.c
>> @@ -19,7 +19,7 @@
>> #include <linux/memory.h>
>> #include <linux/memory_hotplug.h>
>> #include <linux/mm.h>
>> -#include <linux/radix-tree.h>
>> +#include <linux/xarray.h>
>> #include <linux/stat.h>
>> #include <linux/slab.h>
>>
>> @@ -58,11 +58,11 @@ static struct bus_type memory_subsys = {
>> };
>>
>> /*
>> - * Memory blocks are cached in a local radix tree to avoid
>> + * Memory blocks are cached in a local xarray to avoid
>> * a costly linear search for the corresponding device on
>> * the subsystem bus.
>> */
>> -static RADIX_TREE(memory_blocks, GFP_KERNEL);
>> +static DEFINE_XARRAY(memory_blocks);
>>
>> static BLOCKING_NOTIFIER_HEAD(memory_chain);
>>
>> @@ -566,7 +566,7 @@ static struct memory_block *find_memory_block_by_id(unsigned long block_id)
>> {
>> struct memory_block *mem;
>>
>> - mem = radix_tree_lookup(&memory_blocks, block_id);
>> + mem = xa_load(&memory_blocks, block_id);
>> if (mem)
>> get_device(&mem->dev);
>> return mem;
>> @@ -621,7 +621,8 @@ int register_memory(struct memory_block *memory)
>> put_device(&memory->dev);
>> return ret;
>> }
>> - ret = radix_tree_insert(&memory_blocks, memory->dev.id, memory);
>> + ret = xa_err(xa_store(&memory_blocks, memory->dev.id, memory,
>> + GFP_KERNEL));
>> if (ret) {
>> put_device(&memory->dev);
>> device_unregister(&memory->dev);
>> @@ -683,7 +684,7 @@ static void unregister_memory(struct memory_block *memory)
>> if (WARN_ON_ONCE(memory->dev.bus != &memory_subsys))
>> return;
>>
>> - WARN_ON(radix_tree_delete(&memory_blocks, memory->dev.id) == NULL);
>> + WARN_ON(xa_erase(&memory_blocks, memory->dev.id) == NULL);
>>
>> /* drop the ref. we got via find_memory_block() */
>> put_device(&memory->dev);
>
> OK, this looks sensible. xa_store shouldn't ever return an existing
> device as we do the lookpup beforehand so good. We might need to
> reorganize the code if we want to drop the loopup though.

Ping Scott. Will you resend a cleanup like this as a proper patch or
shall I?

--
Thanks,

David / dhildenb