| Message ID | 20230120100028.11142-5-jgross@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | tools/xenstore: rework internal accounting | expand |
Hi Juergen, On 20/01/2023 10:00, Juergen Gross wrote: > Instead of modifying accounting data and undo those modifications in > case of an error during further processing, add a framework for > collecting the needed changes and commit them only when the whole > operation has succeeded. > > This scheme can reuse large parts of the per transaction accounting. > The changed_domain handling can be reused, but the array size of the > accounting data should be possible to be different for both use cases. > > Signed-off-by: Juergen Gross <jgross@suse.com> > --- > tools/xenstore/xenstored_core.c | 5 +++ > tools/xenstore/xenstored_core.h | 3 ++ > tools/xenstore/xenstored_domain.c | 64 +++++++++++++++++++++++++++---- > tools/xenstore/xenstored_domain.h | 5 ++- > 4 files changed, 68 insertions(+), 9 deletions(-) > > diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c > index 27dfbe9593..2d10cacf35 100644 > --- a/tools/xenstore/xenstored_core.c > +++ b/tools/xenstore/xenstored_core.c > @@ -1023,6 +1023,9 @@ static void send_error(struct connection *conn, int error) > break; > } > } > + > + acc_drop(conn); > + > send_reply(conn, XS_ERROR, xsd_errors[i].errstring, > strlen(xsd_errors[i].errstring) + 1); > } > @@ -1060,6 +1063,7 @@ void send_reply(struct connection *conn, enum xsd_sockmsg_type type, > } > > conn->in = NULL; > + acc_commit(conn); AFAIU, if send_reply() is called then we would need to commit the accounting even if we can't send the reply (i.e. send_reply()). So shouldn't this be call right at the beginning of send_reply()? > > /* Update relevant header fields and fill in the message body. */ > bdata->hdr.msg.type = type; > @@ -2195,6 +2199,7 @@ struct connection *new_connection(const struct interface_funcs *funcs) > new->is_stalled = false; > new->transaction_started = 0; > INIT_LIST_HEAD(&new->out_list); > + INIT_LIST_HEAD(&new->acc_list); > INIT_LIST_HEAD(&new->ref_list); > INIT_LIST_HEAD(&new->watches); > INIT_LIST_HEAD(&new->transaction_list); > diff --git a/tools/xenstore/xenstored_core.h b/tools/xenstore/xenstored_core.h > index c59b06551f..1f811f38cb 100644 > --- a/tools/xenstore/xenstored_core.h > +++ b/tools/xenstore/xenstored_core.h > @@ -139,6 +139,9 @@ struct connection > struct list_head out_list; > uint64_t timeout_msec; > > + /* Not yet committed accounting data (valid if in != NULL). */ > + struct list_head acc_list; > + > /* Referenced requests no longer pending. */ > struct list_head ref_list; > > diff --git a/tools/xenstore/xenstored_domain.c b/tools/xenstore/xenstored_domain.c > index f459c5aabb..33105dba8f 100644 > --- a/tools/xenstore/xenstored_domain.c > +++ b/tools/xenstore/xenstored_domain.c > @@ -100,7 +100,7 @@ struct changed_domain > unsigned int domid; > > /* Accounting data. */ > - int acc[ACC_TR_N]; > + int acc[]; Is this actually worth it? How much memory would we save? > }; > > static struct hashtable *domhash; > @@ -577,6 +577,7 @@ static struct changed_domain *acc_find_changed_domain(struct list_head *head, > > static struct changed_domain *acc_get_changed_domain(const void *ctx, > struct list_head *head, > + enum accitem acc_sz, > unsigned int domid) > { > struct changed_domain *cd; > @@ -585,7 +586,7 @@ static struct changed_domain *acc_get_changed_domain(const void *ctx, > if (cd) > return cd; > > - cd = talloc_zero(ctx, struct changed_domain); > + cd = talloc_zero_size(ctx, sizeof(*cd) + acc_sz * sizeof(cd->acc[0])); > if (!cd) > return NULL; > > @@ -596,13 +597,13 @@ static struct changed_domain *acc_get_changed_domain(const void *ctx, > } > > static int acc_add_changed_dom(const void *ctx, struct list_head *head, > - enum accitem what, int val, unsigned int domid) > + enum accitem acc_sz, enum accitem what, > + int val, unsigned int domid) > { > struct changed_domain *cd; > > - assert(what < ARRAY_SIZE(cd->acc)); > - > - cd = acc_get_changed_domain(ctx, head, domid); > + assert(what < acc_sz); > + cd = acc_get_changed_domain(ctx, head, acc_sz, domid); > if (!cd) > return 0; > > @@ -1071,6 +1072,7 @@ static int domain_acc_add(struct connection *conn, unsigned int domid, > enum accitem what, int add, bool no_dom_alloc) > { > struct domain *d; > + struct changed_domain *cd; > struct list_head *head; > int ret; > > @@ -1091,10 +1093,26 @@ static int domain_acc_add(struct connection *conn, unsigned int domid, > } > } > > + /* Temporary accounting data until final commit? */ > + if (conn && conn->in && what < ACC_REQ_N) { > + /* Consider transaction local data. */ > + ret = 0; > + if (conn->transaction && what < ACC_TR_N) { > + head = transaction_get_changed_domains( > + conn->transaction); > + cd = acc_find_changed_domain(head, domid); > + if (cd) > + ret = cd->acc[what]; > + } > + ret += acc_add_changed_dom(conn->in, &conn->acc_list, ACC_REQ_N, > + what, add, domid); > + return errno ? -1 : domain_acc_add_chk(d, what, ret, domid); > + } > + > if (conn && conn->transaction && what < ACC_TR_N) { > head = transaction_get_changed_domains(conn->transaction); > - ret = acc_add_changed_dom(conn->transaction, head, what, > - add, domid); > + ret = acc_add_changed_dom(conn->transaction, head, ACC_TR_N, > + what, add, domid); > if (errno) { > fail_transaction(conn->transaction); > return -1; > @@ -1107,6 +1125,36 @@ static int domain_acc_add(struct connection *conn, unsigned int domid, > return d->acc[what]; > } > > +void acc_drop(struct connection *conn) > +{ > + struct changed_domain *cd; > + > + while ((cd = list_top(&conn->acc_list, struct changed_domain, list))) { NIT: You could use list_for_each_safe(); > + 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? > + while ((cd = list_top(&conn->acc_list, struct changed_domain, list))) { NIT: You could use list_for_each_safe(); > + 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... > + 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 > 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? > @@ -103,6 +104,8 @@ 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); > +void acc_drop(struct connection *conn); > +void acc_commit(struct connection *conn); > > /* Write rate limiting */ > Cheers,
On 20.02.23 23:50, Julien Grall wrote: > Hi Juergen, > > On 20/01/2023 10:00, Juergen Gross wrote: >> Instead of modifying accounting data and undo those modifications in >> case of an error during further processing, add a framework for >> collecting the needed changes and commit them only when the whole >> operation has succeeded. >> >> This scheme can reuse large parts of the per transaction accounting. >> The changed_domain handling can be reused, but the array size of the >> accounting data should be possible to be different for both use cases. >> >> Signed-off-by: Juergen Gross <jgross@suse.com> >> --- >> tools/xenstore/xenstored_core.c | 5 +++ >> tools/xenstore/xenstored_core.h | 3 ++ >> tools/xenstore/xenstored_domain.c | 64 +++++++++++++++++++++++++++---- >> tools/xenstore/xenstored_domain.h | 5 ++- >> 4 files changed, 68 insertions(+), 9 deletions(-) >> >> diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c >> index 27dfbe9593..2d10cacf35 100644 >> --- a/tools/xenstore/xenstored_core.c >> +++ b/tools/xenstore/xenstored_core.c >> @@ -1023,6 +1023,9 @@ static void send_error(struct connection *conn, int error) >> break; >> } >> } >> + >> + acc_drop(conn); >> + >> send_reply(conn, XS_ERROR, xsd_errors[i].errstring, >> strlen(xsd_errors[i].errstring) + 1); >> } >> @@ -1060,6 +1063,7 @@ void send_reply(struct connection *conn, enum >> xsd_sockmsg_type type, >> } >> conn->in = NULL; >> + acc_commit(conn); > > AFAIU, if send_reply() is called then we would need to commit the accounting > even if we can't send the reply (i.e. send_reply()). So shouldn't this be call > right at the beginning of send_reply()? Oh, indeed. Good catch! > >> /* Update relevant header fields and fill in the message body. */ >> bdata->hdr.msg.type = type; >> @@ -2195,6 +2199,7 @@ struct connection *new_connection(const struct >> interface_funcs *funcs) >> new->is_stalled = false; >> new->transaction_started = 0; >> INIT_LIST_HEAD(&new->out_list); >> + INIT_LIST_HEAD(&new->acc_list); >> INIT_LIST_HEAD(&new->ref_list); >> INIT_LIST_HEAD(&new->watches); >> INIT_LIST_HEAD(&new->transaction_list); >> diff --git a/tools/xenstore/xenstored_core.h b/tools/xenstore/xenstored_core.h >> index c59b06551f..1f811f38cb 100644 >> --- a/tools/xenstore/xenstored_core.h >> +++ b/tools/xenstore/xenstored_core.h >> @@ -139,6 +139,9 @@ struct connection >> struct list_head out_list; >> uint64_t timeout_msec; >> + /* Not yet committed accounting data (valid if in != NULL). */ >> + struct list_head acc_list; >> + >> /* Referenced requests no longer pending. */ >> struct list_head ref_list; >> diff --git a/tools/xenstore/xenstored_domain.c >> b/tools/xenstore/xenstored_domain.c >> index f459c5aabb..33105dba8f 100644 >> --- a/tools/xenstore/xenstored_domain.c >> +++ b/tools/xenstore/xenstored_domain.c >> @@ -100,7 +100,7 @@ struct changed_domain >> unsigned int domid; >> /* Accounting data. */ >> - int acc[ACC_TR_N]; >> + int acc[]; > > Is this actually worth it? How much memory would we save? Hmm, true. While being more generic, the saved memory is actually zero, as ACC_TR_N and ACC_REQ_N have the same value. And even if they should differ in future, we can just use the higher of both values here. > >> }; >> static struct hashtable *domhash; >> @@ -577,6 +577,7 @@ static struct changed_domain >> *acc_find_changed_domain(struct list_head *head, >> static struct changed_domain *acc_get_changed_domain(const void *ctx, >> struct list_head *head, >> + enum accitem acc_sz, >> unsigned int domid) >> { >> struct changed_domain *cd; >> @@ -585,7 +586,7 @@ static struct changed_domain *acc_get_changed_domain(const >> void *ctx, >> if (cd) >> return cd; >> - cd = talloc_zero(ctx, struct changed_domain); >> + cd = talloc_zero_size(ctx, sizeof(*cd) + acc_sz * sizeof(cd->acc[0])); >> if (!cd) >> return NULL; >> @@ -596,13 +597,13 @@ static struct changed_domain >> *acc_get_changed_domain(const void *ctx, >> } >> static int acc_add_changed_dom(const void *ctx, struct list_head *head, >> - enum accitem what, int val, unsigned int domid) >> + enum accitem acc_sz, enum accitem what, >> + int val, unsigned int domid) >> { >> struct changed_domain *cd; >> - assert(what < ARRAY_SIZE(cd->acc)); >> - >> - cd = acc_get_changed_domain(ctx, head, domid); >> + assert(what < acc_sz); >> + cd = acc_get_changed_domain(ctx, head, acc_sz, domid); >> if (!cd) >> return 0; >> @@ -1071,6 +1072,7 @@ static int domain_acc_add(struct connection *conn, >> unsigned int domid, >> enum accitem what, int add, bool no_dom_alloc) >> { >> struct domain *d; >> + struct changed_domain *cd; >> struct list_head *head; >> int ret; >> @@ -1091,10 +1093,26 @@ static int domain_acc_add(struct connection *conn, >> unsigned int domid, >> } >> } >> + /* Temporary accounting data until final commit? */ >> + if (conn && conn->in && what < ACC_REQ_N) { >> + /* Consider transaction local data. */ >> + ret = 0; >> + if (conn->transaction && what < ACC_TR_N) { >> + head = transaction_get_changed_domains( >> + conn->transaction); >> + cd = acc_find_changed_domain(head, domid); >> + if (cd) >> + ret = cd->acc[what]; >> + } >> + ret += acc_add_changed_dom(conn->in, &conn->acc_list, ACC_REQ_N, >> + what, add, domid); >> + return errno ? -1 : domain_acc_add_chk(d, what, ret, domid); >> + } >> + >> if (conn && conn->transaction && what < ACC_TR_N) { >> head = transaction_get_changed_domains(conn->transaction); >> - ret = acc_add_changed_dom(conn->transaction, head, what, >> - add, domid); >> + ret = acc_add_changed_dom(conn->transaction, head, ACC_TR_N, >> + what, add, domid); >> if (errno) { >> fail_transaction(conn->transaction); >> return -1; >> @@ -1107,6 +1125,36 @@ static int domain_acc_add(struct connection *conn, >> unsigned int domid, >> return d->acc[what]; >> } >> +void acc_drop(struct connection *conn) >> +{ >> + struct changed_domain *cd; >> + >> + while ((cd = list_top(&conn->acc_list, struct changed_domain, list))) { > > NIT: You could use list_for_each_safe(); Yes, at the cost of an additional variable. > >> + 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. > >> + 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. > >> + 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. > >> 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: enum accitem { ACC_NODES, /* Number of elements per request and domain. */ ACC_REQ_N, /* Number of elements per transaction and domain. */ ACC_TR_N = ACC_REQ_N, ACC_WATCH = ACC_TR_N, ACC_OUTST, ACC_MEM, ACC_TRANS, ACC_TRANSNODES, ACC_NPERM, ACC_PATHLEN, ACC_NODESZ, /* Number of elements per domain. */ ACC_N }; Juergen
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. 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. > >> >>> + 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? > >> >>> 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? > ACC_TR_N = ACC_REQ_N, > ACC_WATCH = ACC_TR_N, > ACC_OUTST, > ACC_MEM, > ACC_TRANS, > ACC_TRANSNODES, > ACC_NPERM, > ACC_PATHLEN, > ACC_NODESZ, > /* Number of elements per domain. */ > ACC_N > }; > > > Juergen > Cheers,
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
diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c index 27dfbe9593..2d10cacf35 100644 --- a/tools/xenstore/xenstored_core.c +++ b/tools/xenstore/xenstored_core.c @@ -1023,6 +1023,9 @@ static void send_error(struct connection *conn, int error) break; } } + + acc_drop(conn); + send_reply(conn, XS_ERROR, xsd_errors[i].errstring, strlen(xsd_errors[i].errstring) + 1); } @@ -1060,6 +1063,7 @@ void send_reply(struct connection *conn, enum xsd_sockmsg_type type, } conn->in = NULL; + acc_commit(conn); /* Update relevant header fields and fill in the message body. */ bdata->hdr.msg.type = type; @@ -2195,6 +2199,7 @@ struct connection *new_connection(const struct interface_funcs *funcs) new->is_stalled = false; new->transaction_started = 0; INIT_LIST_HEAD(&new->out_list); + INIT_LIST_HEAD(&new->acc_list); INIT_LIST_HEAD(&new->ref_list); INIT_LIST_HEAD(&new->watches); INIT_LIST_HEAD(&new->transaction_list); diff --git a/tools/xenstore/xenstored_core.h b/tools/xenstore/xenstored_core.h index c59b06551f..1f811f38cb 100644 --- a/tools/xenstore/xenstored_core.h +++ b/tools/xenstore/xenstored_core.h @@ -139,6 +139,9 @@ struct connection struct list_head out_list; uint64_t timeout_msec; + /* Not yet committed accounting data (valid if in != NULL). */ + struct list_head acc_list; + /* Referenced requests no longer pending. */ struct list_head ref_list; diff --git a/tools/xenstore/xenstored_domain.c b/tools/xenstore/xenstored_domain.c index f459c5aabb..33105dba8f 100644 --- a/tools/xenstore/xenstored_domain.c +++ b/tools/xenstore/xenstored_domain.c @@ -100,7 +100,7 @@ struct changed_domain unsigned int domid; /* Accounting data. */ - int acc[ACC_TR_N]; + int acc[]; }; static struct hashtable *domhash; @@ -577,6 +577,7 @@ static struct changed_domain *acc_find_changed_domain(struct list_head *head, static struct changed_domain *acc_get_changed_domain(const void *ctx, struct list_head *head, + enum accitem acc_sz, unsigned int domid) { struct changed_domain *cd; @@ -585,7 +586,7 @@ static struct changed_domain *acc_get_changed_domain(const void *ctx, if (cd) return cd; - cd = talloc_zero(ctx, struct changed_domain); + cd = talloc_zero_size(ctx, sizeof(*cd) + acc_sz * sizeof(cd->acc[0])); if (!cd) return NULL; @@ -596,13 +597,13 @@ static struct changed_domain *acc_get_changed_domain(const void *ctx, } static int acc_add_changed_dom(const void *ctx, struct list_head *head, - enum accitem what, int val, unsigned int domid) + enum accitem acc_sz, enum accitem what, + int val, unsigned int domid) { struct changed_domain *cd; - assert(what < ARRAY_SIZE(cd->acc)); - - cd = acc_get_changed_domain(ctx, head, domid); + assert(what < acc_sz); + cd = acc_get_changed_domain(ctx, head, acc_sz, domid); if (!cd) return 0; @@ -1071,6 +1072,7 @@ static int domain_acc_add(struct connection *conn, unsigned int domid, enum accitem what, int add, bool no_dom_alloc) { struct domain *d; + struct changed_domain *cd; struct list_head *head; int ret; @@ -1091,10 +1093,26 @@ static int domain_acc_add(struct connection *conn, unsigned int domid, } } + /* Temporary accounting data until final commit? */ + if (conn && conn->in && what < ACC_REQ_N) { + /* Consider transaction local data. */ + ret = 0; + if (conn->transaction && what < ACC_TR_N) { + head = transaction_get_changed_domains( + conn->transaction); + cd = acc_find_changed_domain(head, domid); + if (cd) + ret = cd->acc[what]; + } + ret += acc_add_changed_dom(conn->in, &conn->acc_list, ACC_REQ_N, + what, add, domid); + return errno ? -1 : domain_acc_add_chk(d, what, ret, domid); + } + if (conn && conn->transaction && what < ACC_TR_N) { head = transaction_get_changed_domains(conn->transaction); - ret = acc_add_changed_dom(conn->transaction, head, what, - add, domid); + ret = acc_add_changed_dom(conn->transaction, head, ACC_TR_N, + what, add, domid); if (errno) { fail_transaction(conn->transaction); return -1; @@ -1107,6 +1125,36 @@ static int domain_acc_add(struct connection *conn, unsigned int domid, return d->acc[what]; } +void acc_drop(struct connection *conn) +{ + struct changed_domain *cd; + + while ((cd = list_top(&conn->acc_list, struct changed_domain, list))) { + 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. */ + while ((cd = list_top(&conn->acc_list, struct changed_domain, list))) { + list_del(&cd->list); + for (what = 0; what < ACC_REQ_N; what++) + 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, ACC_TR_N, /* Number of elements per transaction and domain. */ ACC_N = ACC_TR_N /* Number of elements per domain. */ }; @@ -103,6 +104,8 @@ 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); +void acc_drop(struct connection *conn); +void acc_commit(struct connection *conn); /* Write rate limiting */
Instead of modifying accounting data and undo those modifications in case of an error during further processing, add a framework for collecting the needed changes and commit them only when the whole operation has succeeded. This scheme can reuse large parts of the per transaction accounting. The changed_domain handling can be reused, but the array size of the accounting data should be possible to be different for both use cases. Signed-off-by: Juergen Gross <jgross@suse.com> --- tools/xenstore/xenstored_core.c | 5 +++ tools/xenstore/xenstored_core.h | 3 ++ tools/xenstore/xenstored_domain.c | 64 +++++++++++++++++++++++++++---- tools/xenstore/xenstored_domain.h | 5 ++- 4 files changed, 68 insertions(+), 9 deletions(-)