Re: [dpdk-dev] [PATCH v3 4/8] examples/power: add host channel to power manager

["Hunt, David" <david.hunt@intel.com>]

Hi Anatoly,


On 25/9/2018 10:48 AM, Burakov, Anatoly wrote:
> On 14-Sep-18 2:54 PM, David Hunt wrote:
>> This patch adds a fifo channel to the vm_power_manager app through which
>> we can send commands and polices. Intended for sending JSON strings.
>> The fifo is at /tmp/powermonitor/fifo.0
>>
>> Signed-off-by: David Hunt <david.hunt@intel.com>
>> ---
>
> A bunch of nitpick comments below :)
>
>> examples/vm_power_manager/channel_manager.c | 108 +++++++++++++++
>>   examples/vm_power_manager/channel_manager.h |  17 ++-
>>   examples/vm_power_manager/channel_monitor.c | 146 +++++++++++++++-----
>>   examples/vm_power_manager/main.c            |   2 +
>>   4 files changed, 238 insertions(+), 35 deletions(-)
>>
>
> <snip>
>
>> +                "Error(%s) setting non-blocking "
>> +                "socket for '%s'\n",
>> +                strerror(errno), info->channel_path);
>> +        return -1;
>> +    }
>> +    return 0;
>> +}
>> +
>
> As far as i can tell, vm power manager is a proper DPDK application, 
> meaning there can technically be several of these running 
> independently under different prefixes. Hardcoded paths are OK, but 
> you probably need to place a write-lock on a file to prevent another 
> VM power manager from (accidentally) taking over the FIFO? Init would 
> probably fail earlier, but you never know :)

vm_power_manager is only expected to be a single instance looking after 
the power across all cores in a system, so we do not expect multiple 
instance of this to be running.

>
>>   static int
>>   setup_channel_info(struct virtual_machine_info **vm_info_dptr,
>>           struct channel_info **chan_info_dptr, unsigned channel_num)
>> @@ -294,6 +327,7 @@ setup_channel_info(struct virtual_machine_info 
>> **vm_info_dptr,
>>       chan_info->channel_num = channel_num;
>>       chan_info->priv_info = (void *)vm_info;
>>       chan_info->status = CHANNEL_MGR_CHANNEL_DISCONNECTED;
>> +    chan_info->type = CHANNEL_TYPE_BINARY;
>>       if (open_non_blocking_channel(chan_info) < 0) {
>>           RTE_LOG(ERR, CHANNEL_MANAGER, "Could not open channel: "
>>                   "'%s' for VM '%s'\n",
>> @@ -316,6 +350,35 @@ setup_channel_info(struct virtual_machine_info 
>> **vm_info_dptr,
>>       return 0;
>>   }
>>   +static int
>> +setup_host_channel_info(struct channel_info **chan_info_dptr,
>> +        unsigned int channel_num)
>> +{
>> +    struct channel_info *chan_info = *chan_info_dptr;
>> +
>> +    chan_info->channel_num = channel_num;
>> +    chan_info->priv_info = (void *)0;
>
> NULL?
>

Fixed in next version.


>> +    chan_info->status = CHANNEL_MGR_CHANNEL_DISCONNECTED;
>> +    chan_info->type = CHANNEL_TYPE_JSON;
>> +    sprintf(chan_info->channel_path, "%sfifo.0", 
>> CHANNEL_MGR_SOCKET_PATH);
>
> Here, 0 is part of the format string...
>
>> +
>> +    if (open_host_channel(chan_info) < 0) {
>> +        RTE_LOG(ERR, CHANNEL_MANAGER, "Could not open host channel: "
>> +                "'%s'\n",
>> +                chan_info->channel_path);
>> +        return -1;
>> +    }
>
> <snip>
>
>> +int
>> +add_host_channel(void)
>> +{
>> +    struct channel_info *chan_info;
>> +    char socket_path[PATH_MAX];
>> +    int num_channels_enabled = 0;
>> +    int ret;
>> +
>> +    snprintf(socket_path, sizeof(socket_path), "%sfifo.%u",
>> +            CHANNEL_MGR_SOCKET_PATH, 0);
>
> ...while here, it's an argument. What's the significance of 0 in this 
> context? Also, maybe better to put it in a function, so as to only 
> have one place to fix if anything changes, instead of two?
>

I'll add a function to generate this string. And I've dropped the ".0". 
It was inherited from the VM channel code, and in this case there is no 
index needed.


>> +
>> +    errno = 0;
>> +    ret = mkfifo(socket_path, 0666);
>
> 0666 seems like overly permissive to me?
>

changed to 660


>> +    if ((errno != EEXIST) && (ret < 0)) {
>> +        printf(" %d %d, %d\n", ret, EEXIST, errno);
>
> This looks like a leftover debug printf?
>

Removed in next rev.


> Also, maybe if (ret < 0 && errno != EEXIST)? I don't think there's a 
> need to set errno beforehand here.
>
>> +        RTE_LOG(ERR, CHANNEL_MANAGER, "Cannot create fifo '%s' error: "
>> +                "%s\n", socket_path, strerror(errno));
>> +        return 0;
>> +    }
>> +
>> +    errno = 0;
>
> ...and here too - if access() call failed, does it not always set 
> errno value?


Removed in next rev.

>
>> +    if (access(socket_path, F_OK) < 0) {
>> +        RTE_LOG(ERR, CHANNEL_MANAGER, "Channel path '%s' error: "
>> +                "%s\n", socket_path, strerror(errno));
>> +        return 0;
>> +    }
>> +    chan_info = rte_malloc(NULL, sizeof(*chan_info),
>> +            RTE_CACHE_LINE_SIZE);
>
> 0 alignment is equivalent to RTE_CACHE_LINE_SIZE, so no need to 
> specify it explicitly.
>

Sure. Changed to 0.

>> +    if (chan_info == NULL) {
>> +        RTE_LOG(ERR, CHANNEL_MANAGER, "Error allocating memory for "
>> +                "channel '%s'\n", socket_path);
>> +        return 0;
>> +    }
>> +    snprintf(chan_info->channel_path,
>> +            sizeof(chan_info->channel_path), "%sfifo.%u",
>> +            CHANNEL_MGR_SOCKET_PATH, 0);
>
> Creating FIFO path again. Definitely needs a function :)
>

Function created. :)

>> +    if (setup_host_channel_info(&chan_info, 0) < 0) {
>> +        rte_free(chan_info);
>> +        return 0;
>> +    }
>> +    num_channels_enabled++;
>> +
>> +    return num_channels_enabled;
>> +}
>> +
>>   int
>>   remove_channel(struct channel_info **chan_info_dptr)
>>   {
>> diff --git a/examples/vm_power_manager/channel_manager.h 
>> b/examples/vm_power_manager/channel_manager.h
>> index 872ec6140..c157cc22b 100644
>> --- a/examples/vm_power_manager/channel_manager.h
>> +++ b/examples/vm_power_manager/channel_manager.h
>> @@ -37,7 +37,7 @@ struct sockaddr_un _sockaddr_un;
>>   #define UNIX_PATH_MAX sizeof(_sockaddr_un.sun_path)
>>   #endif
>>   -#define MAX_VMS 4
>> +#define MAX_VMS 64
>
> This change probably needs to be called out in commit message and 
> explained. Or broken into a separate commit? Also, i think technically 
> "MAX_VMS" is a bad name now that you're supporting containers as well 
> as VM's. MAX_CLIENTS maybe?
>

Sure. I've split it out into it's own commit in next version, and 
renamed it as MAX_CLIENTS.


>>   #define MAX_VCPUS 20
>>     @@ -54,6 +54,11 @@ enum channel_status { 
>> CHANNEL_MGR_CHANNEL_DISCONNECTED = 0,
>>       CHANNEL_MGR_CHANNEL_DISABLED,
>>       CHANNEL_MGR_CHANNEL_PROCESSING};
>>   +/* Communication Channel Type */
>> +enum channel_type { CHANNEL_TYPE_BINARY = 0,
>
> Should probably start values on a new line?
>

I was sticking with the existing style, but I do prefer the new lines. 
Will change in the new version.


>> +    CHANNEL_TYPE_INI,
>> +    CHANNEL_TYPE_JSON};
>> +
>>   /* VM libvirt(qemu/KVM) connection status */
>>   enum vm_status { CHANNEL_MGR_VM_INACTIVE = 0, CHANNEL_MGR_VM_ACTIVE};
>>   @@ -66,6 +71,7 @@ struct channel_info {
>>       volatile uint32_t status;    /**< Connection status(enum 
>> channel_status) */
>
> <snip>
>
>> - pol->core_share[count].pcpu = pcpu;
>> -                    printf("Monitoring pcpu %d\n", pcpu);
>> -                }
>> +    RTE_LOG(INFO, CHANNEL_MONITOR,
>> +            "Looking for pcpu for %s\n", pol->pkt.vm_name);
>> +
>> +    /*
>> +     * So now that we're handling virtual and physical cores, we 
>> need to
>> +     * differenciate between them when adding them to the branch 
>> monitor.
>> +     * Virtual cores need to be converted to physical cores.
>> +     */
>> +
>> +
>> +
>> +
>
> Needs moar newlines :)
>

One can never have enough newlines. :)
Removed in the next version.


>> +    if (pol->pkt.core_type == CORE_TYPE_VIRTUAL) {
>> +        /*
>> +         * If the cores in the policy are virtual, we need to map them
>> +         * to physical core. We look up the vm info and use that for
>> +         * the mapping.
>> +         */
>> +        get_info_vm(pol->pkt.vm_name, &info);
>
> <snip>
>
>> @@ -362,10 +425,12 @@ process_request(struct channel_packet *pkt, 
>> struct channel_info *chan_info)
>>       if (pkt->command == CPU_POWER) {
>>           core_mask = get_pcpus_mask(chan_info, pkt->resource_id);
>>           if (core_mask == 0) {
>> -            RTE_LOG(ERR, CHANNEL_MONITOR, "Error get physical CPU 
>> mask for "
>> -                "channel '%s' using vCPU(%u)\n", 
>> chan_info->channel_path,
>> -                (unsigned)pkt->unit);
>> -            return -1;
>> +            /*
>> +             * Core mask will be 0 in the case where
>> +             * hypervisor is not available so we're working in
>> +             * the host, so use the core as the mask.
>> +             */
>> +            core_mask = 1 << pkt->resource_id;
>
> 1ULL?
>
>

Done in next version .

Thanks for the review, Anatoly.

Rgds,
Dave.