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

[Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>]

Hi Jan,

Jan Beulich <jbeulich@xxxxxxxx> writes:

> 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.

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