Re: [dpdk-dev] [PATCH v3 3/4] power: process incoming confirmation cmds

["Burakov, Anatoly" <anatoly.burakov@intel.com>]

On 21-Mar-19 10:55 AM, Hajkowski wrote:
> From: Marcin Hajkowski <marcinx.hajkowski@intel.com>
> 
> Extend vm_power_guest to check incoming confirmations
> of messages previously sent to host.
> 
> Signed-off-by: Marcin Hajkowski <marcinx.hajkowski@intel.com>
> ---
>   examples/vm_power_manager/guest_cli/Makefile  |  1 +
>   .../guest_cli/vm_power_cli_guest.c            | 65 +++++++++++++++----
>   2 files changed, 54 insertions(+), 12 deletions(-)
> 
> diff --git a/examples/vm_power_manager/guest_cli/Makefile b/examples/vm_power_manager/guest_cli/Makefile
> index a5634eacf..51a5010ab 100644
> --- a/examples/vm_power_manager/guest_cli/Makefile
> +++ b/examples/vm_power_manager/guest_cli/Makefile
> @@ -18,6 +18,7 @@ SRCS-y := main.c vm_power_cli_guest.c parse.c
>   
>   CFLAGS += -O3 -I$(RTE_SDK)/lib/librte_power/
>   CFLAGS += $(WERROR_FLAGS)
> +CFLAGS += -DALLOW_EXPERIMENTAL_API
>   
>   # workaround for a gcc bug with noreturn attribute
>   # http://gcc.gnu.org/bugzilla/show_bug.cgi?id=12603
> diff --git a/examples/vm_power_manager/guest_cli/vm_power_cli_guest.c b/examples/vm_power_manager/guest_cli/vm_power_cli_guest.c
> index 2d9e7689a..698dd5062 100644
> --- a/examples/vm_power_manager/guest_cli/vm_power_cli_guest.c
> +++ b/examples/vm_power_manager/guest_cli/vm_power_cli_guest.c
> @@ -132,6 +132,26 @@ struct cmd_set_cpu_freq_result {
>   	cmdline_fixed_string_t cmd;
>   };
>   
> +static int
> +check_response_cmd(unsigned int lcore_id, int *result)
> +{
> +	struct channel_packet pkt;
> +	int ret;
> +
> +	ret = rte_power_guest_channel_receive_msg(&pkt, lcore_id);
> +	if (ret < 0)
> +		return -1;
> +
> +	if (pkt.command != CPU_POWER_CMD_ACK &&
> +		pkt.command != CPU_POWER_CMD_NACK) {
> +		RTE_LOG(DEBUG, POWER, "Not expected command has been received.\n");
> +		return -1;
> +	}

Maybe use switch?

E.g.

swithc (pkt.command) {
case ACK:
case NACK:
     *result = blah;
     break;
default:
     log failure;
     fail;
}

> +
> +	*result = (pkt.command == CPU_POWER_CMD_ACK);
> +	return 0;
> +}
> +
>   static void
>   cmd_set_cpu_freq_parsed(void *parsed_result, struct cmdline *cl,
>   		       __attribute__((unused)) void *data)
> @@ -139,20 +159,31 @@ cmd_set_cpu_freq_parsed(void *parsed_result, struct cmdline *cl,
>   	int ret = -1;
>   	struct cmd_set_cpu_freq_result *res = parsed_result;
>   
> -	if (!strcmp(res->cmd , "up"))
> +	if (!strcmp(res->cmd, "up"))
>   		ret = rte_power_freq_up(res->lcore_id);
> -	else if (!strcmp(res->cmd , "down"))
> +	else if (!strcmp(res->cmd, "down"))
>   		ret = rte_power_freq_down(res->lcore_id);
> -	else if (!strcmp(res->cmd , "min"))
> +	else if (!strcmp(res->cmd, "min"))
>   		ret = rte_power_freq_min(res->lcore_id);
> -	else if (!strcmp(res->cmd , "max"))
> +	else if (!strcmp(res->cmd, "max"))
>   		ret = rte_power_freq_max(res->lcore_id);
>   	else if (!strcmp(res->cmd, "enable_turbo"))
>   		ret = rte_power_freq_enable_turbo(res->lcore_id);
>   	else if (!strcmp(res->cmd, "disable_turbo"))
>   		ret = rte_power_freq_disable_turbo(res->lcore_id);
> -	if (ret != 1)
> +
> +	if (ret != 1) {
>   		cmdline_printf(cl, "Error sending message: %s\n", strerror(ret));
> +		return;
> +	}
> +	int result;
> +	ret = check_response_cmd(res->lcore_id, &result);
> +	if (ret < 0) {
> +		RTE_LOG(DEBUG, POWER, "No confirmation for sent msg received\n");
> +	} else {
> +		cmdline_printf(cl, "Ack for sent msg received with result: %s.\n",
> +				result == 1 ? "SUCCESS" : "ERROR");

Here and in other places: others can correct me if i'm wrong, but i'm 
pretty sure RTE_LOG(..., POWER) is reserved for logs related to power 
library, and are not supposed to be used in user applications. If 
logging in an application is required, there are USER log levels.

I can see that the app in question already does that in a few places, so 
it's kind of consistent with how things are already, but i would 
question the value of consistency in cases like these. Not sure how to 
proceed here.

> +	}
>   }
>   
>   cmdline_parse_token_string_t cmd_set_cpu_freq =
> @@ -185,16 +216,26 @@ struct cmd_send_policy_result {
>   };
>   
>   static inline int
> -send_policy(struct channel_packet *pkt)
> +send_policy(struct channel_packet *pkt, struct cmdline *cl)
>   {
>   	int ret;
>   
>   	ret = rte_power_guest_channel_send_msg(pkt, 1);
> -	if (ret == 0)
> -		return 1;
> -	RTE_LOG(DEBUG, POWER, "Error sending message: %s\n",
> -			ret > 0 ? strerror(ret) : "channel not connected");
> -	return -1;
> +	if (ret < 0) {
> +		RTE_LOG(DEBUG, POWER, "Error sending message: %s\n",
> +				ret > 0 ? strerror(ret) : "channel not connected");
> +		return -1;
> +	}
> +
> +	int result;
> +	ret = check_response_cmd(1, &result);
> +	if (ret < 0) {
> +		RTE_LOG(DEBUG, POWER, "No confirmation for sent policy received\n");
> +	} else {
> +		cmdline_printf(cl, "Ack for sent policy received with result: %s.\n",
> +				result == 1 ? "SUCCESS" : "ERROR");
> +	}
> +	return 1;
>   }
>   
>   static void
> @@ -206,7 +247,7 @@ cmd_send_policy_parsed(void *parsed_result, struct cmdline *cl,
>   
>   	if (!strcmp(res->cmd, "now")) {
>   		printf("Sending Policy down now!\n");
> -		ret = send_policy(&policy);
> +		ret = send_policy(&policy, cl);
>   	}
>   	if (ret != 1)
>   		cmdline_printf(cl, "Error sending message: %s\n",
> 


-- 
Thanks,
Anatoly