Hello Jan,
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?
>> +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.
[...]
