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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 12.04.2023 00:38, Volodymyr Babchuk wrote:
> Jan Beulich <jbeulich@xxxxxxxx> 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