Re: [PATCH v13 06/12] numa: Extend CLI to provide memory latency and bandwidth information

From: Tao Xu <tao3.xu@intel.com>
To: Eduardo Habkost <ehabkost@redhat.com>,
	Markus Armbruster <armbru@redhat.com>,
	Igor Mammedov <imammedo@redhat.com>
Cc: "Liu, Jingqi" <jingqi.liu@intel.com>,
	"Du, Fan" <fan.du@intel.com>,
	"qemu-devel@nongnu.org" <qemu-devel@nongnu.org>,
	"jonathan.cameron@huawei.com" <jonathan.cameron@huawei.com>
Subject: Re: [PATCH v13 06/12] numa: Extend CLI to provide memory latency and bandwidth information
Date: Mon, 28 Oct 2019 10:05:21 +0800	[thread overview]
Message-ID: <71543104-7254-c25e-e87c-d73a894bcc2e@intel.com> (raw)
In-Reply-To: <20191025205141.GF6744@habkost.net>

On 10/26/2019 4:51 AM, Eduardo Habkost wrote:
> On Fri, Oct 25, 2019 at 09:44:50PM +0200, Markus Armbruster wrote:
>> Igor Mammedov <imammedo@redhat.com> writes:
>>
>>> On Fri, 25 Oct 2019 14:33:53 +0800
>>> Tao Xu <tao3.xu@intel.com> wrote:
>>>
>>>> On 10/23/2019 11:28 PM, Igor Mammedov wrote:
>>>>> On Sun, 20 Oct 2019 19:11:19 +0800
>>>>> Tao Xu <tao3.xu@intel.com> wrote:
>>>> [...]
>>>>>> +#
>>>>>> +# @access-bandwidth: access bandwidth (MB/s)
>>>>>> +#
>>>>>> +# @read-bandwidth: read bandwidth (MB/s)
>>>>>> +#
>>>>>> +# @write-bandwidth: write bandwidth (MB/s)
>>>>> I think units here are not appropriate, values stored in fields are
>>>>> minimal base units only and nothing else (i.e. ps and B/s)
>>>>>    
>>>> Eric suggest me to drop picoseconds. So here I can use ns. For
>>>> bandwidth, if we use B/s here, does it let user or developer to
>>>> misunderstand that the smallest unit is B/s ?
>>>
>>> It's not nanoseconds or MB/s stored in theses fields, isn't it?
>>> I'd specify units in which value is stored or drop units altogether.
>>>
>>> Maybe Eric and Markus can suggest a better way to describe fields.
>>
>> This isn't review (yet), just an attempt to advise more quickly on
>> general QAPI/QMP conventions.
>>
>> Unit prefixes like Mebi- are nice for humans, because 1MiB is clearer
>> than 1048576B.
>>
>> QMP is for machines.  We eschew unit prefixes and unit symbols there.
>> The unit is implied.  Unit prefixes only complicate things.  Machines
>> can deal with 1048576 easily.  Also dealing 1024Ki and 1Mi is additional
>> work.  We therefore use JSON numbers for byte counts, not strings with
>> units.
>>
>> The general rule is "always use the plainest implied unit that would
>> do."  There are exceptions, mostly due to review failure.
>>
>> Byte rates should be in bytes per second.
>>
>> For time, we've made a godawful mess.  The plainest unit is clearly the
>> second.  We commonly need sub-second granularity, though.
>> Floating-point seconds are unpopular for some reason :)  Instead we use
>> milli-, micro-, and nanoseconds, and even (seconds, microseconds) pairs.
>>
>> QAPI schema documentation describes both the generated C and the QMP
>> wire protocol.  It must be written with the implied unit.  If you send a
>> byte rate in bytes per second via QMP, that's what you document.  Even
>> if a human interface lets you specify the byte rate in MiB/s.
>>
>> Does this make sense?
> 
> This makes sense for the bandwidth fields.  We still need to
> decide how to represent the latency field, though.
> 
> Seconds would be the obvious choice, if only it didn't risk
> silently losing precision when converting numbers to floats.
> 
Got it. I will use bytes per second for bandwidth here. Usually we use 
nanosecond for memory latency, so if we use second for latency, it may 
lose precision. So can I use nanosecond here, because we now use 
nanosecond as smallest time unit.