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.
> 
> 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 @@
> +#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)
> +{
> +    return atomic_read(&refcnt->refcnt);
> +}
> +
> +static inline void refcnt_get(refcnt_t *refcnt)
> +{
> +    int old = atomic_add_unless(&refcnt->refcnt, 1, 0);

Occurred to me while looking at the next patch:

Don't you also need to print a warning (and saturate the counter
maybe?) if old == 0, as that would imply the caller is attempting
to take a reference of an object that should be destroyed? IOW: it
would point to some kind of memory leak.

Thanks, Roger.