Re: OF_DYNAMIC node lifecycle

[Grant Likely <grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>]

On Tue, 24 Jun 2014 15:07:05 -0500, Nathan Fontenot <nfont-V7BBcbaFuwjMbYB6QlFGEg@public.gmane.org> wrote:
> On 06/23/2014 09:58 AM, Grant Likely wrote:
> > On Thu, 19 Jun 2014 11:33:20 +0300, Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org> wrote:
> >> Hi Grant,
> >>
> >> CCing Thomas Gleixner & Steven Rostedt, since they might have a few
> >> ideas...
> >>
> >> On Jun 18, 2014, at 11: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().
> >>>
> >>
> >> In hindsight it appears that drivers just can't get the lifecycle right.
> >> So we need to simplify things.
> >>
> >>> 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?
> >>> - How frequent are the changes? How many changes would be likely over
> >>> the runtime of the system?
> >>> - Are you able to verify that removed nodes are actually able to be
> >>> freed correctly? Do you have any testcases for node removal?
> >>>
> >>> 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.
> >>>
> >>> 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.
> >>>
> >>
> >> Since we're going about changing things, how about that devtree_lock?
> > 
> > I believe rcu would pretty much eliminate the devtree_lock entirely. All
> > modifiers would need to grab a mutex to ensure there is only one writer
> > at any given time, but readers would have free reign to parse the tree
> > however they like.
> > 
> > DT writers would have to follow some strict rules about how to handle
> > nodes that are removed (ie. don't modify or of_node_put() them until
> > after rcu is syncronized), but the number of writers is very small and
> > we have control of all of them.
> > 
> >> We're using a raw_spinlock and we're always taking the lock with
> >> interrupts disabled.
> >>
> >> If we're going to make DT changes frequently during normal runtime
> >> and not only during boot time, those are bad for any kind of real-time
> >> performance.
> >>
> >> So the question is, do we really have code that access the live tree
> >> during atomic sections?  Is that something we want? Enforcing this
> >> will make our lives easier, and we'll get the change to replace
> >> that spinlock with a mutex.
> > 
> > Yes, I believe the powerpc CPU hotplug code accesses the DT in atomic
> > sections. I cannot put my finger on the exact code however. Nathan might
> > know better. But, if I'm right, the whole problem goes away with RCU.
> 
> I went back through the cpu hotplug code. we do update the DT during cpu
> hotplug but I don't see it happening during atomic sections.
> 
> The code is in arch/powerpc/platforms/pseries/dlpar.c

Great, thanks,

By the way, notifiers currently get sent before any updates are applied
to the tree. I want to change it so that the notifier gets sent
afterwards. Does that work for you? I've looked through all the users
and aside from a stupid block of code in arch/powerpc/kernel/prom.c
which does things that should be done by of_attach_node(), it looks like
everything should be fine.

g.
--
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