Re: [PATCH] of: add early boot allocation of of_find_node_by_phandle() cache

[Frank Rowand]

On 02/16/18 01:07, Chintan Pandya wrote:
> 
> 
> On 2/15/2018 6:14 AM, frowand.list@xxxxxxxxx wrote:
>> From: Frank Rowand <frank.rowand@xxxxxxxx>
>>
>> The initial implementation of the of_find_node_by_phandle() cache
>> allocates the cache using kcalloc(). Add an early boot allocation
>> of the cache so it will be usable during early boot. Switch over
>> to the kcalloc() based cache once normal memory allocation
>> becomes available.
>>
>> Signed-off-by: Frank Rowand <frank.rowand@xxxxxxxx>
>> ---
>>
>> This patch is optional, to be added at Rob's discretion. The
>> extra complexity is not as much as I had feared, but the boot
>> speed up is also likely small.
>>
>> Â drivers/of/base.cÂÂÂÂÂÂ | 33 +++++++++++++++++++++++++++++++++
>> Â drivers/of/fdt.cÂÂÂÂÂÂÂ |Â 2 ++
>> Â drivers/of/of_private.h |Â 2 ++
>> Â 3 files changed, 37 insertions(+)
>>
>> diff --git a/drivers/of/base.c b/drivers/of/base.c
>> index ab545dfa9173..d7b1ff1209e8 100644
>> --- a/drivers/of/base.c
>> +++ b/drivers/of/base.c
>> @@ -16,9 +16,11 @@
>> Â Â #define pr_fmt(fmt)ÂÂÂ "OF: " fmt
>> Â +#include <linux/bootmem.h>
>> Â #include <linux/console.h>
>> Â #include <linux/ctype.h>
>> Â #include <linux/cpu.h>
>> +#include <linux/memblock.h>
>> Â #include <linux/module.h>
>> Â #include <linux/of.h>
>> Â #include <linux/of_device.h>
>> @@ -131,6 +133,29 @@ static void of_populate_phandle_cache(void)
>> ÂÂÂÂÂ raw_spin_unlock_irqrestore(&devtree_lock, flags);
>> Â }
>> Â +void __init of_populate_phandle_cache_early(void)
>> +{
>> +ÂÂÂ u32 cache_entries;
>> +ÂÂÂ struct device_node *np;
>> +ÂÂÂ u32 phandles = 0;
>> +ÂÂÂ size_t size;
>> +
>> +ÂÂÂ for_each_of_allnodes(np)
>> +ÂÂÂÂÂÂÂ if (np->phandle && np->phandle != OF_PHANDLE_ILLEGAL)
>> +ÂÂÂÂÂÂÂÂÂÂÂ phandles++;
>> +
>> +ÂÂÂ cache_entries = roundup_pow_of_two(phandles);
>> +ÂÂÂ phandle_cache_mask = cache_entries - 1;
>> +
>> +ÂÂÂ size = cache_entries * sizeof(*phandle_cache);
>> +ÂÂÂ phandle_cache = memblock_virt_alloc(size, 4);
>> +ÂÂÂ memset(phandle_cache, 0, size);
>> +
>> +ÂÂÂ for_each_of_allnodes(np)
>> +ÂÂÂÂÂÂÂ if (np->phandle && np->phandle != OF_PHANDLE_ILLEGAL)
>> +ÂÂÂÂÂÂÂÂÂÂÂ phandle_cache[np->phandle & phandle_cache_mask] = np;
>> +}
> 
> There is a lot of code duplication in this function with
> of_populate_phandle_cache. Would you think of taking out
> common code or differ the function with extra bool parameter
> to say 'early' or 'not early'.

Good observation, and normally yes.  My first implementation of this
feature actually did what you suggest.

It turns out to be a bit more complicated than one might expect
because some of the code is marked __init.  That results in
passing the memory allocation function as a parameter to
of_populate_phandle_cache().  See __unflatten_device_tree() for
an example of what this entails.  Then the parts of
of_populate_cache() that need to be encapsulated in a 'if (!early)'
test are scattered throughout the function, so the test becomes
rather intrusive in terms of code readability.

In the end, the method I chose results in cleaner code for
of_populate_phandle_cache(), plus the memory used by
of_populate_cache_early() gets reclaimed after boot, since
it is marked __init.  Both functions are relatively small and
the code common to both is unlikely to be modified, so I do
not see this causing a maintenance burden.

-Frank

>> +
>> Â #ifndef CONFIG_MODULES
>> Â static int __init of_free_phandle_cache(void)
>> Â {
>> @@ -150,7 +175,15 @@ static int __init of_free_phandle_cache(void)
>> Â Â void __init of_core_init(void)
>> Â {
>> +ÂÂÂ unsigned long flags;
>> ÂÂÂÂÂ struct device_node *np;
>> +ÂÂÂ phys_addr_t size;
>> +
>> +ÂÂÂ raw_spin_lock_irqsave(&devtree_lock, flags);
>> +ÂÂÂ size = (phandle_cache_mask + 1) * sizeof(*phandle_cache);
>> +ÂÂÂ memblock_free(__pa(phandle_cache), size);
>> +ÂÂÂ phandle_cache = NULL;
>> +ÂÂÂ raw_spin_unlock_irqrestore(&devtree_lock, flags);
>> Â ÂÂÂÂÂ of_populate_phandle_cache();
>> Â diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
>> index 84aa9d676375..cb320df23f26 100644
>> --- a/drivers/of/fdt.c
>> +++ b/drivers/of/fdt.c
>> @@ -1264,6 +1264,8 @@ void __init unflatten_device_tree(void)
>> ÂÂÂÂÂ of_alias_scan(early_init_dt_alloc_memory_arch);
>> Â ÂÂÂÂÂ unittest_unflatten_overlay_base();
>> +
>> +ÂÂÂ of_populate_phandle_cache_early();
>> Â }
>> Â Â /**
>> diff --git a/drivers/of/of_private.h b/drivers/of/of_private.h
>> index fa70650136b4..6720448c84cc 100644
>> --- a/drivers/of/of_private.h
>> +++ b/drivers/of/of_private.h
>> @@ -134,6 +134,8 @@ extern void __of_sysfs_remove_bin_file(struct device_node *np,
>> Â /* illegal phandle value (set when unresolved) */
>> Â #define OF_PHANDLE_ILLEGALÂÂÂ 0xdeadbeef
>> Â +extern void __init of_populate_phandle_cache_early(void);
>> +
>> Â /* iterators for transactions, used for overlays */
>> Â /* forward iterator */
>> Â #define for_each_transaction_entry(_oft, _te) \
>>
> 
> Chintan