From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <anatoly.burakov@intel.com>
Received: from mga17.intel.com (mga17.intel.com [192.55.52.151])
 by dpdk.org (Postfix) with ESMTP id 63B8758C6
 for <dev@dpdk.org>; Wed, 27 Mar 2019 15:23:31 +0100 (CET)
X-Amp-Result: SKIPPED(no attachment in message)
X-Amp-File-Uploaded: False
Received: from orsmga004.jf.intel.com ([10.7.209.38])
 by fmsmga107.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384;
 27 Mar 2019 07:23:30 -0700
X-ExtLoop1: 1
X-IronPort-AV: E=Sophos;i="5.60,277,1549958400"; d="scan'208";a="286337518"
Received: from aburakov-mobl1.ger.corp.intel.com (HELO [10.237.220.103])
 ([10.237.220.103])
 by orsmga004.jf.intel.com with ESMTP; 27 Mar 2019 07:23:29 -0700
To: Hajkowski <marcinx.hajkowski@intel.com>, david.hunt@intel.com
Cc: dev@dpdk.org
References: <20190321105506.6656-1-marcinx.hajkowski@intel.com>
 <20190321105506.6656-3-marcinx.hajkowski@intel.com>
From: "Burakov, Anatoly" <anatoly.burakov@intel.com>
Message-ID: <63269bb6-ad80-2ac7-a337-880b8dec6c9e@intel.com>
Date: Wed, 27 Mar 2019 14:23:28 +0000
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:60.0) Gecko/20100101
 Thunderbird/60.6.0
MIME-Version: 1.0
In-Reply-To: <20190321105506.6656-3-marcinx.hajkowski@intel.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Subject: Re: [dpdk-dev] [PATCH v3 2/4] power: extend guest channel api for
 reading
X-BeenThere: dev@dpdk.org
X-Mailman-Version: 2.1.15
Precedence: list
List-Id: DPDK patches and discussions <dev.dpdk.org>
List-Unsubscribe: <https://mails.dpdk.org/options/dev>,
 <mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://mails.dpdk.org/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://mails.dpdk.org/listinfo/dev>,
 <mailto:dev-request@dpdk.org?subject=subscribe>
X-List-Received-Date: Wed, 27 Mar 2019 14:23:32 -0000

On 21-Mar-19 10:55 AM, Hajkowski wrote:
> From: Marcin Hajkowski <marcinx.hajkowski@intel.com>
> 
> Added new experimental API rte_power_guest_channel_receive_msg
> which gives possibility to receive messages send to guest.
> 
> Signed-off-by: Marcin Hajkowski <marcinx.hajkowski@intel.com>
> ---
>   lib/librte_power/channel_commands.h    |  5 ++
>   lib/librte_power/guest_channel.c       | 65 ++++++++++++++++++++++++++
>   lib/librte_power/guest_channel.h       | 35 ++++++++++++++
>   lib/librte_power/rte_power_version.map |  1 +
>   4 files changed, 106 insertions(+)
> 
> diff --git a/lib/librte_power/channel_commands.h b/lib/librte_power/channel_commands.h
> index e7b93a797..33fd53a6d 100644
> --- a/lib/librte_power/channel_commands.h
> +++ b/lib/librte_power/channel_commands.h
> @@ -28,6 +28,11 @@ extern "C" {
>   #define CPU_POWER_SCALE_MIN     4
>   #define CPU_POWER_ENABLE_TURBO  5
>   #define CPU_POWER_DISABLE_TURBO 6
> +
> +/* Generic Power Command Response */
> +#define CPU_POWER_CMD_ACK       1
> +#define CPU_POWER_CMD_NACK      2
> +
>   #define HOURS 24
>   
>   #define MAX_VFS 10
> diff --git a/lib/librte_power/guest_channel.c b/lib/librte_power/guest_channel.c
> index 9cf7d2cb2..3213fa769 100644
> --- a/lib/librte_power/guest_channel.c
> +++ b/lib/librte_power/guest_channel.c
> @@ -10,6 +10,7 @@
>   #include <fcntl.h>
>   #include <string.h>
>   #include <errno.h>
> +#include <poll.h>
>   
>   
>   #include <rte_log.h>
> @@ -19,6 +20,9 @@
>   
>   #define RTE_LOGTYPE_GUEST_CHANNEL RTE_LOGTYPE_USER1
>   
> +/* Timeout for incoming message in milliseconds. */
> +#define TIMEOUT 10
> +
>   static int global_fds[RTE_MAX_LCORE] = { [0 ... RTE_MAX_LCORE-1] = -1 };
>   
>   int
> @@ -125,6 +129,67 @@ int rte_power_guest_channel_send_msg(struct channel_packet *pkt,
>   	return guest_channel_send_msg(pkt, lcore_id);
>   }
>   
> +int power_guest_channel_read_msg(struct channel_packet *pkt,
> +			unsigned int lcore_id)
> +{
> +	int ret;
> +
> +	struct pollfd fds;

Nitpick, but we generally prefer having a newline after definitions.

> +	fds.fd = global_fds[lcore_id];
> +	fds.events = POLLIN;
> +
> +	ret = poll(&fds, 1, TIMEOUT);
> +	if (ret == 0) {
> +		RTE_LOG(DEBUG, GUEST_CHANNEL, "Timeout occurs during poll function.\n");

Occurred?

> +		return -1;
> +	} else if (ret < 0) {
> +		RTE_LOG(ERR, GUEST_CHANNEL, "Error occurred during poll function: %s\n",
> +				strerror(ret));
> +		return -1;
> +	}
> +
> +	void *buffer = pkt;

Declarations have to be in the beginning of the function.

> +
> +	if (lcore_id >= RTE_MAX_LCORE) {
> +		RTE_LOG(ERR, GUEST_CHANNEL, "Channel(%u) is out of range 0...%d\n",
> +				lcore_id, RTE_MAX_LCORE-1);
> +		return -1;
> +	}
> +
> +	if (global_fds[lcore_id] < 0) {
> +		RTE_LOG(ERR, GUEST_CHANNEL, "Channel is not connected\n");
> +		return -1;
> +	}
> +
> +	int buffer_len = sizeof(*pkt);

Same, declarations on top.

> +
> +	while (buffer_len > 0) {
> +		ret = read(global_fds[lcore_id],
> +				buffer, buffer_len);
> +		if (ret < 0) {
> +			if (errno == EINTR)
> +				continue;
> +			return -1;
> +		}
> +		/* No data has been read, this can indicate not
> +		 * consistent buffer_len with actual data size.
> +		 */
> +		if (ret == 0) {
> +			RTE_LOG(ERR, GUEST_CHANNEL, "No more data has been read.\n");
> +			return -1;
> +		}

If i remember correctly, this cannot happen with a blocking fifo read 
unless the fifo was closed either by us or by the other end. Perhaps the 
error message should be reworded to reflect that? E.g. something like 
"Expected more data, but fifo is closed"?

-- 
Thanks,
Anatoly

From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <dev-bounces@dpdk.org>
Received: from dpdk.org (dpdk.org [92.243.14.124])
	by dpdk.space (Postfix) with ESMTP id 0CE76A05D3
	for <public@inbox.dpdk.org>; Wed, 27 Mar 2019 15:23:34 +0100 (CET)
Received: from [92.243.14.124] (localhost [127.0.0.1])
	by dpdk.org (Postfix) with ESMTP id D4F3858CB;
	Wed, 27 Mar 2019 15:23:32 +0100 (CET)
Received: from mga17.intel.com (mga17.intel.com [192.55.52.151])
 by dpdk.org (Postfix) with ESMTP id 63B8758C6
 for <dev@dpdk.org>; Wed, 27 Mar 2019 15:23:31 +0100 (CET)
X-Amp-Result: SKIPPED(no attachment in message)
X-Amp-File-Uploaded: False
Received: from orsmga004.jf.intel.com ([10.7.209.38])
 by fmsmga107.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384;
 27 Mar 2019 07:23:30 -0700
X-ExtLoop1: 1
X-IronPort-AV: E=Sophos;i="5.60,277,1549958400"; d="scan'208";a="286337518"
Received: from aburakov-mobl1.ger.corp.intel.com (HELO [10.237.220.103])
 ([10.237.220.103])
 by orsmga004.jf.intel.com with ESMTP; 27 Mar 2019 07:23:29 -0700
To: Hajkowski <marcinx.hajkowski@intel.com>, david.hunt@intel.com
Cc: dev@dpdk.org
References: <20190321105506.6656-1-marcinx.hajkowski@intel.com>
 <20190321105506.6656-3-marcinx.hajkowski@intel.com>
From: "Burakov, Anatoly" <anatoly.burakov@intel.com>
Message-ID: <63269bb6-ad80-2ac7-a337-880b8dec6c9e@intel.com>
Date: Wed, 27 Mar 2019 14:23:28 +0000
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:60.0) Gecko/20100101
 Thunderbird/60.6.0
MIME-Version: 1.0
In-Reply-To: <20190321105506.6656-3-marcinx.hajkowski@intel.com>
Content-Type: text/plain; charset="UTF-8"; format="flowed"
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Subject: Re: [dpdk-dev] [PATCH v3 2/4] power: extend guest channel api for
 reading
X-BeenThere: dev@dpdk.org
X-Mailman-Version: 2.1.15
Precedence: list
List-Id: DPDK patches and discussions <dev.dpdk.org>
List-Unsubscribe: <https://mails.dpdk.org/options/dev>,
 <mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://mails.dpdk.org/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://mails.dpdk.org/listinfo/dev>,
 <mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-bounces@dpdk.org
Sender: "dev" <dev-bounces@dpdk.org>
Message-ID: <20190327142328.1gHFG1XGauzasJELCUXuyh90a4MTN91ve1YxcqghY-c@z>

On 21-Mar-19 10:55 AM, Hajkowski wrote:
> From: Marcin Hajkowski <marcinx.hajkowski@intel.com>
> 
> Added new experimental API rte_power_guest_channel_receive_msg
> which gives possibility to receive messages send to guest.
> 
> Signed-off-by: Marcin Hajkowski <marcinx.hajkowski@intel.com>
> ---
>   lib/librte_power/channel_commands.h    |  5 ++
>   lib/librte_power/guest_channel.c       | 65 ++++++++++++++++++++++++++
>   lib/librte_power/guest_channel.h       | 35 ++++++++++++++
>   lib/librte_power/rte_power_version.map |  1 +
>   4 files changed, 106 insertions(+)
> 
> diff --git a/lib/librte_power/channel_commands.h b/lib/librte_power/channel_commands.h
> index e7b93a797..33fd53a6d 100644
> --- a/lib/librte_power/channel_commands.h
> +++ b/lib/librte_power/channel_commands.h
> @@ -28,6 +28,11 @@ extern "C" {
>   #define CPU_POWER_SCALE_MIN     4
>   #define CPU_POWER_ENABLE_TURBO  5
>   #define CPU_POWER_DISABLE_TURBO 6
> +
> +/* Generic Power Command Response */
> +#define CPU_POWER_CMD_ACK       1
> +#define CPU_POWER_CMD_NACK      2
> +
>   #define HOURS 24
>   
>   #define MAX_VFS 10
> diff --git a/lib/librte_power/guest_channel.c b/lib/librte_power/guest_channel.c
> index 9cf7d2cb2..3213fa769 100644
> --- a/lib/librte_power/guest_channel.c
> +++ b/lib/librte_power/guest_channel.c
> @@ -10,6 +10,7 @@
>   #include <fcntl.h>
>   #include <string.h>
>   #include <errno.h>
> +#include <poll.h>
>   
>   
>   #include <rte_log.h>
> @@ -19,6 +20,9 @@
>   
>   #define RTE_LOGTYPE_GUEST_CHANNEL RTE_LOGTYPE_USER1
>   
> +/* Timeout for incoming message in milliseconds. */
> +#define TIMEOUT 10
> +
>   static int global_fds[RTE_MAX_LCORE] = { [0 ... RTE_MAX_LCORE-1] = -1 };
>   
>   int
> @@ -125,6 +129,67 @@ int rte_power_guest_channel_send_msg(struct channel_packet *pkt,
>   	return guest_channel_send_msg(pkt, lcore_id);
>   }
>   
> +int power_guest_channel_read_msg(struct channel_packet *pkt,
> +			unsigned int lcore_id)
> +{
> +	int ret;
> +
> +	struct pollfd fds;

Nitpick, but we generally prefer having a newline after definitions.

> +	fds.fd = global_fds[lcore_id];
> +	fds.events = POLLIN;
> +
> +	ret = poll(&fds, 1, TIMEOUT);
> +	if (ret == 0) {
> +		RTE_LOG(DEBUG, GUEST_CHANNEL, "Timeout occurs during poll function.\n");

Occurred?

> +		return -1;
> +	} else if (ret < 0) {
> +		RTE_LOG(ERR, GUEST_CHANNEL, "Error occurred during poll function: %s\n",
> +				strerror(ret));
> +		return -1;
> +	}
> +
> +	void *buffer = pkt;

Declarations have to be in the beginning of the function.

> +
> +	if (lcore_id >= RTE_MAX_LCORE) {
> +		RTE_LOG(ERR, GUEST_CHANNEL, "Channel(%u) is out of range 0...%d\n",
> +				lcore_id, RTE_MAX_LCORE-1);
> +		return -1;
> +	}
> +
> +	if (global_fds[lcore_id] < 0) {
> +		RTE_LOG(ERR, GUEST_CHANNEL, "Channel is not connected\n");
> +		return -1;
> +	}
> +
> +	int buffer_len = sizeof(*pkt);

Same, declarations on top.

> +
> +	while (buffer_len > 0) {
> +		ret = read(global_fds[lcore_id],
> +				buffer, buffer_len);
> +		if (ret < 0) {
> +			if (errno == EINTR)
> +				continue;
> +			return -1;
> +		}
> +		/* No data has been read, this can indicate not
> +		 * consistent buffer_len with actual data size.
> +		 */
> +		if (ret == 0) {
> +			RTE_LOG(ERR, GUEST_CHANNEL, "No more data has been read.\n");
> +			return -1;
> +		}

If i remember correctly, this cannot happen with a blocking fifo read 
unless the fifo was closed either by us or by the other end. Perhaps the 
error message should be reworded to reflect that? E.g. something like 
"Expected more data, but fifo is closed"?

-- 
Thanks,
Anatoly