| Message ID | 20220831141040.13231-5-volodymyr_babchuk@epam.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Rework PCI locking | expand |
On 31.08.2022 16:10, Volodymyr Babchuk wrote: > --- /dev/null > +++ b/xen/include/xen/refcnt.h > @@ -0,0 +1,28 @@ > +#ifndef __XEN_REFCNT_H__ > +#define __XEN_REFCNT_H__ > + > +#include <asm/atomic.h> > + > +typedef atomic_t refcnt_t; Like Linux has it, I think this would better be a separate struct. At least in debug builds, i.e. it could certainly use typesafe.h if that ended up to be a good fit (which I'm not sure it would, so this is merely a thought). > +static inline void refcnt_init(refcnt_t *refcnt) > +{ > + atomic_set(refcnt, 1); > +} > + > +static inline void refcnt_get(refcnt_t *refcnt) > +{ > +#ifndef NDEBUG > + ASSERT(atomic_add_unless(refcnt, 1, 0) > 0); > +#else > + atomic_add_unless(refcnt, 1, 0); > +#endif > +} I think this wants doing without any #ifdef-ary, e.g. static inline void refcnt_get(refcnt_t *refcnt) { int ret = atomic_add_unless(refcnt, 1, 0); ASSERT(ret > 0); } I wonder though whether certain callers may not want to instead know whether a refcount was successfully obtained, i.e. whether instead of asserting here you don't want to return a boolean success indicator, which callers then would deal with (either by asserting or by suitably handling the case). See get_page() and page_get_owner_and_reference() for similar behavior we have (and use) already. > +static inline void refcnt_put(refcnt_t *refcnt, void (*destructor)(refcnt_t *refcnt)) > +{ > + if ( atomic_dec_and_test(refcnt) ) > + destructor(refcnt); > +} No assertion here as to the count being positive? Also the entire file wants to use Xen's space indentation, not hard tabs. Jan
Hello Jan, Jan Beulich <jbeulich@suse.com> writes: > On 31.08.2022 16:10, Volodymyr Babchuk wrote: >> --- /dev/null >> +++ b/xen/include/xen/refcnt.h >> @@ -0,0 +1,28 @@ >> +#ifndef __XEN_REFCNT_H__ >> +#define __XEN_REFCNT_H__ >> + >> +#include <asm/atomic.h> >> + >> +typedef atomic_t refcnt_t; > > Like Linux has it, I think this would better be a separate struct. At > least in debug builds, i.e. it could certainly use typesafe.h if that > ended up to be a good fit (which I'm not sure it would, so this is > merely a thought). Sadly, TYPE_SAFE does not support pointers. e.g I can't get pointer to an encapsulated value which is also passed as a pointer. I can expand TYPE_SAFE with $FOO_x_ptr(): static inline _type *_name##_x_ptr(_name##_t *n) { &return n->_name; } or make custom encapsulation in refcnt.h. Which one you prefer? >> +static inline void refcnt_init(refcnt_t *refcnt) >> +{ >> + atomic_set(refcnt, 1); >> +} >> + >> +static inline void refcnt_get(refcnt_t *refcnt) >> +{ >> +#ifndef NDEBUG >> + ASSERT(atomic_add_unless(refcnt, 1, 0) > 0); >> +#else >> + atomic_add_unless(refcnt, 1, 0); >> +#endif >> +} > I think this wants doing without any #ifdef-ary, e.g. > > static inline void refcnt_get(refcnt_t *refcnt) > { > int ret = atomic_add_unless(refcnt, 1, 0); > > ASSERT(ret > 0); > } > Thanks, did as you suggested. I was afraid that compiler would complain about unused ret in non-debug builds. > I wonder though whether certain callers may not want to instead know > whether a refcount was successfully obtained, i.e. whether instead of > asserting here you don't want to return a boolean success indicator, > which callers then would deal with (either by asserting or by suitably > handling the case). See get_page() and page_get_owner_and_reference() > for similar behavior we have (and use) already. For now there are no such callers, so I don't want to implement unused functionality. But, if you prefer this way, I'll do this. [...]
On 17.02.2023 02:56, Volodymyr Babchuk wrote: > Jan Beulich <jbeulich@suse.com> writes: >> On 31.08.2022 16:10, Volodymyr Babchuk wrote: >>> --- /dev/null >>> +++ b/xen/include/xen/refcnt.h >>> @@ -0,0 +1,28 @@ >>> +#ifndef __XEN_REFCNT_H__ >>> +#define __XEN_REFCNT_H__ >>> + >>> +#include <asm/atomic.h> >>> + >>> +typedef atomic_t refcnt_t; >> >> Like Linux has it, I think this would better be a separate struct. At >> least in debug builds, i.e. it could certainly use typesafe.h if that >> ended up to be a good fit (which I'm not sure it would, so this is >> merely a thought). > > Sadly, TYPE_SAFE does not support pointers. e.g I can't get pointer to > an encapsulated value which is also passed as a pointer. I can expand > TYPE_SAFE with $FOO_x_ptr(): > > static inline _type *_name##_x_ptr(_name##_t *n) { &return n->_name; } > > or make custom encapsulation in refcnt.h. Which one you prefer? First of all, as said - typesafe.h may not be a good fit. And then the helper you suggest looks to be UB if the passed in pointer was to an array rather than a singular object, so having something like that in a very generic piece of infrastructure is inappropriate anyway. >>> +static inline void refcnt_init(refcnt_t *refcnt) >>> +{ >>> + atomic_set(refcnt, 1); >>> +} >>> + >>> +static inline void refcnt_get(refcnt_t *refcnt) >>> +{ >>> +#ifndef NDEBUG >>> + ASSERT(atomic_add_unless(refcnt, 1, 0) > 0); >>> +#else >>> + atomic_add_unless(refcnt, 1, 0); >>> +#endif >>> +} > >> I think this wants doing without any #ifdef-ary, e.g. >> >> static inline void refcnt_get(refcnt_t *refcnt) >> { >> int ret = atomic_add_unless(refcnt, 1, 0); >> >> ASSERT(ret > 0); >> } >> > > Thanks, did as you suggested. I was afraid that compiler would complain > about unused ret in non-debug builds. > >> I wonder though whether certain callers may not want to instead know >> whether a refcount was successfully obtained, i.e. whether instead of >> asserting here you don't want to return a boolean success indicator, >> which callers then would deal with (either by asserting or by suitably >> handling the case). See get_page() and page_get_owner_and_reference() >> for similar behavior we have (and use) already. > > For now there are no such callers, so I don't want to implement unused > functionality. But, if you prefer this way, I'll do this. Well, I can see your point about unused functionality. That needs to be weighed against this being a pretty basic piece of infrastructure, which may want using elsewhere as well. Such re-use would then better not trigger touching all the code which already uses it (in principle the domain ref counting might be able to re-use it, for example, but there's that DOMAIN_DESTROYED special case which may require it to continue to have a custom implementation). What you may want to do is check Linux'es equivalent. Depending on how close ours is going to be, using the same naming may also want considering. Jan
Hi Jan, Jan Beulich <jbeulich@suse.com> writes: > On 17.02.2023 02:56, Volodymyr Babchuk wrote: >> Jan Beulich <jbeulich@suse.com> writes: >>> On 31.08.2022 16:10, Volodymyr Babchuk wrote: >>>> --- /dev/null >>>> +++ b/xen/include/xen/refcnt.h >>>> @@ -0,0 +1,28 @@ >>>> +#ifndef __XEN_REFCNT_H__ >>>> +#define __XEN_REFCNT_H__ >>>> + >>>> +#include <asm/atomic.h> >>>> + >>>> +typedef atomic_t refcnt_t; >>> >>> Like Linux has it, I think this would better be a separate struct. At >>> least in debug builds, i.e. it could certainly use typesafe.h if that >>> ended up to be a good fit (which I'm not sure it would, so this is >>> merely a thought). >> >> Sadly, TYPE_SAFE does not support pointers. e.g I can't get pointer to >> an encapsulated value which is also passed as a pointer. I can expand >> TYPE_SAFE with $FOO_x_ptr(): >> >> static inline _type *_name##_x_ptr(_name##_t *n) { &return n->_name; } >> >> or make custom encapsulation in refcnt.h. Which one you prefer? > > First of all, as said - typesafe.h may not be a good fit. And then the > helper you suggest looks to be UB if the passed in pointer was to an > array rather than a singular object, so having something like that in > a very generic piece of infrastructure is inappropriate anyway. Okay, no problem. I'll use a separate struct. Also, I played a bit with compiler outputs. Looks like there is no additional overhead in reading single value from a struct. So I don't think that we need an additional non-debug implementation for this type. >>>> +static inline void refcnt_init(refcnt_t *refcnt) >>>> +{ >>>> + atomic_set(refcnt, 1); >>>> +} >>>> + >>>> +static inline void refcnt_get(refcnt_t *refcnt) >>>> +{ >>>> +#ifndef NDEBUG >>>> + ASSERT(atomic_add_unless(refcnt, 1, 0) > 0); >>>> +#else >>>> + atomic_add_unless(refcnt, 1, 0); >>>> +#endif >>>> +} >> >>> I think this wants doing without any #ifdef-ary, e.g. >>> >>> static inline void refcnt_get(refcnt_t *refcnt) >>> { >>> int ret = atomic_add_unless(refcnt, 1, 0); >>> >>> ASSERT(ret > 0); >>> } >>> >> >> Thanks, did as you suggested. I was afraid that compiler would complain >> about unused ret in non-debug builds. >> >>> I wonder though whether certain callers may not want to instead know >>> whether a refcount was successfully obtained, i.e. whether instead of >>> asserting here you don't want to return a boolean success indicator, >>> which callers then would deal with (either by asserting or by suitably >>> handling the case). See get_page() and page_get_owner_and_reference() >>> for similar behavior we have (and use) already. >> >> For now there are no such callers, so I don't want to implement unused >> functionality. But, if you prefer this way, I'll do this. > > Well, I can see your point about unused functionality. That needs to be > weighed against this being a pretty basic piece of infrastructure, which > may want using elsewhere as well. Such re-use would then better not > trigger touching all the code which already uses it (in principle the > domain ref counting might be able to re-use it, for example, but there's > that DOMAIN_DESTROYED special case which may require it to continue to > have a custom implementation). > > What you may want to do is check Linux'es equivalent. Depending on how > close ours is going to be, using the same naming may also want considering. I wrote my implementation from scratch to avoid any potential licensing issues. But, looking at Linux implementation: There are two abstractions: struct refcount and struct kref. Struct refcount is like atomic_t but with saturation to avoid wrapping. Struct kref is built on top of struct refcount. It is tailored to handle reference counted objects by having ability to call release() function when refcounter reaches zero. Both kref_get() and refcount_inc() functions return void. My implementation has no separation on this two types - ref counter with saturation and kernel object reference counter. My implementation does only latter thing. It is a good idea to add saturation and I will do this in the next patch version. As for details on function prototypes and type names - I'll do as you say. If you want refcnt_put() to return bool - no problem. If you want this functionality renamed or aligned with Linux's one - just tell me. From my point of view, right now we have minimal implementation that covers all available use cases and can be easily expended in the future to cover new use cases. For use cases I can see PCI, cpupool and maybe couple of ARM IOMMU drivers. All others: - get_domain() uses that DOMAIN_DESTROYED special case you mentioned - {get,put}_page* does not use atomic_t all and rely on direct cmpxchg() call for some reason. - OP-TEE code is happy with atomics due to complex logic - {get,put}_cpu_var and put_gfn does not use ref counting at all
diff --git a/xen/include/xen/refcnt.h b/xen/include/xen/refcnt.h new file mode 100644 index 0000000000..7f5395a21c --- /dev/null +++ b/xen/include/xen/refcnt.h @@ -0,0 +1,28 @@ +#ifndef __XEN_REFCNT_H__ +#define __XEN_REFCNT_H__ + +#include <asm/atomic.h> + +typedef atomic_t refcnt_t; + +static inline void refcnt_init(refcnt_t *refcnt) +{ + atomic_set(refcnt, 1); +} + +static inline void refcnt_get(refcnt_t *refcnt) +{ +#ifndef NDEBUG + ASSERT(atomic_add_unless(refcnt, 1, 0) > 0); +#else + atomic_add_unless(refcnt, 1, 0); +#endif +} + +static inline void refcnt_put(refcnt_t *refcnt, void (*destructor)(refcnt_t *refcnt)) +{ + if ( atomic_dec_and_test(refcnt) ) + destructor(refcnt); +} + +#endif
We can use reference counter to ease up object lifetime management. This patch adds very basic support for reference counters. refcnt should be used in the following way: 1. Protected structure should have refcnt_t field 2. This field should be initialized with refcnt_init() during object construction. 3. If code holds a valid pointer to a structure/object it can increase refcount with refcnt_get(). No additional locking is required. 4. Code should call refcnt_put() before dropping pointer to a protected structure. `destructor` is a call back function that should destruct object and free all resources, including structure protected itself. Destructor will be called if reference counter reaches zero. 5. If code does not hold a valid pointer to a protected structure it should use other locking mechanism to obtain a pointer. For example, it should lock a list that hold protected objects. Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com> --- xen/include/xen/refcnt.h | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) create mode 100644 xen/include/xen/refcnt.h