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

[Neil Horman <nhorman@tuxdriver.com>]

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