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

[Jan Beulich <jbeulich@xxxxxxxx>]

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