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

[Jan Beulich <jbeulich@xxxxxxxx>]

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