Re: [PATCH v2 04/13] tools/xenstore: add framework to commit accounting data on success only

[Juergen Gross <jgross@suse.com>]

[-- Attachment #1.1.1: Type: text/plain, Size: 4892 bytes --]

On 21.02.23 23:42, Julien Grall wrote:
> Hi Juergen,
> 
> On 21/02/2023 08:37, Juergen Gross wrote:
>> On 20.02.23 23:50, Julien Grall wrote:
>>>> +        list_del(&cd->list);
>>>> +        talloc_free(cd);
>>>> +    }
>>>> +}
>>>> +
>>>> +void acc_commit(struct connection *conn)
>>>> +{
>>>> +    struct changed_domain *cd;
>>>> +    struct buffered_data *in = conn->in;
>>>> +    enum accitem what;
>>>> +
>>>> +    conn->in = NULL; /* Avoid recursion. */
>>>
>>> I am not sure I understand this comment. Can you provide more details where 
>>> the recursion would happen?
>>
>> domain_acc_add() might do temporary accounting if conn->in isn't NULL.
> 
> I am confused. To me recursion means the function (or the caller) will call 
> itself. But here you seem to say you just want to avoid temporary accounting.

It is basically data recursion. Trying to apply temporary accounting data
leading to that temporary accounting data being modified again might result
in endless loops.

> What did I miss?
> 
>>
>>>
>>>> +    while ((cd = list_top(&conn->acc_list, struct changed_domain, list))) {
>>>
>>> NIT: You could use list_for_each_safe();
>>
>> Like above.
>>
>>>
>>>> +        list_del(&cd->list);
>>>> +        for (what = 0; what < ACC_REQ_N; what++)
>>>
>>> There is a chance that some compiler will complain about this line because 
>>> ACC_REQ_N = 0. So this would always be true. Therefore...
>>
>> Huh? It would always be false.
> 
> Yes false sorry. This doesn't change about the potential (temporary) warning.

Shouldn't that rather result in dead code elimination instead?

> 
>>
>>>
>>>> +            if (cd->acc[what])
>>>> +                domain_acc_add(conn, cd->domid, what,
>>>> +                           cd->acc[what], true);
>>>> +
>>>> +        talloc_free(cd);
>>>> +    }
>>>> +
>>>> +    conn->in = in;
>>>> +}
>>>> +
>>>>   int domain_nbentry_inc(struct connection *conn, unsigned int domid)
>>>>   {
>>>>       return (domain_acc_add(conn, domid, ACC_NODES, 1, false) < 0)
>>>> diff --git a/tools/xenstore/xenstored_domain.h 
>>>> b/tools/xenstore/xenstored_domain.h
>>>> index 8259c114b0..cd85bd0cad 100644
>>>> --- a/tools/xenstore/xenstored_domain.h
>>>> +++ b/tools/xenstore/xenstored_domain.h
>>>> @@ -20,7 +20,8 @@
>>>>   #define _XENSTORED_DOMAIN_H
>>>>   enum accitem {
>>>> -    ACC_NODES,
>>>> +    ACC_REQ_N,       /* Number of elements per request and domain. */
>>>> +    ACC_NODES = ACC_REQ_N,
>>>
>>> I would bring forward the change in patch #5. I mean:
>>>
>>> ACC_NODES,
>>> ACC_REQ_N
>>
>> I'm not sure this is a good idea. This would activate the temporary
>> accounting for nodes, but keeping the error handling active. I'd rather
>> activate temporary accounting for nodes together with removing the
>> accounting correction in the error handling.
> 
> I am not sure I fully understand what you would rather do. Can you clarify?

Having ACC_REQ_N > 0 will result in temporary accounting being active for
ACC_NODES (which is 0), due to "what < ACC_REQ_N".

At the same time there are still all the places where in error cases the
node accounting is corrected again (the mentioned error handling). So we
would have both error handling mechanisms (explicit and temp accounting)
active at the same time for nodes.

> 
>>
>>>
>>>>       ACC_TR_N,        /* Number of elements per transaction and domain. */
>>>>       ACC_N = ACC_TR_N /* Number of elements per domain. */
>>>>   };
>>>
>>> This enum is getting extremely confusing. There are many "number of elements 
>>> per ... domain". Can you clarify?
>>
>> I will add some more comments to the header. Would you like it better to have
>> the limits indented more? something like:
> 
> I am afraid it doesn't help understanding the comment. For instance,
> 
>>
>> enum accitem {
>>          ACC_NODES,
>>          /* Number of elements per request and domain. */
> 
> you wrote "per request and domain" here. But...
> 
>>          ACC_REQ_N,
>>          /* Number of elements per transaction and domain. */
> 
> ... here this is "per transaction and domain". Should not nbe "elements per 
> transaction"? And if not, then why don't we had "per request, transaction and 
> domain" above?

This is due to the nesting: the outermost nesting level is "all accounting
items", which covers everything, and is tracked on a per domain basis.

The next level is "per transaction" (I can drop the "and per domain", as
everything is per domain).

The innermost level is "per request". A request can be either part of a
transaction, or not. This doesn't matter.


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]