[dpdk-dev,06/10] Alternate implementation of librte_power for VM Power Management(Guest).

Message ID 1411410879-28872-7-git-send-email-alan.carew@intel.com (mailing list archive)
State Superseded, archived
Headers

Commit Message

Alan Carew Sept. 22, 2014, 6:34 p.m. UTC
Re-using the host based librte_power API the alternate implementation uses
the guest channel API to forward request for frequency changes to the host
monitor.
A subset of the librte_power API is supported:
 rte_power_init(unsigned lcore_id)
 rte_power_exit(unsigned lcore_id)
 rte_power_freq_up(unsigned lcore_id)
 rte_power_freq_down(unsigned lcore_id)
 rte_power_freq_min(unsigned lcore_id)
 rte_power_freq_max(unsigned lcore_id)

The other unsupported APIs from librte_power return -ENOTSUP.

Signed-off-by: Alan Carew <alan.carew@intel.com>
---
 lib/librte_power_vm/Makefile    |  49 ++++++++++++++
 lib/librte_power_vm/rte_power.c | 146 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 195 insertions(+)
 create mode 100644 lib/librte_power_vm/Makefile
 create mode 100644 lib/librte_power_vm/rte_power.c
  

Comments

Neil Horman Sept. 22, 2014, 7:17 p.m. UTC | #1
On Mon, Sep 22, 2014 at 07:34:35PM +0100, Alan Carew wrote:
> Re-using the host based librte_power API the alternate implementation uses
> the guest channel API to forward request for frequency changes to the host
> monitor.
> A subset of the librte_power API is supported:
>  rte_power_init(unsigned lcore_id)
>  rte_power_exit(unsigned lcore_id)
>  rte_power_freq_up(unsigned lcore_id)
>  rte_power_freq_down(unsigned lcore_id)
>  rte_power_freq_min(unsigned lcore_id)
>  rte_power_freq_max(unsigned lcore_id)
> 
> The other unsupported APIs from librte_power return -ENOTSUP.
> 
> Signed-off-by: Alan Carew <alan.carew@intel.com>
> ---
>  lib/librte_power_vm/Makefile    |  49 ++++++++++++++
>  lib/librte_power_vm/rte_power.c | 146 ++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 195 insertions(+)
>  create mode 100644 lib/librte_power_vm/Makefile
>  create mode 100644 lib/librte_power_vm/rte_power.c
> 
NAK.
This is a bad design choice.  Creating an alternate library with all the same
symbols in place prevents an application from compiling in support for both host
and guest power management in parallel (i.e. if an app wants to be able to do
power management in either environment, and only gets built once, it won't
work).

In fact, linking a statically built library with both CONFIG_RTE_LIBRTE_POWER=y
and CONFIG_RTE_LIBRTE_POWER_VM=y yields the following link-time build break:

LD test
/home/nhorman/git/dpdk/build/lib/librte_power.a(guest_channel.o): In function
`guest_channel_host_connect':
guest_channel.c:(.text+0x0): multiple definition of `guest_channel_host_connect'
/home/nhorman/git/dpdk/build/lib/librte_power.a(guest_channel.o):guest_channel.c:(.text+0x0):
first defined here
/home/nhorman/git/dpdk/build/lib/librte_power.a(guest_channel.o): In function
`guest_channel_send_msg':
guest_channel.c:(.text+0x370): multiple definition of `guest_channel_send_msg'
....
Ad nauseum.

What you should do is merge this functionality in with the existing librte power
library, and make the choice of implementation a run time decision, so theres
only a single public facing API symbol set, and both implementations can
coexist, getting chosen at run time (via initialization config option,
environment detection, etc).  Konstantin and I had a simmilar discussion
regarding the ACL library and the use of the match function.  I think we came up
with some reasonably performant solutions.

Neil
  
Alan Carew Sept. 23, 2014, 7:48 a.m. UTC | #2
Hi Neil,


> -----Original Message-----
> From: Neil Horman [mailto:nhorman@tuxdriver.com]
> Sent: Monday, September 22, 2014 8:18 PM
> To: Carew, Alan
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH 06/10] Alternate implementation of
> librte_power for VM Power Management(Guest).
> 
> On Mon, Sep 22, 2014 at 07:34:35PM +0100, Alan Carew wrote:
> > Re-using the host based librte_power API the alternate implementation uses
> > the guest channel API to forward request for frequency changes to the host
> > monitor.
> > A subset of the librte_power API is supported:
> >  rte_power_init(unsigned lcore_id)
> >  rte_power_exit(unsigned lcore_id)
> >  rte_power_freq_up(unsigned lcore_id)
> >  rte_power_freq_down(unsigned lcore_id)
> >  rte_power_freq_min(unsigned lcore_id)
> >  rte_power_freq_max(unsigned lcore_id)
> >
> > The other unsupported APIs from librte_power return -ENOTSUP.
> >
> > Signed-off-by: Alan Carew <alan.carew@intel.com>
> > ---
> >  lib/librte_power_vm/Makefile    |  49 ++++++++++++++
> >  lib/librte_power_vm/rte_power.c | 146
> ++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 195 insertions(+)
> >  create mode 100644 lib/librte_power_vm/Makefile
> >  create mode 100644 lib/librte_power_vm/rte_power.c
> >
> NAK.
> This is a bad design choice.  Creating an alternate library with all the same
> symbols in place prevents an application from compiling in support for both host
> and guest power management in parallel (i.e. if an app wants to be able to do
> power management in either environment, and only gets built once, it won't
> work).
> 
> In fact, linking a statically built library with both CONFIG_RTE_LIBRTE_POWER=y
> and CONFIG_RTE_LIBRTE_POWER_VM=y yields the following link-time build
> break:
> 
> LD test
> /home/nhorman/git/dpdk/build/lib/librte_power.a(guest_channel.o): In
> function
> `guest_channel_host_connect':
> guest_channel.c:(.text+0x0): multiple definition of
> `guest_channel_host_connect'
> /home/nhorman/git/dpdk/build/lib/librte_power.a(guest_channel.o):guest_cha
> nnel.c:(.text+0x0):
> first defined here
> /home/nhorman/git/dpdk/build/lib/librte_power.a(guest_channel.o): In
> function
> `guest_channel_send_msg':
> guest_channel.c:(.text+0x370): multiple definition of `guest_channel_send_msg'
> ....
> Ad nauseum.
> 
> What you should do is merge this functionality in with the existing librte power
> library, and make the choice of implementation a run time decision, so theres
> only a single public facing API symbol set, and both implementations can
> coexist, getting chosen at run time (via initialization config option,
> environment detection, etc).  Konstantin and I had a simmilar discussion
> regarding the ACL library and the use of the match function.  I think we came up
> with some reasonably performant solutions.
> 
> Neil

Makes sense, I'll take a look at runtime configuration options and post a V2.

Thanks,
Alan
  

Patch

diff --git a/lib/librte_power_vm/Makefile b/lib/librte_power_vm/Makefile
new file mode 100644
index 0000000..284ec2c
--- /dev/null
+++ b/lib/librte_power_vm/Makefile
@@ -0,0 +1,49 @@ 
+#   BSD LICENSE
+#
+#   Copyright(c) 2010-2014 Intel Corporation. All rights reserved.
+#   All rights reserved.
+#
+#   Redistribution and use in source and binary forms, with or without
+#   modification, are permitted provided that the following conditions
+#   are met:
+#
+#     * Redistributions of source code must retain the above copyright
+#       notice, this list of conditions and the following disclaimer.
+#     * Redistributions in binary form must reproduce the above copyright
+#       notice, this list of conditions and the following disclaimer in
+#       the documentation and/or other materials provided with the
+#       distribution.
+#     * Neither the name of Intel Corporation nor the names of its
+#       contributors may be used to endorse or promote products derived
+#       from this software without specific prior written permission.
+#
+#   THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+#   "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+#   LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+#   A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+#   OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+#   SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+#   LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+#   DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+#   THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+#   (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+#   OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+
+include $(RTE_SDK)/mk/rte.vars.mk
+
+# library name
+LIB = librte_power.a
+
+CFLAGS += $(WERROR_FLAGS) -I$(SRCDIR) -O3 -fno-strict-aliasing
+CFLAGS += -I$(RTE_SDK)/lib/librte_power/
+
+# all source are stored in SRCS-y
+SRCS-$(CONFIG_RTE_LIBRTE_POWER_VM) := guest_channel.c rte_power.c
+
+# install this header file
+SYMLINK-y-include := ../librte_power/rte_power.h
+
+# this lib needs eal
+DEPDIRS-$(CONFIG_RTE_LIBRTE_POWER) += lib/librte_eal
+
+include $(RTE_SDK)/mk/rte.lib.mk
diff --git a/lib/librte_power_vm/rte_power.c b/lib/librte_power_vm/rte_power.c
new file mode 100644
index 0000000..1ce3fb0
--- /dev/null
+++ b/lib/librte_power_vm/rte_power.c
@@ -0,0 +1,146 @@ 
+/*-
+ *   BSD LICENSE
+ *
+ *   Copyright(c) 2010-2014 Intel Corporation. All rights reserved.
+ *   All rights reserved.
+ *
+ *   Redistribution and use in source and binary forms, with or without
+ *   modification, are permitted provided that the following conditions
+ *   are met:
+ *
+ *     * Redistributions of source code must retain the above copyright
+ *       notice, this list of conditions and the following disclaimer.
+ *     * Redistributions in binary form must reproduce the above copyright
+ *       notice, this list of conditions and the following disclaimer in
+ *       the documentation and/or other materials provided with the
+ *       distribution.
+ *     * Neither the name of Intel Corporation nor the names of its
+ *       contributors may be used to endorse or promote products derived
+ *       from this software without specific prior written permission.
+ *
+ *   THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+ *   "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+ *   LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+ *   A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+ *   OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ *   SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+ *   LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+ *   DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+ *   THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ *   (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+ *   OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+#include <errno.h>
+#include <string.h>
+
+#include <rte_log.h>
+#include <rte_config.h>
+
+#include "guest_channel.h"
+#include "channel_commands.h"
+#include "rte_power.h"
+
+#define RTE_LOGTYPE_POWER_VM RTE_LOGTYPE_USER1
+
+#define FD_PATH "/dev/virtio-ports/virtio.serial.port.poweragent"
+
+static struct channel_packet pkt[RTE_MAX_LCORE];
+
+
+int
+rte_power_init(unsigned lcore_id)
+{
+	pkt[lcore_id].command = CPU_POWER;
+	pkt[lcore_id].resource_id = lcore_id;
+	return guest_channel_host_connect(FD_PATH, lcore_id);
+}
+
+int
+rte_power_exit(unsigned lcore_id)
+{
+	guest_channel_host_disconnect(lcore_id);
+	return 0;
+}
+
+uint32_t
+rte_power_freqs(__attribute__((unused)) unsigned lcore_id,
+		__attribute__((unused)) uint32_t *freqs,
+		__attribute__((unused)) uint32_t num)
+{
+	RTE_LOG(ERR, POWER_VM, "rte_power_freqs is not implemented "
+			"for Virtual Machine Power Management\n");
+	return -ENOTSUP;
+}
+
+uint32_t
+rte_power_get_freq(__attribute__((unused)) unsigned lcore_id)
+{
+	RTE_LOG(ERR, POWER_VM, "rte_power_get_freq is not implemented "
+			"for Virtual Machine Power Management\n");
+	return -ENOTSUP;
+}
+
+int
+rte_power_set_freq(__attribute__((unused)) unsigned lcore_id,
+		__attribute__((unused)) uint32_t index)
+{
+	RTE_LOG(ERR, POWER_VM, "rte_power_set_freq is not implemented "
+			"for Virtual Machine Power Management\n");
+	return -ENOTSUP;
+}
+
+int
+rte_power_freq_up(unsigned lcore_id)
+{
+	int ret;
+	pkt[lcore_id].unit = CPU_SCALE_UP;
+
+	ret = guest_channel_send_msg(&pkt[lcore_id], lcore_id);
+	if (ret <= 0)
+		return ret;
+	if (ret > 0)
+		RTE_LOG(DEBUG, POWER_VM, "Error sending message: %s\n", strerror(ret));
+	return -1;
+}
+
+int
+rte_power_freq_down(__attribute__((unused)) unsigned lcore_id)
+{
+	int ret;
+	pkt[lcore_id].unit = CPU_SCALE_DOWN;
+
+	ret = guest_channel_send_msg(&pkt[lcore_id], lcore_id);
+	if (ret <= 0)
+		return ret;
+	if (ret > 0)
+		RTE_LOG(DEBUG, POWER_VM, "Error sending message: %s\n", strerror(ret));
+	return -1;
+}
+
+int
+rte_power_freq_max(__attribute__((unused)) unsigned lcore_id)
+{
+	int ret;
+	pkt[lcore_id].unit = CPU_SCALE_MAX;
+
+	ret = guest_channel_send_msg(&pkt[lcore_id], lcore_id);
+	if (ret <= 0)
+		return ret;
+	if (ret > 0)
+		RTE_LOG(DEBUG, POWER_VM, "Error sending message: %s\n", strerror(ret));
+	return -1;
+}
+
+int
+rte_power_freq_min(__attribute__((unused)) unsigned lcore_id)
+{
+	int ret;
+	pkt[lcore_id].unit = CPU_SCALE_MIN;
+
+	ret = guest_channel_send_msg(&pkt[lcore_id], lcore_id);
+	if (ret <= 0)
+		return ret;
+	if (ret > 0)
+		RTE_LOG(DEBUG, POWER_VM, "Error sending message: %s\n", strerror(ret));
+	return -1;
+}