Re: [PATCH v3 1/6] xen: add reference counter support

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Tue, Mar 14, 2023 at 08:56:29PM +0000, Volodymyr Babchuk wrote:
> We can use reference counter to ease up object lifetime management.
> This patch adds very basic support for reference counters. refcnt
> should be used in the following way:
> 
> 1. Protected structure should have refcnt_t field
> 
> 2. This field should be initialized with refcnt_init() during object
> construction.
> 
> 3. If code holds a valid pointer to a structure/object it can increase
> refcount with refcnt_get(). No additional locking is required.
> 
> 4. Code should call refcnt_put() before dropping pointer to a
> protected structure. `destructor` is a call back function that should
> destruct object and free all resources, including structure protected
> itself. Destructor will be called if reference counter reaches zero.
> 
> 5. If code does not hold a valid pointer to a protected structure it
> should use other locking mechanism to obtain a pointer. For example,
> it should lock a list that hold protected objects.

Sorry, I didn't look at the previous versions, but did we consider
importing refcount_t and related logic from Linux?

> 
> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@xxxxxxxx>
> 
> ---
> v3:
>   - moved in from another patch series
>   - used structure to encapsulate refcnt_t
>   - removed #ifndef NDEBUG in favor of just calling ASSERT
>   - added assertion to refcnt_put
>   - added saturation support: code catches overflow and underflow
>   - added EMACS magic at end of the file
>   - fixed formatting
> ---
>  xen/include/xen/refcnt.h | 59 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 59 insertions(+)
>  create mode 100644 xen/include/xen/refcnt.h
> 
> diff --git a/xen/include/xen/refcnt.h b/xen/include/xen/refcnt.h
> new file mode 100644
> index 0000000000..1ac05d927c
> --- /dev/null
> +++ b/xen/include/xen/refcnt.h
> @@ -0,0 +1,59 @@

This seems to be missing some kind of license, can we have an SPDX tag
at least?

> +#ifndef __XEN_REFCNT_H__
> +#define __XEN_REFCNT_H__
> +
> +#include <asm/atomic.h>
> +#include <asm/bug.h>
> +
> +#define REFCNT_SATURATED (INT_MIN / 2)
> +
> +typedef struct {
> +    atomic_t refcnt;
> +} refcnt_t;
> +
> +static inline void refcnt_init(refcnt_t *refcnt)
> +{
> +    atomic_set(&refcnt->refcnt, 1);
> +}
> +
> +static inline int refcnt_read(refcnt_t *refcnt)

const.

> +{
> +    return atomic_read(&refcnt->refcnt);
> +}
> +
> +static inline void refcnt_get(refcnt_t *refcnt)
> +{
> +    int old = atomic_add_unless(&refcnt->refcnt, 1, 0);
> +
> +    if ( unlikely(old < 0) || unlikely (old + 1 < 0) )
                                         ^ extra space

You want a single unlikely for both conditions.

> +    {
> +        atomic_set(&refcnt->refcnt, REFCNT_SATURATED);
> +        printk(XENLOG_ERR"refcnt saturation: old = %d\n", old);

Should this be printed only once for refcount? I fear it might spam
the console once a refcnt hits it.

> +        WARN();
> +    }
> +}
> +
> +static inline void refcnt_put(refcnt_t *refcnt, void (*destructor)(refcnt_t 
> *refcnt))
> +{
> +    int ret = atomic_dec_return(&refcnt->refcnt);
> +
> +    if ( ret == 0 )
> +        destructor(refcnt);
> +
> +    if ( unlikely(ret < 0))
                            ^ missing space
> +    {
> +        atomic_set(&refcnt->refcnt, REFCNT_SATURATED);
> +        printk(XENLOG_ERR"refcnt already hit 0: val = %d\n", ret);

Same here regarding the spamming.

> +        WARN();
> +    }
> +}
> +

Extra newline.

I will look at further patches to see how this gets used.

Thanks, Roger.