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

[David Hildenbrand]

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