Move SMR pointer type definition and access macros to smr_types.h.
ClosedPublic
Actions

Authored by markj on Mar 6 2020, 4:52 PM.

Details

Reviewers

mjg
jeff
rlibby

Commits

rS358728: Move SMR pointer type definition and access macros to smr_types.h.

Summary

The intent is to provide a header that can be included by other headers
without introducing too much pollution. smr.h depends on various
headers and will likely grow over time, but is less likely to be
required by system headers.

Move SMR assertion macros to _smr.h. I don't think they belong in
smr_types.h, but smr_types.h macros rely on them.

Rename SMR_TYPE_DECLARE() to SMR_POINTER(). There are two motivations
for this: 1) One might use SMR to protect more than just pointers; it
could be used for resizeable arrays, for example, so TYPE seems too
generic. 2) It would be nice to be able to define anonymous pointer
types, akin to TAILQ_HEAD/_ENTRY, and the _DECLARE suffix makes that
look wrong. I think

struct foo {
	SMR_POINTER(struct bar *)	p;
};

is nicer, and one can write

typedef SMR_POINTER(struct bar *) smrbar_t;

if that happens to be useful.

Diff Detail

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 29830
Build 27656: arc lint + arc unit

Event Timeline

markj created this revision.Mar 6 2020, 4:52 PM

Harbormaster completed remote builds in B29829: Diff 69269.Mar 6 2020, 4:52 PM

markj mentioned this in D23889: fd: use SMR for managing struct pwd.Mar 6 2020, 4:55 PM

markj added reviewers: mjg, jeff, rlibby.

Can you please add:

/*
 * Special case to handle headers which are partially shared with userspace.
 */
#define smr_kvm_load(p) ({                                              \
        (__typeof((p)->__ptr))atomic_load_ptr((uintptr_t *)&(p)->__ptr); \
})

or an equivalent. see the D23889 review for the use case.

jeff accepted this revision.Mar 6 2020, 5:52 PM

jeff added inline comments.

sys/sys/_smr.h
45	Should possibly use SMR_ASSERT for this

This revision is now accepted and ready to land.Mar 6 2020, 5:52 PM

In D23988#527323, @mjg wrote:

Can you please add:

/*
 * Special case to handle headers which are partially shared with userspace.
 */
#define smr_kvm_load(p) ({                                              \
        (__typeof((p)->__ptr))atomic_load_ptr((uintptr_t *)&(p)->__ptr); \
})

or an equivalent. see the D23889 review for the use case.

Ok. Not sure why it needs to be an atomic load.

sys/sys/_smr.h
45	It'd then print "Assertion curthread->td_critnest != 0 && ... failed" which seems less clear than "In smr section".

Add an accessor for userspace code using libkvm to get at kernel structures.

This revision now requires review to proceed.Mar 6 2020, 5:57 PM

Harbormaster completed remote builds in B29830: Diff 69270.Mar 6 2020, 5:57 PM

In D23988#527329, @markj wrote:
In D23988#527323, @mjg wrote:
Can you please add:
/*
 * Special case to handle headers which are partially shared with userspace.
 */
#define smr_kvm_load(p) ({                                              \
        (__typeof((p)->__ptr))atomic_load_ptr((uintptr_t *)&(p)->__ptr); \
})
or an equivalent. see the D23889 review for the use case.
Ok. Not sure why it needs to be an atomic load.

It does not. It's the original accessor stripped of the acquire barrier.

This revision is now accepted and ready to land.Mar 6 2020, 6:03 PM

mjg mentioned this in D23912: smr: move accessors to sys/_smr.h.Mar 6 2020, 6:04 PM

jeff added inline comments.Mar 6 2020, 6:12 PM

sys/sys/_smr.h
39	For this assert to work you will need to move the smr structure into _smr.h.
sys/sys/smr.h
38	I would prefer a stronger comment here but I will write it later if that is ok.

markj added inline comments.Mar 6 2020, 7:06 PM

sys/sys/_smr.h
39	If code is using SMR_ENTERED() then it will presumably have called smr_enter() somewhere in the same file, in which case smr.h is included already. Most of the macros in smr_types.h can be used without smr.h.

rlibby accepted this revision.Mar 6 2020, 8:02 PM

rlibby added inline comments.

sys/sys/_smr.h
45	It seems like SMR_ASSERT's value is providing the equivalent of `__func__` for the macros where it's used, which doesn't seem valuable for SMR_ASSERT_ENTERED/NOT_ENTERED.