On 17.02.2023 02:56, Volodymyr Babchuk wrote: > Jan Beulich <jbeul...@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