On Thursday, 2020-08-27 at 22:47:15 +08, Zheng Chuan wrote: > On 2020/8/27 21:07, David Edmondson wrote: >> On Thursday, 2020-08-27 at 20:55:51 +08, Zheng Chuan wrote: >> >>> On 2020/8/27 19:58, David Edmondson wrote: >>>> On Thursday, 2020-08-27 at 17:34:13 +08, Zheng Chuan wrote: >>>> >>>>>>> + /* >>>>>>> + * Only support query once for each calculation, >>>>>>> + * reset as DIRTY_RATE_STATUS_UNSTARTED after query >>>>>>> + */ >>>>>>> + (void)dirtyrate_set_state(&CalculatingState, CalculatingState, >>>>>>> + DIRTY_RATE_STATUS_UNSTARTED); >>>>>> >>>>>> Is there a reason for this restriction? Removing it would require >>>>>> clarifying the state model, I suppose. >>>>>> >>>>> We only support query once for each calculation. >>>>> Otherwise, it could always query dirtyrate, but maybe the dirtyrate is >>>>> calculated >>>>> long time ago. >>>> >>>> There's nothing in the current interface that prevents this from being >>>> the case already - the caller could initiate a 1 second sample, then >>>> wait 24 hours to query the result. >>>> >>>> Obviously this would generally be regarded as "d'oh - don't do that", >>>> but the same argument would apply if the caller is allowed to query the >>>> results multiple times. >>>> >>>> Perhaps a complete solution would be to include information about the >>>> sample period with the result. The caller could then determine whether >>>> the sample is of adequate quality (sufficiently recent, taken over a >>>> sufficiently long time period) for its' intended use. >>>> >>> You mean add timestamp when i calculate? >> >> You already have a timestamp, though I'm not sure if it is one that is >> appropriate to report to a user. >> >> I was thinking that you would include both the start time and duration >> of the sample in the output of the query-dirty-rate QMP command, as well >> as the dirty rate itself. That way the caller can make a decision about >> whether the data is useful. >> > OK, i understand. > I may add it like this: > +## > +{ 'struct': 'DirtyRateInfo', > + 'data': {'dirty-rate': 'int64', > + 'status': 'DirtyRateStatus', > + 'start-timestamp': 'int64', > + 'calc-time': 'int64'} } > + > +## > the stat-timestamp would be initial_time which gets from > qemu_clock_get_ms(QEMU_CLOCK_REALTIME) > at the beginning of calculation while calc_time is time-duration in > microsecond.
The calc-time reported here should be in the same units as when it is specified in calc-dirty-rate (seconds for both seems fine). I suspect that providing the start-timestamp in seconds would also be fine - it's not obvious that knowing the value in milliseconds adds much value. > But i reconsider that, it maybe still need to reset the CalculatingState as > DIRTY_RATE_STATUS_UNSTARTED > here? > > Initialization like: > void qmp_calc_dirty_rate(int64_t calc_time, Error **errp) > { > XXXX > > if (CalculatingState == DIRTY_RATE_STATUS_MEASURING) { > return; > } > > > (void)dirtyrate_set_state(&CalculatingState, CalculatingState, > DIRTY_RATE_STATUS_UNSTARTED); > XXXX > } > > It could not prevent concurrent scene which may lead to disorder state:( It should be possible to initiate measurement when the state is either UNSTARTED or MEASURED - only MEASURING should rule it out. > > >>> Actually, I do not want make it complicate for qemu code, >>> maybe it could be left for user to implement both two qmp commands >>> like in libvirt-api. >> >> Sorry, I didn't understand this comment. >> >>> On the other hand, it really bother me that we need to reset calculating >>> state >>> to make sure the state model could be restart in next calculation. >>> >>> For now, i put it after query_dirty_rate_info is finished as you see, it >>> should not be a good idea:( >>> >>> Maybe it is better to initialize at the beginning of qmp_calc_dirty_rate(). >>> >>>> dme. >>>> >> >> dme. >> dme. -- Everyone I know, goes away in the end.