Message ID | 20221101152842.4257-12-jgross@suse.com (mailing list archive) |
---|---|
State | Superseded |
Headers | show |
Series | tools/xenstore: do some cleanup and fixes | expand |
Hi Juergen, On 01/11/2022 15:28, Juergen Gross wrote: > static bool check_indexes(XENSTORE_RING_IDX cons, XENSTORE_RING_IDX prod) > @@ -492,8 +504,12 @@ static struct domain *find_or_alloc_existing_domain(unsigned int domid) > xc_dominfo_t dominfo; > > domain = find_domain_struct(domid); > - if (!domain && get_domain_info(domid, &dominfo)) > - domain = alloc_domain(NULL, domid); > + if (!domain) { > + if (!get_domain_info(domid, &dominfo)) > + errno = ENOENT; > + else > + domain = alloc_domain(NULL, domid); > + } I don't understand how this change is related to this commit. [...] > +int acc_add_dom_nbentry(const void *ctx, struct list_head *head, int val, > + unsigned int domid) > +{ > + struct changed_domain *cd; > + > + cd = acc_get_changed_domain(ctx, head, domid); > + if (!cd) > + return errno; > + > + cd->nbentry += val; As a future improvement, it would be worth considering to check for underflow/overflow. Cheers,
On 01.12.22 22:58, Julien Grall wrote: > Hi Juergen, > > On 01/11/2022 15:28, Juergen Gross wrote: >> static bool check_indexes(XENSTORE_RING_IDX cons, XENSTORE_RING_IDX prod) >> @@ -492,8 +504,12 @@ static struct domain >> *find_or_alloc_existing_domain(unsigned int domid) >> xc_dominfo_t dominfo; >> domain = find_domain_struct(domid); >> - if (!domain && get_domain_info(domid, &dominfo)) >> - domain = alloc_domain(NULL, domid); >> + if (!domain) { >> + if (!get_domain_info(domid, &dominfo)) >> + errno = ENOENT; >> + else >> + domain = alloc_domain(NULL, domid); >> + } > > I don't understand how this change is related to this commit. It is directly related to the hunk below. Returning errno in acc_add_dom_nbentry() requires it to be set correctly in find_or_alloc_existing_domain(). I'll add a remark in the commit message. > > [...] > >> +int acc_add_dom_nbentry(const void *ctx, struct list_head *head, int val, >> + unsigned int domid) >> +{ >> + struct changed_domain *cd; >> + >> + cd = acc_get_changed_domain(ctx, head, domid); >> + if (!cd) >> + return errno; >> + >> + cd->nbentry += val; > > As a future improvement, it would be worth considering to check for > underflow/overflow. Do you really think we need to make sure not to add/remove more than 2 billion nodes owned by a single domain? Juergen
On 13.12.22 07:53, Juergen Gross wrote: > On 01.12.22 22:58, Julien Grall wrote: >> Hi Juergen, >> >> On 01/11/2022 15:28, Juergen Gross wrote: >>> static bool check_indexes(XENSTORE_RING_IDX cons, XENSTORE_RING_IDX prod) >>> @@ -492,8 +504,12 @@ static struct domain >>> *find_or_alloc_existing_domain(unsigned int domid) >>> xc_dominfo_t dominfo; >>> domain = find_domain_struct(domid); >>> - if (!domain && get_domain_info(domid, &dominfo)) >>> - domain = alloc_domain(NULL, domid); >>> + if (!domain) { >>> + if (!get_domain_info(domid, &dominfo)) >>> + errno = ENOENT; >>> + else >>> + domain = alloc_domain(NULL, domid); >>> + } >> >> I don't understand how this change is related to this commit. > > It is directly related to the hunk below. Returning errno in > acc_add_dom_nbentry() requires it to be set correctly in > find_or_alloc_existing_domain(). Wait, that was nonsense. This change is probably a leftover of a previous version. Will remove it. Juergen
Hi Juergen, On 13/12/2022 06:53, Juergen Gross wrote: > On 01.12.22 22:58, Julien Grall wrote: >> Hi Juergen, >> >> On 01/11/2022 15:28, Juergen Gross wrote: >>> static bool check_indexes(XENSTORE_RING_IDX cons, XENSTORE_RING_IDX >>> prod) >>> @@ -492,8 +504,12 @@ static struct domain >>> *find_or_alloc_existing_domain(unsigned int domid) >>> xc_dominfo_t dominfo; >>> domain = find_domain_struct(domid); >>> - if (!domain && get_domain_info(domid, &dominfo)) >>> - domain = alloc_domain(NULL, domid); >>> + if (!domain) { >>> + if (!get_domain_info(domid, &dominfo)) >>> + errno = ENOENT; >>> + else >>> + domain = alloc_domain(NULL, domid); >>> + } >> >> I don't understand how this change is related to this commit. > > It is directly related to the hunk below. Returning errno in > acc_add_dom_nbentry() requires it to be set correctly in > find_or_alloc_existing_domain(). > > I'll add a remark in the commit message. > >> >> [...] >> >>> +int acc_add_dom_nbentry(const void *ctx, struct list_head *head, int >>> val, >>> + unsigned int domid) >>> +{ >>> + struct changed_domain *cd; >>> + >>> + cd = acc_get_changed_domain(ctx, head, domid); >>> + if (!cd) >>> + return errno; >>> + >>> + cd->nbentry += val; >> >> As a future improvement, it would be worth considering to check for >> underflow/overflow. > > Do you really think we need to make sure not to add/remove more than > 2 billion nodes owned by a single domain? No and that's not my point. If you look at domain_entry_fix() we have an assert() to check if the sum is still over 0. This assert() was actually triggered a few times while testing the previous XSAs batch. So I think it would be worth to carry a similar check (maybe not an assert()) just in case we mess up with accounting in the future. Cheers,
On 13.12.22 10:35, Julien Grall wrote: > Hi Juergen, > > On 13/12/2022 06:53, Juergen Gross wrote: >> On 01.12.22 22:58, Julien Grall wrote: >>> Hi Juergen, >>> >>> On 01/11/2022 15:28, Juergen Gross wrote: >>>> static bool check_indexes(XENSTORE_RING_IDX cons, XENSTORE_RING_IDX prod) >>>> @@ -492,8 +504,12 @@ static struct domain >>>> *find_or_alloc_existing_domain(unsigned int domid) >>>> xc_dominfo_t dominfo; >>>> domain = find_domain_struct(domid); >>>> - if (!domain && get_domain_info(domid, &dominfo)) >>>> - domain = alloc_domain(NULL, domid); >>>> + if (!domain) { >>>> + if (!get_domain_info(domid, &dominfo)) >>>> + errno = ENOENT; >>>> + else >>>> + domain = alloc_domain(NULL, domid); >>>> + } >>> >>> I don't understand how this change is related to this commit. >> >> It is directly related to the hunk below. Returning errno in >> acc_add_dom_nbentry() requires it to be set correctly in >> find_or_alloc_existing_domain(). >> >> I'll add a remark in the commit message. >> >>> >>> [...] >>> >>>> +int acc_add_dom_nbentry(const void *ctx, struct list_head *head, int val, >>>> + unsigned int domid) >>>> +{ >>>> + struct changed_domain *cd; >>>> + >>>> + cd = acc_get_changed_domain(ctx, head, domid); >>>> + if (!cd) >>>> + return errno; >>>> + >>>> + cd->nbentry += val; >>> >>> As a future improvement, it would be worth considering to check for >>> underflow/overflow. >> >> Do you really think we need to make sure not to add/remove more than >> 2 billion nodes owned by a single domain? > No and that's not my point. If you look at domain_entry_fix() we have an > assert() to check if the sum is still over 0. > > This assert() was actually triggered a few times while testing the previous XSAs > batch. So I think it would be worth to carry a similar check (maybe not an > assert()) just in case we mess up with accounting in the future. Patch 2 of the 2nd series does that already. Juergen
On 13/12/2022 09:54, Juergen Gross wrote: > On 13.12.22 10:35, Julien Grall wrote: >> Hi Juergen, >> >> On 13/12/2022 06:53, Juergen Gross wrote: >>> On 01.12.22 22:58, Julien Grall wrote: >>>> Hi Juergen, >>>> >>>> On 01/11/2022 15:28, Juergen Gross wrote: >>>>> static bool check_indexes(XENSTORE_RING_IDX cons, >>>>> XENSTORE_RING_IDX prod) >>>>> @@ -492,8 +504,12 @@ static struct domain >>>>> *find_or_alloc_existing_domain(unsigned int domid) >>>>> xc_dominfo_t dominfo; >>>>> domain = find_domain_struct(domid); >>>>> - if (!domain && get_domain_info(domid, &dominfo)) >>>>> - domain = alloc_domain(NULL, domid); >>>>> + if (!domain) { >>>>> + if (!get_domain_info(domid, &dominfo)) >>>>> + errno = ENOENT; >>>>> + else >>>>> + domain = alloc_domain(NULL, domid); >>>>> + } >>>> >>>> I don't understand how this change is related to this commit. >>> >>> It is directly related to the hunk below. Returning errno in >>> acc_add_dom_nbentry() requires it to be set correctly in >>> find_or_alloc_existing_domain(). >>> >>> I'll add a remark in the commit message. >>> >>>> >>>> [...] >>>> >>>>> +int acc_add_dom_nbentry(const void *ctx, struct list_head *head, >>>>> int val, >>>>> + unsigned int domid) >>>>> +{ >>>>> + struct changed_domain *cd; >>>>> + >>>>> + cd = acc_get_changed_domain(ctx, head, domid); >>>>> + if (!cd) >>>>> + return errno; >>>>> + >>>>> + cd->nbentry += val; >>>> >>>> As a future improvement, it would be worth considering to check for >>>> underflow/overflow. >>> >>> Do you really think we need to make sure not to add/remove more than >>> 2 billion nodes owned by a single domain? >> No and that's not my point. If you look at domain_entry_fix() we have >> an assert() to check if the sum is still over 0. >> >> This assert() was actually triggered a few times while testing the >> previous XSAs batch. So I think it would be worth to carry a similar >> check (maybe not an assert()) just in case we mess up with accounting >> in the future. > > Patch 2 of the 2nd series does that already. Ok. I will have a look. Cheers,
diff --git a/tools/xenstore/xenstored_domain.c b/tools/xenstore/xenstored_domain.c index 5756010944..14fd84c288 100644 --- a/tools/xenstore/xenstored_domain.c +++ b/tools/xenstore/xenstored_domain.c @@ -91,6 +91,18 @@ struct domain bool wrl_delay_logged; }; +struct changed_domain +{ + /* List of all changed domains. */ + struct list_head list; + + /* Identifier of the changed domain. */ + unsigned int domid; + + /* Amount by which this domain's nbentry field has changed. */ + int nbentry; +}; + static struct hashtable *domhash; static bool check_indexes(XENSTORE_RING_IDX cons, XENSTORE_RING_IDX prod) @@ -492,8 +504,12 @@ static struct domain *find_or_alloc_existing_domain(unsigned int domid) xc_dominfo_t dominfo; domain = find_domain_struct(domid); - if (!domain && get_domain_info(domid, &dominfo)) - domain = alloc_domain(NULL, domid); + if (!domain) { + if (!get_domain_info(domid, &dominfo)) + errno = ENOENT; + else + domain = alloc_domain(NULL, domid); + } return domain; } @@ -547,6 +563,71 @@ static struct domain *find_domain_by_domid(unsigned int domid) return (d && d->introduced) ? d : NULL; } +int acc_fix_domains(struct list_head *head, bool update) +{ + struct changed_domain *cd; + int cnt; + + list_for_each_entry(cd, head, list) { + cnt = domain_entry_fix(cd->domid, cd->nbentry, update); + if (!update) { + if (cnt >= quota_nb_entry_per_domain) + return ENOSPC; + if (cnt < 0) + return ENOMEM; + } + } + + return 0; +} + +static struct changed_domain *acc_find_changed_domain(struct list_head *head, + unsigned int domid) +{ + struct changed_domain *cd; + + list_for_each_entry(cd, head, list) { + if (cd->domid == domid) + return cd; + } + + return NULL; +} + +static struct changed_domain *acc_get_changed_domain(const void *ctx, + struct list_head *head, + unsigned int domid) +{ + struct changed_domain *cd; + + cd = acc_find_changed_domain(head, domid); + if (cd) + return cd; + + cd = talloc_zero(ctx, struct changed_domain); + if (!cd) + return NULL; + + cd->domid = domid; + list_add_tail(&cd->list, head); + + return cd; +} + +int acc_add_dom_nbentry(const void *ctx, struct list_head *head, int val, + unsigned int domid) +{ + struct changed_domain *cd; + + cd = acc_get_changed_domain(ctx, head, domid); + if (!cd) + return errno; + + cd->nbentry += val; + + return 0; +} + static void domain_conn_reset(struct domain *domain) { struct connection *conn = domain->conn; diff --git a/tools/xenstore/xenstored_domain.h b/tools/xenstore/xenstored_domain.h index 630641d620..9e20d2b17d 100644 --- a/tools/xenstore/xenstored_domain.h +++ b/tools/xenstore/xenstored_domain.h @@ -98,6 +98,9 @@ void domain_outstanding_dec(struct connection *conn); void domain_outstanding_domid_dec(unsigned int domid); int domain_get_quota(const void *ctx, struct connection *conn, unsigned int domid); +int acc_fix_domains(struct list_head *head, bool update); +int acc_add_dom_nbentry(const void *ctx, struct list_head *head, int val, + unsigned int domid); /* Write rate limiting */ diff --git a/tools/xenstore/xenstored_transaction.c b/tools/xenstore/xenstored_transaction.c index ac854197ca..89b92f0baf 100644 --- a/tools/xenstore/xenstored_transaction.c +++ b/tools/xenstore/xenstored_transaction.c @@ -137,18 +137,6 @@ struct accessed_node bool watch_exact; }; -struct changed_domain -{ - /* List of all changed domains in the context of this transaction. */ - struct list_head list; - - /* Identifier of the changed domain. */ - unsigned int domid; - - /* Amount by which this domain's nbentry field has changed. */ - int nbentry; -}; - struct transaction { /* List of all transactions active on this connection. */ @@ -514,24 +502,6 @@ int do_transaction_start(const void *ctx, struct connection *conn, return 0; } -static int transaction_fix_domains(struct transaction *trans, bool update) -{ - struct changed_domain *d; - int cnt; - - list_for_each_entry(d, &trans->changed_domains, list) { - cnt = domain_entry_fix(d->domid, d->nbentry, update); - if (!update) { - if (cnt >= quota_nb_entry_per_domain) - return ENOSPC; - if (cnt < 0) - return ENOMEM; - } - } - - return 0; -} - int do_transaction_end(const void *ctx, struct connection *conn, struct buffered_data *in) { @@ -558,7 +528,7 @@ int do_transaction_end(const void *ctx, struct connection *conn, if (streq(arg, "T")) { if (trans->fail) return ENOMEM; - ret = transaction_fix_domains(trans, false); + ret = acc_fix_domains(&trans->changed_domains, false); if (ret) return ret; ret = finalize_transaction(conn, trans, &is_corrupt); @@ -568,7 +538,7 @@ int do_transaction_end(const void *ctx, struct connection *conn, wrl_apply_debit_trans_commit(conn); /* fix domain entry for each changed domain */ - transaction_fix_domains(trans, true); + acc_fix_domains(&trans->changed_domains, true); if (is_corrupt) corrupt(conn, "transaction inconsistency"); @@ -580,44 +550,18 @@ int do_transaction_end(const void *ctx, struct connection *conn, void transaction_entry_inc(struct transaction *trans, unsigned int domid) { - struct changed_domain *d; - - list_for_each_entry(d, &trans->changed_domains, list) - if (d->domid == domid) { - d->nbentry++; - return; - } - - d = talloc(trans, struct changed_domain); - if (!d) { + if (acc_add_dom_nbentry(trans, &trans->changed_domains, 1, domid)) { /* Let the transaction fail. */ trans->fail = true; - return; } - d->domid = domid; - d->nbentry = 1; - list_add_tail(&d->list, &trans->changed_domains); } void transaction_entry_dec(struct transaction *trans, unsigned int domid) { - struct changed_domain *d; - - list_for_each_entry(d, &trans->changed_domains, list) - if (d->domid == domid) { - d->nbentry--; - return; - } - - d = talloc(trans, struct changed_domain); - if (!d) { + if (acc_add_dom_nbentry(trans, &trans->changed_domains, -1, domid)) { /* Let the transaction fail. */ trans->fail = true; - return; } - d->domid = domid; - d->nbentry = -1; - list_add_tail(&d->list, &trans->changed_domains); } void fail_transaction(struct transaction *trans)
Move all code related to struct changed_domain from xenstored_transaction.c to xenstored_domain.c. This will be needed later in order to simplify the accounting data updates in cases of errors during a request. Split the code to have a more generic base framework. Signed-off-by: Juergen Gross <jgross@suse.com> --- tools/xenstore/xenstored_domain.c | 85 +++++++++++++++++++++++++- tools/xenstore/xenstored_domain.h | 3 + tools/xenstore/xenstored_transaction.c | 64 ++----------------- 3 files changed, 90 insertions(+), 62 deletions(-)