Rework storage for Accessors #678
Conversation
There was a problem hiding this comment.
I'm not very familiar with the context around this tree structure and how it's used, so I've tried to focus on the implementation of this change. Overall, this seems good, and most of my comments were style nits. I think it might be worth using NonNull rather than *const for the key type, so that the null case can be represented using an Option.
Regarding the poisoning operation on trees, what does this actually represent and when do we do it? A comment on the method might be nice, IMO, although I understand that there wasn't one previously, so it doesn't necessarily have to be added in this PR...I'll look at the rest of the codebase and try to give a more thorough review
lib/propolis/src/accessors.rs
Outdated
| let mut ent = node.0.lock().unwrap(); | ||
| ent.tree = Arc::clone(&tree_ref); | ||
| ent.resource = self.resource_root.clone(); | ||
| drop(ent); |
There was a problem hiding this comment.
silly style nit, feel free to disagree: in general, I prefer to use scopes with curly-braces for stuff like this, rather than explicit drops, it makes the portion of code for which the lock is held a little more visually obvious to readers.
also, i notice that we do it that way later in the file, so the consistency might be nice?
There was a problem hiding this comment.
This should be cleaned up, as well as the other instances I found. (I didn't bother going through the tests, though)
lib/propolis/src/accessors.rs
Outdated
| pub struct NodeKey(*const Node<c_void>); | ||
| impl NodeKey { | ||
| const NULL: Self = Self(std::ptr::null()); | ||
| } |
There was a problem hiding this comment.
IMO, it might be nicer to represent this as NonNull<Node<c_void>>, allowing us to use Option<NodeKey> rather than treating NodeKey as nullable (and using NonNull means that Option<NodeKey> should be represented the same as NodeKey(ptr::null()) due to niche optimization).
This way, code that touches a nullable NodeKey (the parent ID) is forced to handle the null case, while the compiler ensures that other NodeKeys that are not the parent key are never null.
There was a problem hiding this comment.
I waffled a bit on this while writing it. It makes some accesses a bit more annoying, and further suggests to one examining NodeKey that the pointer it holds might actually be useful for something (beyond a unique ID). The help from the compiler is probably worth it, though.
|
Thanks for taking a look.
It's there mainly for the top of the accessor hierarchy to quickly withdraw access to the underlying resource in as prompt a manner as possible. Consumers may be in the middle of an access (with a guard held) which would delay the process somewhat, but absent that, it should allow the resource to be freed quickly. I've added some detail to the |
The initial storage (trees/nodes) and lifecycle model for Accessors left open a race between node self-cleanup (removing the associated entry from a tree on drop) and racing node re-parenting (as part of node adoption or orphaning). This changes how nodes are cleaned up from their containing tree, and simplifies indexing of nodes in said tree to use (opaque) pointers, rather than arbitrary numeric IDs. Fixes oxidecomputer#675
The initial storage (trees/nodes) and lifecycle model for Accessors left open a race between node self-cleanup (removing the associated entry from a tree on drop) and racing node re-parenting (as part of node adoption or orphaning).
This changes how nodes are cleaned up from their containing tree, and simplifies indexing of nodes in said tree to use (opaque) pointers, rather than arbitrary numeric IDs.
Fixes #675