Page MenuHomeFreeBSD

Type validating smr protected pointer accessors.
ClosedPublic

Authored by jeff on Feb 16 2020, 2:33 AM.
Tags
None
Referenced Files
Unknown Object (File)
Feb 14 2024, 2:29 AM
Unknown Object (File)
Feb 14 2024, 2:29 AM
Unknown Object (File)
Feb 11 2024, 1:48 PM
Unknown Object (File)
Feb 4 2024, 5:45 PM
Unknown Object (File)
Jan 23 2024, 6:41 AM
Unknown Object (File)
Jan 18 2024, 1:48 PM
Unknown Object (File)
Jan 14 2024, 6:11 AM
Unknown Object (File)
Dec 23 2023, 1:09 AM
Subscribers

Details

Summary

Cribbing a good idea from linux, use specific accessors for smr so that we can get some kinds of assertions. The basic notion is that when you load or store through a smr protected pointer you need to explain how you have access. This is done with an invariants only expression. For a load this is as simple as asserting that you're in the section. For a write you want to assert something like mtx_owned().

i.e.
x = smr_entered_load(foo, foo_smr);
x = smr_serialized_load(foo, mtx_owned(&foo_lock));
smr_serialized_store(foo, y, mtx_owned(&foo_lock));

smr_unserialized_store(foo, NULL, in_destructor);

Linux has a static analysis tool that takes annotated sections to discover invalid dereferences. That is nice but I think using a type that hides pointer contents is sufficient and makes it very explicit what consumers are doing. If someone casts their way out of it, it's no different from failing to use the type. Just as lock asserts have become a culturally mandatory practice, smr accessors should, and generic datatypes that implement concurrent access should follow a pattern to start to ingrain this practice.

This also provides the acq/rel barriers necessary. It assumes that the caller has made some modifications to the thing that it is storing and the thing that is loading it needs the corresponding barrier. For routines that don't need barriers they may use the unsynchronized variants to still be explicit. For example, in the destructor in vm_radix the structure is no longer visible so access is unsynchronized so as to not add overhead.

I will add a man page when all of this stabilizes but I am looking for design feedback now. I have used this in radix but I do not have the lock visible so it's not possible for me to use the assert portions for complicated layering reasons.

I'm concerned about the lack of annotation in epoch use in the network stack and how that may proliferate into anti-patterns. I would like to establish a safer, more obvious framework so that we maintain some debuggability.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

jeff added reviewers: alc, kib, markj, jhb, mjg, mmacy, rlibby.
jeff set the repository for this revision to rS FreeBSD src repository - subversion.
sys/sys/smr.h
92 ↗(On Diff #68390)

maybe smr_section_load

I think this avoidably loses the pointer type.

Note I recently patched atomic_load_ptr to return values with type based on the argument and cursory reading suggests this property can be used here.

We can take advantage of the fact this is all macros, so the idea would be like this:
SMR_DECLARE_TYPE(struct foo, foo_smr_t); or whatever

this would expand to:

struct ... {
     TYPE __ptr;
};

then you could drop the casts from atomic_load_ptr since it would start returning properly typed pointer.

Note the rel/acq variants are arch-specific and currently force uintptr_t. This should be fixed but there is more than one way to do it. Thus in the meantime this code can hack around by casting to the result of __typeof, see atomic_load_ptr itself for an example.

I like the suggestion but I'm not sure how I feel about declaring everywhere. Hopefully others will chime in.

In radix I defined accessors on top of this to handle types and asserts relevant to radix. They handled the casting.

unrelated, but, another variant I considered was something like:

smr_load(p, access, smr, ex)
Where access would be an enum or SMR_SECTION, SMR_SYNCHRONIZED, SMR_UNSYCHRONIZED

The switch would compile away but ultimately I'm not sure all accessors make sense for all synchronization types. It would be convenient for things like LIST macros that run in a lock or section.

Not sure what you mean by everywhere. It can be next to whatever struct is at play, which really should not be a problem to add given that all accesses will have to be augmented to use the new routines. I'm confident that's a small price to pay for allowing the compiler to provide type checking.

jeff edited the summary of this revision. (Show Details)

Implement mjg's suggestion

sys/sys/smr.h
107 ↗(On Diff #68391)

there should be no need to cast any of atomic_load_ptr uses (but there is a need to cast acq/rel routines)

128 ↗(On Diff #68391)

I don't think this macro is necessary.

easier would be something in the lines of:

__typeof(*(p)->__ptr) __myvar = (v);

And then stop using v. This has a side effect of not expanding it more than once.

Use temporary assignments to enforce types instead of asserts.

Expand the comment because there was some confusion around naming.

I am likely to commit this soon so that I can carry on with using it in
the VM. I am not opposed to changing it before 13 if there are good
suggestions. I will write man pages once it has settled.

This revision is now accepted and ready to land.Feb 17 2020, 8:27 PM
sys/sys/smr.h
134 ↗(On Diff #68473)

Why you need rel there ? Or a dual question, why serialized load does not have acq ?

sys/sys/smr.h
134 ↗(On Diff #68473)

Writers are intended to serialize with other writers using a conventional lock. So smr_serialized_* is for updating what smr_entered_load() will see. Ostensibly the pointer can't change while the write side lock is held but the reader needs to see any changes to the datastructure before the pointer is updated.

These semantics are intended to make the most common types of algorithms work as expected. For example, in a linked list primitive. I found this paper about rcu consumers and patterns relatively informative:

https://pdos.csail.mit.edu/6.828/2019/readings/rcu-decade-later.pdf

This revision was automatically updated to reflect the committed changes.