| Message ID | 20230314205612.3703668-2-volodymyr_babchuk@epam.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | vpci: first series in preparation for vpci on ARM | expand |
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@epam.com> > > --- > 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.
On 16.03.2023 14:54, Roger Pau Monné wrote: > On Tue, Mar 14, 2023 at 08:56:29PM +0000, Volodymyr Babchuk wrote: >> --- /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? Not "at least", but strictly that way for any new code. Patches to convert various existing code to SPDX are already pending iirc. >> +#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. Are you sure? My experience was generally the other way around: likely() and unlikely() become ineffectual as soon as the compiler needs more than one branch for the inner construct (ie generally for and && or ||). Jan
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@epam.com> > > --- > 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.
On Thu, Mar 16, 2023 at 03:03:42PM +0100, Jan Beulich wrote: > On 16.03.2023 14:54, Roger Pau Monné wrote: > > On Tue, Mar 14, 2023 at 08:56:29PM +0000, Volodymyr Babchuk wrote: > >> +{ > >> + 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. > > Are you sure? My experience was generally the other way around: likely() > and unlikely() become ineffectual as soon as the compiler needs more > than one branch for the inner construct (ie generally for and && or ||). Oh, OK, never mind then. We have examples of both in the code base. Roger.
On 16.03.2023 17:19, Roger Pau Monné wrote: > On Tue, Mar 14, 2023 at 08:56:29PM +0000, Volodymyr Babchuk wrote: >> +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. Hmm, I notice the function presently returns void. I think what to do when the counter is zero needs leaving to the caller. See e.g. get_page() which will simply indicate failure to the caller in case the refcnt is zero. (There overflow handling also is left to the caller ... All that matters is whether a ref can be acquired.) Jan
On Thu, Mar 16, 2023 at 05:32:38PM +0100, Jan Beulich wrote: > On 16.03.2023 17:19, Roger Pau Monné wrote: > > On Tue, Mar 14, 2023 at 08:56:29PM +0000, Volodymyr Babchuk wrote: > >> +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. > > Hmm, I notice the function presently returns void. I think what to do > when the counter is zero needs leaving to the caller. See e.g. > get_page() which will simply indicate failure to the caller in case > the refcnt is zero. (There overflow handling also is left to the > caller ... All that matters is whether a ref can be acquired.) Hm, likely. I guess pages never go away even when it's refcount reaches 0. For the pdev case attempting to take a refcount on an object that has 0 refcounts implies that the caller is using leaked memory, as the point an object reaches 0 it supposed to be destroyed. Returning success would be fine, as then for the pdev use-case we could print a warning and likely take some action to prevent further damage if possible. Thanks, Roger.
On 16.03.2023 17:39, Roger Pau Monné wrote: > On Thu, Mar 16, 2023 at 05:32:38PM +0100, Jan Beulich wrote: >> On 16.03.2023 17:19, Roger Pau Monné wrote: >>> On Tue, Mar 14, 2023 at 08:56:29PM +0000, Volodymyr Babchuk wrote: >>>> +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. >> >> Hmm, I notice the function presently returns void. I think what to do >> when the counter is zero needs leaving to the caller. See e.g. >> get_page() which will simply indicate failure to the caller in case >> the refcnt is zero. (There overflow handling also is left to the >> caller ... All that matters is whether a ref can be acquired.) > > Hm, likely. I guess pages never go away even when it's refcount > reaches 0. > > For the pdev case attempting to take a refcount on an object that has > 0 refcounts implies that the caller is using leaked memory, as the > point an object reaches 0 it supposed to be destroyed. Hmm, my thinking was that a device would remain at refcnt 0 until it is actually removed, i.e. refcnt == 0 being a prereq for pci_remove_device() to be willing to do anything at all. But maybe that's not a viable model. Jan
On Thu, Mar 16, 2023 at 05:43:18PM +0100, Jan Beulich wrote: > On 16.03.2023 17:39, Roger Pau Monné wrote: > > On Thu, Mar 16, 2023 at 05:32:38PM +0100, Jan Beulich wrote: > >> On 16.03.2023 17:19, Roger Pau Monné wrote: > >>> On Tue, Mar 14, 2023 at 08:56:29PM +0000, Volodymyr Babchuk wrote: > >>>> +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. > >> > >> Hmm, I notice the function presently returns void. I think what to do > >> when the counter is zero needs leaving to the caller. See e.g. > >> get_page() which will simply indicate failure to the caller in case > >> the refcnt is zero. (There overflow handling also is left to the > >> caller ... All that matters is whether a ref can be acquired.) > > > > Hm, likely. I guess pages never go away even when it's refcount > > reaches 0. > > > > For the pdev case attempting to take a refcount on an object that has > > 0 refcounts implies that the caller is using leaked memory, as the > > point an object reaches 0 it supposed to be destroyed. > > Hmm, my thinking was that a device would remain at refcnt 0 until it is > actually removed, i.e. refcnt == 0 being a prereq for pci_remove_device() > to be willing to do anything at all. But maybe that's not a viable model. Right, I think the intention was for pci_remove_device() to drop the refcount to 0 and do the removal, so the refcount should be 1 when calling pci_remove_device(). But none of this is written down, so it's mostly my assumptions from looking at the code. I have some comments about the model in patch 2, I think we need to clarify the intended usage on the commit log about pdev and refcounts. Thanks, Roger.
On 16.03.2023 17:48, Roger Pau Monné wrote: > On Thu, Mar 16, 2023 at 05:43:18PM +0100, Jan Beulich wrote: >> On 16.03.2023 17:39, Roger Pau Monné wrote: >>> On Thu, Mar 16, 2023 at 05:32:38PM +0100, Jan Beulich wrote: >>>> On 16.03.2023 17:19, Roger Pau Monné wrote: >>>>> On Tue, Mar 14, 2023 at 08:56:29PM +0000, Volodymyr Babchuk wrote: >>>>>> +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. >>>> >>>> Hmm, I notice the function presently returns void. I think what to do >>>> when the counter is zero needs leaving to the caller. See e.g. >>>> get_page() which will simply indicate failure to the caller in case >>>> the refcnt is zero. (There overflow handling also is left to the >>>> caller ... All that matters is whether a ref can be acquired.) >>> >>> Hm, likely. I guess pages never go away even when it's refcount >>> reaches 0. >>> >>> For the pdev case attempting to take a refcount on an object that has >>> 0 refcounts implies that the caller is using leaked memory, as the >>> point an object reaches 0 it supposed to be destroyed. >> >> Hmm, my thinking was that a device would remain at refcnt 0 until it is >> actually removed, i.e. refcnt == 0 being a prereq for pci_remove_device() >> to be willing to do anything at all. But maybe that's not a viable model. > > Right, I think the intention was for pci_remove_device() to drop the > refcount to 0 and do the removal, so the refcount should be 1 when > calling pci_remove_device(). But none of this is written down, so > it's mostly my assumptions from looking at the code. Could such work at all? The function can't safely drop a reference and _then_ check whether it was the last one. The function either has to take refcnt == 0 as prereq, or it needs to be the destructor function that refcnt_put() calls. Jan
On 14.03.2023 21:56, Volodymyr Babchuk wrote: > +static inline void refcnt_put(refcnt_t *refcnt, void (*destructor)(refcnt_t *refcnt)) Hmm, this means all callers need to pass (and agree on) the supposedly single destructor function that needs calling. Wouldn't the destructor function better be stored elsewhere (and supplied to e.g. refcnt_init())? > +{ > + int ret = atomic_dec_return(&refcnt->refcnt); > + > + if ( ret == 0 ) > + destructor(refcnt); > + > + if ( unlikely(ret < 0)) > + { > + atomic_set(&refcnt->refcnt, REFCNT_SATURATED); It's undefined whether *refcnt still exists once the destructor was called (which would have happened before we can make it here). While even the atomic_dec_return() above would already have acted in an unknown way in this case I don't think it's a good idea to access the object yet another time. (Same for the "negative" case in refcnt_get() then.) Jan
On Thu, Mar 16, 2023 at 05:56:00PM +0100, Jan Beulich wrote: > On 16.03.2023 17:48, Roger Pau Monné wrote: > > On Thu, Mar 16, 2023 at 05:43:18PM +0100, Jan Beulich wrote: > >> On 16.03.2023 17:39, Roger Pau Monné wrote: > >>> On Thu, Mar 16, 2023 at 05:32:38PM +0100, Jan Beulich wrote: > >>>> On 16.03.2023 17:19, Roger Pau Monné wrote: > >>>>> On Tue, Mar 14, 2023 at 08:56:29PM +0000, Volodymyr Babchuk wrote: > >>>>>> +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. > >>>> > >>>> Hmm, I notice the function presently returns void. I think what to do > >>>> when the counter is zero needs leaving to the caller. See e.g. > >>>> get_page() which will simply indicate failure to the caller in case > >>>> the refcnt is zero. (There overflow handling also is left to the > >>>> caller ... All that matters is whether a ref can be acquired.) > >>> > >>> Hm, likely. I guess pages never go away even when it's refcount > >>> reaches 0. > >>> > >>> For the pdev case attempting to take a refcount on an object that has > >>> 0 refcounts implies that the caller is using leaked memory, as the > >>> point an object reaches 0 it supposed to be destroyed. > >> > >> Hmm, my thinking was that a device would remain at refcnt 0 until it is > >> actually removed, i.e. refcnt == 0 being a prereq for pci_remove_device() > >> to be willing to do anything at all. But maybe that's not a viable model. > > > > Right, I think the intention was for pci_remove_device() to drop the > > refcount to 0 and do the removal, so the refcount should be 1 when > > calling pci_remove_device(). But none of this is written down, so > > it's mostly my assumptions from looking at the code. > > Could such work at all? The function can't safely drop a reference > and _then_ check whether it was the last one. The function either has > to take refcnt == 0 as prereq, or it needs to be the destructor > function that refcnt_put() calls. But then you also get in the trouble of asserting that refcnt == 0 doesn't change between evaluation and actual removal of the structure. Should all refcounts to pdev be taken and dropped while holding the pcidevs lock? I there an email (outside of this series) that contains a description of how the refcounting is to be used with pdevs? Thanks, Roger.
On 17.03.2023 11:05, Roger Pau Monné wrote: > On Thu, Mar 16, 2023 at 05:56:00PM +0100, Jan Beulich wrote: >> On 16.03.2023 17:48, Roger Pau Monné wrote: >>> On Thu, Mar 16, 2023 at 05:43:18PM +0100, Jan Beulich wrote: >>>> On 16.03.2023 17:39, Roger Pau Monné wrote: >>>>> On Thu, Mar 16, 2023 at 05:32:38PM +0100, Jan Beulich wrote: >>>>>> On 16.03.2023 17:19, Roger Pau Monné wrote: >>>>>>> On Tue, Mar 14, 2023 at 08:56:29PM +0000, Volodymyr Babchuk wrote: >>>>>>>> +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. >>>>>> >>>>>> Hmm, I notice the function presently returns void. I think what to do >>>>>> when the counter is zero needs leaving to the caller. See e.g. >>>>>> get_page() which will simply indicate failure to the caller in case >>>>>> the refcnt is zero. (There overflow handling also is left to the >>>>>> caller ... All that matters is whether a ref can be acquired.) >>>>> >>>>> Hm, likely. I guess pages never go away even when it's refcount >>>>> reaches 0. >>>>> >>>>> For the pdev case attempting to take a refcount on an object that has >>>>> 0 refcounts implies that the caller is using leaked memory, as the >>>>> point an object reaches 0 it supposed to be destroyed. >>>> >>>> Hmm, my thinking was that a device would remain at refcnt 0 until it is >>>> actually removed, i.e. refcnt == 0 being a prereq for pci_remove_device() >>>> to be willing to do anything at all. But maybe that's not a viable model. >>> >>> Right, I think the intention was for pci_remove_device() to drop the >>> refcount to 0 and do the removal, so the refcount should be 1 when >>> calling pci_remove_device(). But none of this is written down, so >>> it's mostly my assumptions from looking at the code. >> >> Could such work at all? The function can't safely drop a reference >> and _then_ check whether it was the last one. The function either has >> to take refcnt == 0 as prereq, or it needs to be the destructor >> function that refcnt_put() calls. > > But then you also get in the trouble of asserting that refcnt == 0 > doesn't change between evaluation and actual removal of the structure. > > Should all refcounts to pdev be taken and dropped while holding the > pcidevs lock? > > I there an email (outside of this series) that contains a description > of how the refcounting is to be used with pdevs? I'm not aware of one. The intentions indeed need outlining somewhere. Jan
Hello Roger, Sorry for the late answer. Roger Pau Monné <roger.pau@citrix.com> writes: > 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? Well, I considered this. But it is more complex. It has separate refcount module, which just counts references + kref code, that is capable of calling destructors. I am not sure if Xen need this division. In any case, I tried to replicate Linux behavior as close as possible. On other hand, Jan suggests to rework API, so it will be differ from Linux one...
Jan Beulich <jbeulich@suse.com> writes: > On 14.03.2023 21:56, Volodymyr Babchuk wrote: >> +static inline void refcnt_put(refcnt_t *refcnt, void (*destructor)(refcnt_t *refcnt)) > > Hmm, this means all callers need to pass (and agree on) the supposedly > single destructor function that needs calling. Wouldn't the destructor > function better be stored elsewhere (and supplied to e.g. refcnt_init())? > I tried to replicate Linux approach. They provide destructor function every time. On other hand, kref_put() is often called from a wrapper function (like pdev_put() in our case), so destructor in fact, is provided only once. >> +{ >> + int ret = atomic_dec_return(&refcnt->refcnt); >> + >> + if ( ret == 0 ) >> + destructor(refcnt); >> + >> + if ( unlikely(ret < 0)) >> + { >> + atomic_set(&refcnt->refcnt, REFCNT_SATURATED); > > It's undefined whether *refcnt still exists once the destructor was > called (which would have happened before we can make it here). While > even the atomic_dec_return() above would already have acted in an > unknown way in this case I don't think it's a good idea to access the > object yet another time. (Same for the "negative" case in > refcnt_get() then.) Okay, then I'll remove saturation logic.
On Tue, Apr 11, 2023 at 10:27:45PM +0000, Volodymyr Babchuk wrote: > > Hello Roger, > > Sorry for the late answer. > > Roger Pau Monné <roger.pau@citrix.com> writes: > > > 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? > > Well, I considered this. But it is more complex. It has separate > refcount module, which just counts references + kref code, that is > capable of calling destructors. I am not sure if Xen need this > division. In any case, I tried to replicate Linux behavior as close as > possible. On other hand, Jan suggests to rework API, so it will be > differ from Linux one... OK, just asking because it's likely for the interface to grow if there are more users of refcounting, and at some point we might need a set of features similar to Linux. Thanks, Roger.
On 12.04.2023 00:38, Volodymyr Babchuk wrote: > Jan Beulich <jbeulich@suse.com> writes: >> On 14.03.2023 21:56, Volodymyr Babchuk wrote: >>> +static inline void refcnt_put(refcnt_t *refcnt, void (*destructor)(refcnt_t *refcnt)) >> >> Hmm, this means all callers need to pass (and agree on) the supposedly >> single destructor function that needs calling. Wouldn't the destructor >> function better be stored elsewhere (and supplied to e.g. refcnt_init())? >> > > I tried to replicate Linux approach. They provide destructor function > every time. On other hand, kref_put() is often called from a wrapper > function (like pdev_put() in our case), so destructor in fact, is > provided only once. If provided via wrappers, that'll be fine of course. >>> +{ >>> + int ret = atomic_dec_return(&refcnt->refcnt); >>> + >>> + if ( ret == 0 ) >>> + destructor(refcnt); >>> + >>> + if ( unlikely(ret < 0)) >>> + { >>> + atomic_set(&refcnt->refcnt, REFCNT_SATURATED); >> >> It's undefined whether *refcnt still exists once the destructor was >> called (which would have happened before we can make it here). While >> even the atomic_dec_return() above would already have acted in an >> unknown way in this case I don't think it's a good idea to access the >> object yet another time. (Same for the "negative" case in >> refcnt_get() then.) > > Okay, then I'll remove saturation logic. Wait. Saturating on overflow might still be a reasonable concept. But here you convert an underflow to the "saturated" value. Jan
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); + + if ( unlikely(old < 0) || unlikely (old + 1 < 0) ) + { + atomic_set(&refcnt->refcnt, REFCNT_SATURATED); + printk(XENLOG_ERR"refcnt saturation: old = %d\n", old); + 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)) + { + atomic_set(&refcnt->refcnt, REFCNT_SATURATED); + printk(XENLOG_ERR"refcnt already hit 0: val = %d\n", ret); + WARN(); + } +} + +#endif + +/* + * Local variables: + * mode: C + * c-file-style: "BSD" + * c-basic-offset: 4 + * indent-tabs-mode: nil + * End: + */
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@epam.com> --- 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