Re: OF_DYNAMIC node lifecycle

[Nathan Fontenot <nfont-V7BBcbaFuwjMbYB6QlFGEg@public.gmane.org>]

On 06/25/2014 03:24 PM, Grant Likely wrote:
> On Tue, 24 Jun 2014 15:10:55 -0500, Nathan Fontenot <nfont-V7BBcbaFuwjMbYB6QlFGEg@public.gmane.org> wrote:
>> On 06/23/2014 09:48 AM, Grant Likely wrote:
>>> On Thu, 19 Jun 2014 10:26:15 -0500, Nathan Fontenot <nfont-V7BBcbaFuwjMbYB6QlFGEg@public.gmane.org> wrote:
>>>> On 06/18/2014 03:07 PM, Grant Likely wrote:
>>>>> Hi Nathan and Tyrel,
>>>>>
>>>>> I'm looking into lifecycle issues on nodes modified by OF_DYNAMIC, and
>>>>> I'm hoping you can help me. Right now, pseries seems to be the only
>>>>> user of OF_DYNAMIC, but making OF_DYNAMIC work has a huge impact on
>>>>> the entire kernel because it requires all DT code to manage reference
>>>>> counting with iterating over nodes. Most users simply get it wrong.
>>>>> Pantelis did some investigation and found that the reference counts on
>>>>> a running kernel are all over the place. I have my doubts that any
>>>>> code really gets it right.
>>>>>
>>>>> The problem is that users need to know when it is appropriate to call
>>>>> of_node_get()/of_node_put(). All list traversals that exit early need
>>>>> an extra call to of_node_put(), and code that is searching for a node
>>>>> in the tree and holding a reference to it needs to call of_node_get().
>>>>>
>>>>> I've got a few pseries questions:
>>>>> - What are the changes being requested by pseries firmware? Is it only
>>>>> CPUs and memory nodes, or does it manipulate things all over the tree?
>>>>
>>>> The short answer, everything.
>>>
>>> :-)
>>>
>>>> For pseries the two big actions that can change the device tree are
>>>> adding/removing resources and partition migration.
>>>>
>>>> The most frequent updates to the device tree happen during resource
>>>> (cpu, memory, and pci/phb) add and remove. During this process we add
>>>> and remove the node and its properties from the device tree.
>>>> - For memory on newer systems this just involves updating the
>>>>   ibm,dynamic-reconfiguration-memory/ibm,dynamic-memory property. Older
>>>>   firmware levels add and remove the memroy@XXX nodes and their properties.
>>>> - For cpus the cpus/PowerPC,POWERXXXX nodes and its properties are added
>>>>   or removed
>>>> - For pci/phb the pci@XXXXX nodes and properties are added/removed.
>>>>
>>>> The less frequent operation of live partition migration (and suspend/resume)
>>>> can update just about anything in the device tree. When this occurs and the
>>>> systems starts after being migrated (or waking up after a suspend) we make
>>>> a call to firmware to get updates to the device tree for the new hardware
>>>> we are running on.
>>>>  
>>>>> - How frequent are the changes? How many changes would be likely over
>>>>> the runtime of the system?
>>>>
>>>> This can happen frequently.
>>>
>>> Thanks, that is exactly the information that I want. I'm not so much
>>> concerned with the addition or removal of nodes/properties, which is
>>> actually pretty easy to handle. It is the lifecycle of allocations on
>>> dynamic nodes that causes heartburn.
>>>
>>>>> - Are you able to verify that removed nodes are actually able to be
>>>>> freed correctly? Do you have any testcases for node removal?
>>>>
>>>> I have always tested this by doing resource add/remove, usually cpu and memory
>>>> since it is the easiest.
>>>
>>> Is that just testing the functionality, or do you have tests that check
>>> if the memory gets freed?
>>
>> In general it's just functionality testing.
>>
>>>
>>>>> I'm thinking very seriously about changing the locking semantics of DT
>>>>> code entirely so that most users never have to worry about
>>>>> of_node_get/put at all. If the DT code is switched to use rcu
>>>>> primitives for tree iteration (which also means making DT code use
>>>>> list_head, something I'm already investigating), then instead of
>>>>> trying to figure out of_node_get/put rules, callers could use
>>>>> rcu_read_lock()/rcu_read_unlock() to protect the region that is
>>>>> searching over nodes, and only call of_node_get() if the node pointer
>>>>> is needed outside the rcu read-side lock.
>>>>>
>>>>
>>>> This sounds good. I like just taking the rcu lock around accessing the DT.
>>>> Do we have many places where DT node pointers are held that require
>>>> keeping the of_node_get/put calls? If this did exist perhaps we could
>>>> update those places to look up the DT node every time instead of
>>>> holding on to the pointer. We could just get rid of the reference counting
>>>> altogether then.
>>>
>>> There are a few, but I would be happy to restrict reference counting to
>>> only those locations. Most places will decode the DT data, and then
>>> throw away the reference. We /might/ even be able to do rcu_lock/unlock
>>> around the entire probe path which would make it transparent to all
>>> device drivers.
>>>
>>>>> I'd really like to be rid of the node reference counting entirely, but
>>>>> I can't figure out a way of doing that safely, so I'd settle for
>>>>> making it a lot easier to get correct.
>>>>>
>>>>
>>>> heh! I have often thought about adding reference counting to device tree
>>>> properties.
>>>
>>> You horrible, horrible man.
>>
>> Yes. I are evil :)
>>
>> After looking again the work needed to add reference counts to properties
>> would be huge. The few properties I am concerned with are specific to powerpc
>> so perhaps just adding an arch specific lock around updating those
>> properties would work.
> 
> Which code/properties? I'd like to have a look myself.

/ibm,dynamic-reconfiguration-memory/ibm,dynamic-memory

The property is updated in 
arch/powerpc/platforms/pseries/hotplug-memory.c:pseries_update_drconf_memory()

-Nathan

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html