| Message ID | 20230405070349.25293-5-jgross@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | tools/xenstore: rework internal accounting | expand |
Hi Juergen, On 05/04/2023 08:03, 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> > --- > V3: > - call acc_commit() earlier (Julien Grall) > - add assert() to acc_commit() > - use fixed sized acc array in struct changed_domain (Julien Grall) > --- > tools/xenstore/xenstored_core.c | 9 ++++-- > tools/xenstore/xenstored_core.h | 3 ++ > tools/xenstore/xenstored_domain.c | 53 ++++++++++++++++++++++++++++++- > tools/xenstore/xenstored_domain.h | 5 ++- > 4 files changed, 66 insertions(+), 4 deletions(-) > > diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c > index 3ca68681e3..84335f5f3d 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); > } > @@ -1034,6 +1037,9 @@ void send_reply(struct connection *conn, enum xsd_sockmsg_type type, > > assert(type != XS_WATCH_EVENT); > > + conn->in = NULL; AFAIU, you are setting conn->in to NULL in order to please.. > + acc_commit(conn); ... this call. However in case of an error like... > + > if ( len > XENSTORE_PAYLOAD_MAX ) { > send_error(conn, E2BIG); ... here, send_reply() will be called again. But the error will not be set because conn->in is NULL. So I think you want to restore conn->in rewrite acc_commit(). The ordering would also deserve an explanation in a comment. > return; > @@ -1059,8 +1065,6 @@ void send_reply(struct connection *conn, enum xsd_sockmsg_type type, > } > } > > - conn->in = NULL; > - > /* Update relevant header fields and fill in the message body. */ > bdata->hdr.msg.type = type; > bdata->hdr.msg.len = len; > @@ -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 30fb9acec6..144cbafb73 100644 > --- a/tools/xenstore/xenstored_domain.c > +++ b/tools/xenstore/xenstored_domain.c > @@ -91,6 +91,8 @@ struct domain > bool wrl_delay_logged; > }; > > +#define ACC_CHD_N (ACC_TR_N < ACC_REQ_N ? ACC_REQ_N : ACC_TR_N) Both ACC_TR_N and ACC_REQ_N are fixed. Can you explain why we need this magic? Related, wouldn't it be better to define it in the enum? > + > struct changed_domain > { > /* List of all changed domains. */ > @@ -100,7 +102,7 @@ struct changed_domain > unsigned int domid; > > /* Accounting data. */ > - int acc[ACC_TR_N]; > + int acc[ACC_CHD_N]; > }; > > static struct hashtable *domhash; > @@ -1070,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; > > @@ -1090,6 +1093,22 @@ 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, what, > + add, domid); > + return errno ? -1 : domain_acc_add_valid(d, what, ret); > + } > + > if (conn && conn->transaction && what < ACC_TR_N) { > head = transaction_get_changed_domains(conn->transaction); > ret = acc_add_changed_dom(conn->transaction, head, what, > @@ -1106,6 +1125,38 @@ 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; > + enum accitem what; > + > + /* > + * Make sure domain_acc_add() below can't add additional data to > + * to be committed accounting records. > + */ > + assert(!conn->in); > + > + 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); > + } > +} > + > 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 9d05eb01da..6355ad4f37 100644 > --- a/tools/xenstore/xenstored_domain.h > +++ b/tools/xenstore/xenstored_domain.h > @@ -25,7 +25,8 @@ > * a per transaction array. > */ > enum accitem { > - ACC_NODES, > + ACC_REQ_N, /* Number of elements per request. */ > + ACC_NODES = ACC_REQ_N, > ACC_TR_N, /* Number of elements per transaction. */ > ACC_N = ACC_TR_N, /* Number of elements per domain. */ > }; > @@ -113,6 +114,8 @@ int domain_get_quota(const void *ctx, struct connection *conn, > * If "update" is true, "chk_quota" is ignored. > */ > int acc_fix_domains(struct list_head *head, bool chk_quota, bool update); > +void acc_drop(struct connection *conn); > +void acc_commit(struct connection *conn); > > /* Write rate limiting */ > Cheers,
On 25.04.23 19:52, Julien Grall wrote: > Hi Juergen, > > On 05/04/2023 08:03, 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> >> --- >> V3: >> - call acc_commit() earlier (Julien Grall) >> - add assert() to acc_commit() >> - use fixed sized acc array in struct changed_domain (Julien Grall) >> --- >> tools/xenstore/xenstored_core.c | 9 ++++-- >> tools/xenstore/xenstored_core.h | 3 ++ >> tools/xenstore/xenstored_domain.c | 53 ++++++++++++++++++++++++++++++- >> tools/xenstore/xenstored_domain.h | 5 ++- >> 4 files changed, 66 insertions(+), 4 deletions(-) >> >> diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c >> index 3ca68681e3..84335f5f3d 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); >> } >> @@ -1034,6 +1037,9 @@ void send_reply(struct connection *conn, enum >> xsd_sockmsg_type type, >> assert(type != XS_WATCH_EVENT); >> + conn->in = NULL; > > AFAIU, you are setting conn->in to NULL in order to please.. > >> + acc_commit(conn); > > ... this call. However in case of an error like... > >> + >> if ( len > XENSTORE_PAYLOAD_MAX ) { > send_error(conn, E2BIG); > > ... here, send_reply() will be called again. But the error will not be set > because conn->in is NULL. > > So I think you want to restore conn->in rewrite acc_commit(). The ordering would > also deserve an explanation in a comment. Just to make sure I understand you correctly (I have some difficulties parsing "So I think you want to restore conn->in rewrite acc_commit()." completely): You are suggesting to move setting conn->in to NULL to acc_commit() and to restore it before returning? I'm fine with that. > >> return; >> @@ -1059,8 +1065,6 @@ void send_reply(struct connection *conn, enum >> xsd_sockmsg_type type, >> } >> } >> - conn->in = NULL; >> - >> /* Update relevant header fields and fill in the message body. */ >> bdata->hdr.msg.type = type; >> bdata->hdr.msg.len = len; >> @@ -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 30fb9acec6..144cbafb73 100644 >> --- a/tools/xenstore/xenstored_domain.c >> +++ b/tools/xenstore/xenstored_domain.c >> @@ -91,6 +91,8 @@ struct domain >> bool wrl_delay_logged; >> }; >> +#define ACC_CHD_N (ACC_TR_N < ACC_REQ_N ? ACC_REQ_N : ACC_TR_N) > > Both ACC_TR_N and ACC_REQ_N are fixed. Can you explain why we need this magic? > > Related, wouldn't it be better to define it in the enum? I can do that, of course. I just didn't want to make the enum even more complex. :-) But with a comment this should be okay IMO. Juergen
Hi, On 26/04/2023 08:08, Juergen Gross wrote: > On 25.04.23 19:52, Julien Grall wrote: >> Hi Juergen, >> >> On 05/04/2023 08:03, 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> >>> --- >>> V3: >>> - call acc_commit() earlier (Julien Grall) >>> - add assert() to acc_commit() >>> - use fixed sized acc array in struct changed_domain (Julien Grall) >>> --- >>> tools/xenstore/xenstored_core.c | 9 ++++-- >>> tools/xenstore/xenstored_core.h | 3 ++ >>> tools/xenstore/xenstored_domain.c | 53 ++++++++++++++++++++++++++++++- >>> tools/xenstore/xenstored_domain.h | 5 ++- >>> 4 files changed, 66 insertions(+), 4 deletions(-) >>> >>> diff --git a/tools/xenstore/xenstored_core.c >>> b/tools/xenstore/xenstored_core.c >>> index 3ca68681e3..84335f5f3d 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); >>> } >>> @@ -1034,6 +1037,9 @@ void send_reply(struct connection *conn, enum >>> xsd_sockmsg_type type, >>> assert(type != XS_WATCH_EVENT); >>> + conn->in = NULL; >> >> AFAIU, you are setting conn->in to NULL in order to please.. >> >>> + acc_commit(conn); >> >> ... this call. However in case of an error like... >> >>> + >>> if ( len > XENSTORE_PAYLOAD_MAX ) { > >>> send_error(conn, E2BIG); >> >> ... here, send_reply() will be called again. But the error will not be >> set because conn->in is NULL. >> >> So I think you want to restore conn->in rewrite acc_commit(). The >> ordering would also deserve an explanation in a comment. > > Just to make sure I understand you correctly (I have some difficulties > parsing "So I think you want to restore conn->in rewrite acc_commit()." > completely): Hmmm... Not sure why I wrote "rewrite". I was meant to say that you want to restore conn->in after acc_commit() is called. > > You are suggesting to move setting conn->in to NULL to acc_commit() and > to restore it before returning? I'm fine with that. Either that or what I wrote above. It depends on whether you expect other caller to be in the same situation. > >> >>> return; >>> @@ -1059,8 +1065,6 @@ void send_reply(struct connection *conn, enum >>> xsd_sockmsg_type type, >>> } >>> } >>> - conn->in = NULL; >>> - >>> /* Update relevant header fields and fill in the message body. */ >>> bdata->hdr.msg.type = type; >>> bdata->hdr.msg.len = len; >>> @@ -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 30fb9acec6..144cbafb73 100644 >>> --- a/tools/xenstore/xenstored_domain.c >>> +++ b/tools/xenstore/xenstored_domain.c >>> @@ -91,6 +91,8 @@ struct domain >>> bool wrl_delay_logged; >>> }; >>> +#define ACC_CHD_N (ACC_TR_N < ACC_REQ_N ? ACC_REQ_N : ACC_TR_N) >> >> Both ACC_TR_N and ACC_REQ_N are fixed. Can you explain why we need >> this magic? >> >> Related, wouldn't it be better to define it in the enum? > > I can do that, of course. I just didn't want to make the enum even more > complex. :-) My concern is there is a disconnect between the enum and this macro. What would happen if we increase the enum past ACC_REQ_N/ACC_TR_N? Would it be necessary to update ACC_CHD_N? Cheers,
On 27.04.23 18:32, Julien Grall wrote: > Hi, > > On 26/04/2023 08:08, Juergen Gross wrote: >> On 25.04.23 19:52, Julien Grall wrote: >>> Hi Juergen, >>> >>> On 05/04/2023 08:03, 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> >>>> --- >>>> V3: >>>> - call acc_commit() earlier (Julien Grall) >>>> - add assert() to acc_commit() >>>> - use fixed sized acc array in struct changed_domain (Julien Grall) >>>> --- >>>> tools/xenstore/xenstored_core.c | 9 ++++-- >>>> tools/xenstore/xenstored_core.h | 3 ++ >>>> tools/xenstore/xenstored_domain.c | 53 ++++++++++++++++++++++++++++++- >>>> tools/xenstore/xenstored_domain.h | 5 ++- >>>> 4 files changed, 66 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c >>>> index 3ca68681e3..84335f5f3d 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); >>>> } >>>> @@ -1034,6 +1037,9 @@ void send_reply(struct connection *conn, enum >>>> xsd_sockmsg_type type, >>>> assert(type != XS_WATCH_EVENT); >>>> + conn->in = NULL; >>> >>> AFAIU, you are setting conn->in to NULL in order to please.. >>> >>>> + acc_commit(conn); >>> >>> ... this call. However in case of an error like... >>> >>>> + >>>> if ( len > XENSTORE_PAYLOAD_MAX ) { > send_error(conn, E2BIG); >>> >>> ... here, send_reply() will be called again. But the error will not be set >>> because conn->in is NULL. >>> >>> So I think you want to restore conn->in rewrite acc_commit(). The ordering >>> would also deserve an explanation in a comment. >> >> Just to make sure I understand you correctly (I have some difficulties >> parsing "So I think you want to restore conn->in rewrite acc_commit()." >> completely): > > Hmmm... Not sure why I wrote "rewrite". I was meant to say that you want to > restore conn->in after acc_commit() is called. > >> >> You are suggesting to move setting conn->in to NULL to acc_commit() and >> to restore it before returning? I'm fine with that. > > Either that or what I wrote above. It depends on whether you expect other caller > to be in the same situation. I think it should be local to acc_commit(), as conn->in being NULL is a requirement of the acc_commit() handling. > >> >>> >>>> return; >>>> @@ -1059,8 +1065,6 @@ void send_reply(struct connection *conn, enum >>>> xsd_sockmsg_type type, >>>> } >>>> } >>>> - conn->in = NULL; >>>> - >>>> /* Update relevant header fields and fill in the message body. */ >>>> bdata->hdr.msg.type = type; >>>> bdata->hdr.msg.len = len; >>>> @@ -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 30fb9acec6..144cbafb73 100644 >>>> --- a/tools/xenstore/xenstored_domain.c >>>> +++ b/tools/xenstore/xenstored_domain.c >>>> @@ -91,6 +91,8 @@ struct domain >>>> bool wrl_delay_logged; >>>> }; >>>> +#define ACC_CHD_N (ACC_TR_N < ACC_REQ_N ? ACC_REQ_N : ACC_TR_N) >>> >>> Both ACC_TR_N and ACC_REQ_N are fixed. Can you explain why we need this magic? >>> >>> Related, wouldn't it be better to define it in the enum? >> >> I can do that, of course. I just didn't want to make the enum even more >> complex. :-) > > My concern is there is a disconnect between the enum and this macro. What would > happen if we increase the enum past ACC_REQ_N/ACC_TR_N? This expansion is happening in later patches. > Would it be necessary to update ACC_CHD_N? With the #define above: no. With including it in the enum: depends on the values of ACC_REQ_N and ACC_TR_N. ACC_CHD_N must always be equal to the larger one of both. In my current V6 variant I've added it to the enum with a comment: + ACC_CHD_N = ACC_TR_N, /* max(ACC_REQ_N, ACC_TR_N), for changed dom. */ Juergen
diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c index 3ca68681e3..84335f5f3d 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); } @@ -1034,6 +1037,9 @@ void send_reply(struct connection *conn, enum xsd_sockmsg_type type, assert(type != XS_WATCH_EVENT); + conn->in = NULL; + acc_commit(conn); + if ( len > XENSTORE_PAYLOAD_MAX ) { send_error(conn, E2BIG); return; @@ -1059,8 +1065,6 @@ void send_reply(struct connection *conn, enum xsd_sockmsg_type type, } } - conn->in = NULL; - /* Update relevant header fields and fill in the message body. */ bdata->hdr.msg.type = type; bdata->hdr.msg.len = len; @@ -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 30fb9acec6..144cbafb73 100644 --- a/tools/xenstore/xenstored_domain.c +++ b/tools/xenstore/xenstored_domain.c @@ -91,6 +91,8 @@ struct domain bool wrl_delay_logged; }; +#define ACC_CHD_N (ACC_TR_N < ACC_REQ_N ? ACC_REQ_N : ACC_TR_N) + struct changed_domain { /* List of all changed domains. */ @@ -100,7 +102,7 @@ struct changed_domain unsigned int domid; /* Accounting data. */ - int acc[ACC_TR_N]; + int acc[ACC_CHD_N]; }; static struct hashtable *domhash; @@ -1070,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; @@ -1090,6 +1093,22 @@ 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, what, + add, domid); + return errno ? -1 : domain_acc_add_valid(d, what, ret); + } + if (conn && conn->transaction && what < ACC_TR_N) { head = transaction_get_changed_domains(conn->transaction); ret = acc_add_changed_dom(conn->transaction, head, what, @@ -1106,6 +1125,38 @@ 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; + enum accitem what; + + /* + * Make sure domain_acc_add() below can't add additional data to + * to be committed accounting records. + */ + assert(!conn->in); + + 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); + } +} + 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 9d05eb01da..6355ad4f37 100644 --- a/tools/xenstore/xenstored_domain.h +++ b/tools/xenstore/xenstored_domain.h @@ -25,7 +25,8 @@ * a per transaction array. */ enum accitem { - ACC_NODES, + ACC_REQ_N, /* Number of elements per request. */ + ACC_NODES = ACC_REQ_N, ACC_TR_N, /* Number of elements per transaction. */ ACC_N = ACC_TR_N, /* Number of elements per domain. */ }; @@ -113,6 +114,8 @@ int domain_get_quota(const void *ctx, struct connection *conn, * If "update" is true, "chk_quota" is ignored. */ int acc_fix_domains(struct list_head *head, bool chk_quota, 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> --- V3: - call acc_commit() earlier (Julien Grall) - add assert() to acc_commit() - use fixed sized acc array in struct changed_domain (Julien Grall) --- tools/xenstore/xenstored_core.c | 9 ++++-- tools/xenstore/xenstored_core.h | 3 ++ tools/xenstore/xenstored_domain.c | 53 ++++++++++++++++++++++++++++++- tools/xenstore/xenstored_domain.h | 5 ++- 4 files changed, 66 insertions(+), 4 deletions(-)