Page MenuHomeFreeBSD

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

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

Details

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

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

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 added inline comments.
sys/sys/_smr.h
45 ↗(On Diff #69269)

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 ↗(On Diff #69269)

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
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
sys/sys/_smr.h
39 ↗(On Diff #69270)

For this assert to work you will need to move the smr structure into _smr.h.

sys/sys/smr.h
38 ↗(On Diff #69270)

I would prefer a stronger comment here but I will write it later if that is ok.

sys/sys/_smr.h
39 ↗(On Diff #69270)

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 added inline comments.
sys/sys/_smr.h
45 ↗(On Diff #69269)

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.