Re: [RFC PATCH 04/10] xen: add reference counter support

[Jan Beulich <jbeulich@xxxxxxxx>]

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