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 <[email protected]> > > --- > 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.
