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

[Tao Xu <tao3.xu@intel.com>]

On 10/22/2019 3:08 PM, Igor Mammedov wrote:
> On Sun, 20 Oct 2019 19:11:19 +0800
> Tao Xu <tao3.xu@intel.com> wrote:
> 
>> From: Liu Jingqi <jingqi.liu@intel.com>
>>
>> Add -numa hmat-lb option to provide System Locality Latency and
>> Bandwidth Information. These memory attributes help to build
>> System Locality Latency and Bandwidth Information Structure(s)
>> in ACPI Heterogeneous Memory Attribute Table (HMAT).
>>
>> Signed-off-by: Liu Jingqi <jingqi.liu@intel.com>
>> Signed-off-by: Tao Xu <tao3.xu@intel.com>
>> ---
[...]
>> +void parse_numa_hmat_lb(NumaState *numa_state, NumaHmatLBOptions *node,
>> +                        Error **errp)
>> +{
>> +    int first_bit, last_bit;
>> +    uint64_t temp_latency;
>> +    NodeInfo *numa_info = numa_state->nodes;
>> +    HMAT_LB_Info *hmat_lb =
>> +        numa_state->hmat_lb[node->hierarchy][node->data_type];
>> +    HMAT_LB_Data lb_data;
>> +
>> +    /* Error checking */
>> +    if (node->initiator >= numa_state->num_nodes) {
>> +        error_setg(errp, "Invalid initiator=%d, it should be less than %d.",
>> +                   node->initiator, numa_state->num_nodes);
>> +        return;
>> +    }
>> +    if (node->target >= numa_state->num_nodes) {
>> +        error_setg(errp, "Invalid target=%d, it should be less than %d.",
>> +                   node->target, numa_state->num_nodes);
>> +        return;
>> +    }
>> +    if (!numa_info[node->initiator].has_cpu) {
>> +        error_setg(errp, "Invalid initiator=%d, it isn't an "
>> +                   "initiator proximity domain.", node->initiator);
>> +        return;
>> +    }
>> +    if (!numa_info[node->target].present) {
>> +        error_setg(errp, "Invalid target=%d, it hasn't a valid NUMA node.",
>> +                   node->target);
>> +        return;
>> +    }
>> +
>> +    if (!hmat_lb) {
>> +        hmat_lb = g_malloc0(sizeof(*hmat_lb));
>> +        numa_state->hmat_lb[node->hierarchy][node->data_type] = hmat_lb;
>> +        hmat_lb->latency = g_array_new(false, true, sizeof(HMAT_LB_Data));
>> +        hmat_lb->bandwidth = g_array_new(false, true, sizeof(HMAT_LB_Data));
>> +    }
>> +    hmat_lb->hierarchy = node->hierarchy;
>> +    hmat_lb->data_type = node->data_type;
>> +    lb_data.initiator = node->initiator;
>> +    lb_data.target = node->target;
>> +
>> +    /* Input latency data */
>> +    if (node->data_type <= HMATLB_DATA_TYPE_WRITE_LATENCY) {
>> +        if (!node->has_latency) {
>> +            error_setg(errp, "Missing 'latency' option.");
>> +            return;
>> +        }
>> +        if (node->has_bandwidth) {
>> +            error_setg(errp, "Invalid option 'bandwidth' since "
>> +                       "the data type is latency.");
>> +            return;
>> +        }
>> +
>> +        temp_latency = node->latency;
>> +        hmat_lb->base_latency = 1;
>> +        while (QEMU_IS_ALIGNED(temp_latency, 10)) {
>> +            temp_latency /= 10;
>> +            hmat_lb->base_latency *= 10;
>> +        }
>> +
>> +        if (temp_latency >= UINT64_MAX) {
>                              ^  ^^^^ doesn't make sense
> 
> can't you use range bitmap here as well?
> 
Because the time unit is decimal. For example, if latency are 
1ns(1000ps, bit 0011 1110 1000) and 65534ns(0011 1110 0111 1111 1000 
0011 0000)

0000 0000 0000 0000 0011 1110 1000
0011 1110 0111 1111 1000 0011 0000

Then we can get the first_bit is 3 and last_bit is 25, 25 - 3 > 16.

But if we use 10 to find base, we can find the base is 1000ps, 
compressed latency are 1 and 65534(< UINT16_MAX).