Message ID | 20210108143048.23755-1-david.hunt@intel.com (mailing list archive) |
---|---|
Headers |
Return-Path: <dev-bounces@dpdk.org> X-Original-To: patchwork@inbox.dpdk.org Delivered-To: patchwork@inbox.dpdk.org Received: from mails.dpdk.org (xvm-189-124.dc0.ghst.net [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id EAD75A0524; Fri, 8 Jan 2021 15:30:56 +0100 (CET) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id B9225140F07; Fri, 8 Jan 2021 15:30:56 +0100 (CET) Received: from mga05.intel.com (mga05.intel.com [192.55.52.43]) by mails.dpdk.org (Postfix) with ESMTP id BF36A140EE2; Fri, 8 Jan 2021 15:30:54 +0100 (CET) IronPort-SDR: ksBEAUEMSmK+uG7/CJKprmq8DSBiHCRGgC/KuPHPf1SZFwzgr6GaEn+sTGeM09kcS3RwtmFemw 589HEMc7L6gw== X-IronPort-AV: E=McAfee;i="6000,8403,9857"; a="262374152" X-IronPort-AV: E=Sophos;i="5.79,331,1602572400"; d="scan'208";a="262374152" Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by fmsmga105.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 08 Jan 2021 06:30:53 -0800 IronPort-SDR: WBpB75Oa2JzQcSpEWD5sRjIi9fbeuuF2m6LDjxsRZAli5O1ZBC+QEqjworKc6ccNucbQ0Nptxd /MsoNcUh71SQ== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.79,331,1602572400"; d="scan'208";a="403371385" Received: from silpixa00399952.ir.intel.com (HELO silpixa00399952.ger.corp.intel.com) ([10.237.222.38]) by FMSMGA003.fm.intel.com with ESMTP; 08 Jan 2021 06:30:52 -0800 From: David Hunt <david.hunt@intel.com> To: dev@dpdk.org Cc: david.hunt@intel.com, stable@dpdk.org Date: Fri, 8 Jan 2021 14:30:42 +0000 Message-Id: <20210108143048.23755-1-david.hunt@intel.com> X-Mailer: git-send-email 2.17.1 In-Reply-To: <20201217113656.28884-1-david.hunt@intel.com> References: <20201217113656.28884-1-david.hunt@intel.com> Subject: [dpdk-dev] [PATCH 0/6] power: fix make build for power apps X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 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> |
Series |
power: fix make build for power apps
|
|
Message
Hunt, David
Jan. 8, 2021, 2:30 p.m. UTC
The power example applications that uses the virtio-serial method of communication cannot currently be built with make, and can only be built using meson/ninja. The guest channel message definitions and functions in guest_channel.h are needed by applications and need to be made public. This worked pre-20.11, but now with all the meson/ninja changes, making these apps externally no longer works. To fix, we need to move the header file with the API definitions for the channel commands public, and rename the functions accordingly. The main change is to rename channel_commands.h to rte_power_guest_channel.h so that it gets picked up by the installer and copied to /usr/local/include. Other changes include renaming #defines to have RTE_ at the beginning instead of CPU_. Finally we refactor the code to work with those changes. --- v2 changes - re-worked from monolithic patch to a 6 patch patchset for easier review [PATCH v2 1/6] power: create guest channel public header file [PATCH v2 2/6] power: make channel msg functions public [PATCH v2 3/6] power: rename public structs [PATCH v2 4/6] power: rename defines [PATCH v2 5/6] power: add new header file to export list [PATCH v2 6/6] power: clean up includes
Comments
On 08-Jan-21 2:30 PM, David Hunt wrote: > The power example applications that uses the virtio-serial method of > communication cannot currently be built with make, and can only be built > using meson/ninja. > > The guest channel message definitions and functions in guest_channel.h > are needed by applications and need to be made public. > > This worked pre-20.11, but now with all the meson/ninja changes, making > these apps externally no longer works. To fix, we need to move the header > file with the API definitions for the channel commands public, and rename > the functions accordingly. > > The main change is to rename channel_commands.h to > rte_power_guest_channel.h so that it gets picked up by the installer and > copied to /usr/local/include. Other changes include renaming #defines to > have RTE_ at the beginning instead of CPU_. Finally we refactor the code > to work with those changes. > > --- > v2 changes > - re-worked from monolithic patch to a 6 patch patchset for easier review > > [PATCH v2 1/6] power: create guest channel public header file > [PATCH v2 2/6] power: make channel msg functions public > [PATCH v2 3/6] power: rename public structs > [PATCH v2 4/6] power: rename defines > [PATCH v2 5/6] power: add new header file to export list > [PATCH v2 6/6] power: clean up includes > Just a general question: wouldn't it be better to move this stuff entirely into sample app and not bother with keeping it in the library? There is precedent - ethtool app has a "library" and an "application" part, i think you should be able to move it out of the library and into the sample app entirely without too much trouble, as code looks to be fairly self-contained.
Hi Anatoly, On 13/1/2021 11:08 AM, Burakov, Anatoly wrote: > On 08-Jan-21 2:30 PM, David Hunt wrote: >> The power example applications that uses the virtio-serial method of >> communication cannot currently be built with make, and can only be built >> using meson/ninja. >> >> The guest channel message definitions and functions in guest_channel.h >> are needed by applications and need to be made public. >> >> This worked pre-20.11, but now with all the meson/ninja changes, making >> these apps externally no longer works. To fix, we need to move the >> header >> file with the API definitions for the channel commands public, and >> rename >> the functions accordingly. >> >> The main change is to rename channel_commands.h to >> rte_power_guest_channel.h so that it gets picked up by the installer and >> copied to /usr/local/include. Other changes include renaming #defines to >> have RTE_ at the beginning instead of CPU_. Finally we refactor the code >> to work with those changes. >> >> --- >> v2 changes >> - re-worked from monolithic patch to a 6 patch patchset for easier >> review >> >> [PATCH v2 1/6] power: create guest channel public header file >> [PATCH v2 2/6] power: make channel msg functions public >> [PATCH v2 3/6] power: rename public structs >> [PATCH v2 4/6] power: rename defines >> [PATCH v2 5/6] power: add new header file to export list >> [PATCH v2 6/6] power: clean up includes >> > > Just a general question: wouldn't it be better to move this stuff > entirely into sample app and not bother with keeping it in the > library? There is precedent - ethtool app has a "library" and an > "application" part, i think you should be able to move it out of the > library and into the sample app entirely without too much trouble, as > code looks to be fairly self-contained. > Agreed, that's a great idea. I could have a common lib under examples/vm_power_manager, then two apps, vm_power_manager and guest_cli. That would keep everything nicely local, and not expose the channel API publicly. The only reason we were making it public was to allow "make" to work, so that's not a good enought reason, tbh. I'll throw a prototype together today. Thanks, Dave.
On 13-Jan-21 11:14 AM, David Hunt wrote: > Hi Anatoly, > > On 13/1/2021 11:08 AM, Burakov, Anatoly wrote: >> On 08-Jan-21 2:30 PM, David Hunt wrote: >>> The power example applications that uses the virtio-serial method of >>> communication cannot currently be built with make, and can only be built >>> using meson/ninja. >>> >>> The guest channel message definitions and functions in guest_channel.h >>> are needed by applications and need to be made public. >>> >>> This worked pre-20.11, but now with all the meson/ninja changes, making >>> these apps externally no longer works. To fix, we need to move the >>> header >>> file with the API definitions for the channel commands public, and >>> rename >>> the functions accordingly. >>> >>> The main change is to rename channel_commands.h to >>> rte_power_guest_channel.h so that it gets picked up by the installer and >>> copied to /usr/local/include. Other changes include renaming #defines to >>> have RTE_ at the beginning instead of CPU_. Finally we refactor the code >>> to work with those changes. >>> >>> --- >>> v2 changes >>> - re-worked from monolithic patch to a 6 patch patchset for easier >>> review >>> >>> [PATCH v2 1/6] power: create guest channel public header file >>> [PATCH v2 2/6] power: make channel msg functions public >>> [PATCH v2 3/6] power: rename public structs >>> [PATCH v2 4/6] power: rename defines >>> [PATCH v2 5/6] power: add new header file to export list >>> [PATCH v2 6/6] power: clean up includes >>> >> >> Just a general question: wouldn't it be better to move this stuff >> entirely into sample app and not bother with keeping it in the >> library? There is precedent - ethtool app has a "library" and an >> "application" part, i think you should be able to move it out of the >> library and into the sample app entirely without too much trouble, as >> code looks to be fairly self-contained. >> > > Agreed, that's a great idea. I could have a common lib under > examples/vm_power_manager, then two apps, vm_power_manager and > guest_cli. That would keep everything nicely local, and not expose the > channel API publicly. The only reason we were making it public was to > allow "make" to work, so that's not a good enought reason, tbh. I'll > throw a prototype together today. Yep, IIRC Make works perfectly fine with ethtool, so i don't see why it wouldn't work for your sample app as well. Thanks! > > Thanks, > Dave. > > > >
On 13/1/2021 11:18 AM, Burakov, Anatoly wrote: > On 13-Jan-21 11:14 AM, David Hunt wrote: >> Hi Anatoly, >> >> On 13/1/2021 11:08 AM, Burakov, Anatoly wrote: >>> On 08-Jan-21 2:30 PM, David Hunt wrote: >>>> The power example applications that uses the virtio-serial method of >>>> communication cannot currently be built with make, and can only be >>>> built >>>> using meson/ninja. >>>> >>>> The guest channel message definitions and functions in guest_channel.h >>>> are needed by applications and need to be made public. >>>> >>>> This worked pre-20.11, but now with all the meson/ninja changes, >>>> making >>>> these apps externally no longer works. To fix, we need to move the >>>> header >>>> file with the API definitions for the channel commands public, and >>>> rename >>>> the functions accordingly. >>>> >>>> The main change is to rename channel_commands.h to >>>> rte_power_guest_channel.h so that it gets picked up by the >>>> installer and >>>> copied to /usr/local/include. Other changes include renaming >>>> #defines to >>>> have RTE_ at the beginning instead of CPU_. Finally we refactor the >>>> code >>>> to work with those changes. >>>> >>>> --- >>>> v2 changes >>>> - re-worked from monolithic patch to a 6 patch patchset for >>>> easier review >>>> >>>> [PATCH v2 1/6] power: create guest channel public header file >>>> [PATCH v2 2/6] power: make channel msg functions public >>>> [PATCH v2 3/6] power: rename public structs >>>> [PATCH v2 4/6] power: rename defines >>>> [PATCH v2 5/6] power: add new header file to export list >>>> [PATCH v2 6/6] power: clean up includes >>>> >>> >>> Just a general question: wouldn't it be better to move this stuff >>> entirely into sample app and not bother with keeping it in the >>> library? There is precedent - ethtool app has a "library" and an >>> "application" part, i think you should be able to move it out of the >>> library and into the sample app entirely without too much trouble, >>> as code looks to be fairly self-contained. >>> >> >> Agreed, that's a great idea. I could have a common lib under >> examples/vm_power_manager, then two apps, vm_power_manager and >> guest_cli. That would keep everything nicely local, and not expose >> the channel API publicly. The only reason we were making it public >> was to allow "make" to work, so that's not a good enought reason, >> tbh. I'll throw a prototype together today. > > Yep, IIRC Make works perfectly fine with ethtool, so i don't see why > it wouldn't work for your sample app as well. Thanks! Hi Anatoly, OK, so I was investigating this, and have come across a blocker on this method. There are three methods for managing frequency, acpi, pstate and vm. It's the third method that's causing the problem with moving the channel functionality out into a sample library alongside vm_power_manger. VM's can call channel functions in the power library to affect frequency for their cores, and these functions use api function calls and several structures and #defines in their code, which is currently part of the power management library. These function declarations, structs and #defines ere needed in both the examples lib/apps and the power library itself, and the prototype changes I made ended up looking very much like the patch set that's already on the mailing list. So, while I would have liked to have a solution along the lines of what you've proposed, it's not possible based on the dependencies on common structues and #defines. Thanks for the suggestion, though. Rgds, Dave.
On 13-Jan-21 1:25 PM, David Hunt wrote: > > On 13/1/2021 11:18 AM, Burakov, Anatoly wrote: >> On 13-Jan-21 11:14 AM, David Hunt wrote: >>> Hi Anatoly, >>> >>> On 13/1/2021 11:08 AM, Burakov, Anatoly wrote: >>>> On 08-Jan-21 2:30 PM, David Hunt wrote: >>>>> The power example applications that uses the virtio-serial method of >>>>> communication cannot currently be built with make, and can only be >>>>> built >>>>> using meson/ninja. >>>>> >>>>> The guest channel message definitions and functions in guest_channel.h >>>>> are needed by applications and need to be made public. >>>>> >>>>> This worked pre-20.11, but now with all the meson/ninja changes, >>>>> making >>>>> these apps externally no longer works. To fix, we need to move the >>>>> header >>>>> file with the API definitions for the channel commands public, and >>>>> rename >>>>> the functions accordingly. >>>>> >>>>> The main change is to rename channel_commands.h to >>>>> rte_power_guest_channel.h so that it gets picked up by the >>>>> installer and >>>>> copied to /usr/local/include. Other changes include renaming >>>>> #defines to >>>>> have RTE_ at the beginning instead of CPU_. Finally we refactor the >>>>> code >>>>> to work with those changes. >>>>> >>>>> --- >>>>> v2 changes >>>>> - re-worked from monolithic patch to a 6 patch patchset for >>>>> easier review >>>>> >>>>> [PATCH v2 1/6] power: create guest channel public header file >>>>> [PATCH v2 2/6] power: make channel msg functions public >>>>> [PATCH v2 3/6] power: rename public structs >>>>> [PATCH v2 4/6] power: rename defines >>>>> [PATCH v2 5/6] power: add new header file to export list >>>>> [PATCH v2 6/6] power: clean up includes >>>>> >>>> >>>> Just a general question: wouldn't it be better to move this stuff >>>> entirely into sample app and not bother with keeping it in the >>>> library? There is precedent - ethtool app has a "library" and an >>>> "application" part, i think you should be able to move it out of the >>>> library and into the sample app entirely without too much trouble, >>>> as code looks to be fairly self-contained. >>>> >>> >>> Agreed, that's a great idea. I could have a common lib under >>> examples/vm_power_manager, then two apps, vm_power_manager and >>> guest_cli. That would keep everything nicely local, and not expose >>> the channel API publicly. The only reason we were making it public >>> was to allow "make" to work, so that's not a good enought reason, >>> tbh. I'll throw a prototype together today. >> >> Yep, IIRC Make works perfectly fine with ethtool, so i don't see why >> it wouldn't work for your sample app as well. Thanks! > > > Hi Anatoly, > > OK, so I was investigating this, and have come across a blocker on this > method. > > There are three methods for managing frequency, acpi, pstate and vm. > It's the third method that's causing the problem with moving the channel > functionality out into a sample library alongside vm_power_manger. VM's > can call channel functions in the power library to affect frequency for > their cores, and these functions use api function calls and several > structures and #defines in their code, which is currently part of the > power management library. These function declarations, structs and > #defines ere needed in both the examples lib/apps and the power library > itself, and the prototype changes I made ended up looking very much like > the patch set that's already on the mailing list. > > So, while I would have liked to have a solution along the lines of what > you've proposed, it's not possible based on the dependencies on common > structues and #defines. > > Thanks for the suggestion, though. > > Rgds, > Dave. > OK, i guess we can live with that. I wonder if there's another way to do this, but since i can't think of anything that doesn't involve serious API/ABI breakages, this is OK IMO.