From mboxrd@z Thu Jan 1 00:00:00 1970 From: Shreyansh Jain Subject: Re: [PATCH v1 2/8] bus: introduce opaque control framework Date: Tue, 12 Dec 2017 12:51:02 +0530 Message-ID: References: <91106540c460d22dd23a30dc2903d7e238ff9a3b.1507796085.git.gaetan.rivet@6wind.com> <088de7d2-9bd4-4a49-44ba-9df9c52b72d1@nxp.com> <20171211124359.zhyeaveywobmobef@bidouze.vm.6wind.com> <665f56dc-3cd5-e199-59b5-2021facd0bba@nxp.com> <20171211143819.hxgyhnxiiwlegjbw@bidouze.vm.6wind.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 8bit Cc: To: =?UTF-8?Q?Ga=c3=abtan_Rivet?= Return-path: Received: from NAM01-BY2-obe.outbound.protection.outlook.com (mail-by2nam01on0050.outbound.protection.outlook.com [104.47.34.50]) by dpdk.org (Postfix) with ESMTP id 40F831C00 for ; Tue, 12 Dec 2017 08:07:31 +0100 (CET) In-Reply-To: <20171211143819.hxgyhnxiiwlegjbw@bidouze.vm.6wind.com> Content-Language: en-US List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On Monday 11 December 2017 08:08 PM, Gaëtan Rivet wrote: > On Mon, Dec 11, 2017 at 07:06:55PM +0530, Shreyansh Jain wrote: >> On Monday 11 December 2017 06:13 PM, Gaëtan Rivet wrote: >>> On Mon, Dec 11, 2017 at 05:30:16PM +0530, Shreyansh Jain wrote: >>>> On Thursday 12 October 2017 01:48 PM, Gaetan Rivet wrote: >> >> [...] >> >>>>> diff --git a/lib/librte_eal/common/include/rte_bus.h b/lib/librte_eal/common/include/rte_bus.h >>>>> index 331d954..bd3c28e 100644 >>>>> --- a/lib/librte_eal/common/include/rte_bus.h >>>>> +++ b/lib/librte_eal/common/include/rte_bus.h >>>>> @@ -183,6 +183,51 @@ struct rte_bus_conf { >>>>> enum rte_bus_probe_mode probe_mode; /**< Probe policy. */ >>>>> }; >>>>> +/** >>>>> + * Bus configuration items. >>>>> + */ >>>>> +enum rte_bus_ctrl_item { >>>>> + RTE_BUS_CTRL_PROBE_MODE = 0, >>>>> + RTE_BUS_CTRL_ITEM_MAX, >>>>> +}; >>>> >>>> I am assuming that a driver implementation can take more than ITEM_MAX >>>> control knobs. It is opaque to the library. Are we on same page? >>>> >>>> For example, a bus driver can implement: >>>> >>>> rte_bus_XXX_ctrl_item { >>>> >>>> RTE_BUS_XYZ_KNOB_1 = 100, >>>> RTE_BUS_XYZ_KNOB_2, >>>> RTE_BUS_XYZ_KNOB_3, >>>> }; >>>> >>>> without the library knowing or restricting the API to RTE_BUS_CTRL_ITEM_MAX. >>>> >>>> I see that in your code for PCI (Patch 5/8: pci_ctrl) you have restricted >>>> the control knob to RTE_BUS_CTRL_ITEM_MAX. >>>> I hope that such restrictions would not float to library layer. >>>> >>>> If we are on same page, should this be documented as a code comment >>>> somewhere? >>>> if not, do you think what I am stating makes sense? >>>> >>> >>> I see what you mean, but I'm not sure it would be a good thing. >>> Actually, I think proposing this ITEM_MAX was a mistake. >>> >>> Regarding the specific bus knobs: >>> >>> - If a single bus needs this knob, then it would be better for the dev >>> to add it as part of the bus' public API, following the correct >>> library versioning processes. This would not break this bus control >>> structure ABI. >> >> Sorry, but can you elaborate on "...add it as part of bus' public API"? >> >> This is what I had in mind: >> >> ctrl_fn = rte_bus_control_get(busname, RTE_BUS_XYZ_KNOB_1); >> >> (unlike specific functions like probe_mode_get/set and iova_mode_get/set) >> >> Where ctrl_fn would then point to a method specific to bus for KNOB_1 >> configuration parameter. >> Thereafter, ctrl_fn(KNOB_1, void *arg). >> >> What other public API method are you hinting at? >> >> > > I was thinking that buses would simply expose a function > > rte_busname_xyz_knob1(void *arg); Yes, that is possible but only for cases where some very specific functionality needs to be exposed which is not expected to be generalized ever. > > as part of their public API. This would not require an ABI break for > this bus, as it would only be an API extension and would not use > callbacks within the bus structure. Yes, agree with your point. As such APIs are outside control of DPDK framework, they are something which will never impact the library layer. > > Thus, I think that for buses tempted to propose a specific API, this would be > the cleanest way. > > The bus proposing it as part of a custom control section would only be > interesting when the operation is expected to become a standard API for > other buses but was not yet accepted. Applications would be able to use > the interface and the ITEM could be added later. But I doubt this is > encouraging best practices as far as API evolution go. So, technically both are feasible: 1) having a bus specific API like rte_busname_xyz_knob1 and 2) expanding OPS with bus specific values and allowing application to use them. But, in either case, if the APIs can be generalized and/or can be used by multiple buses, they can definitely be moved into the library API (e.g. rte_bus_probe_mode_set) and/or can be added to rte_bus_ctrl_item. To summarize, I am OK with your approach. > >>> >>> - If more than one bus implement this knob, then it should be proposed >>> as part of the library API. Buses adding this new knob would break >>> their ABI, other buses would be left untouched. >> >> Agree, if more than one bus implements same operation. >> >>> >>> This makes me realize that proposing this ITEM_MAX value is not good to >>> the intended purpose of this patchset: >>> >>> - If a bus implementation use a reference to ITEM_MAX, then the control >>> structure ABI would be broken by any new control knob added, even if the >>> bus does not implement it. Granted, it would not break the driver >>> structure itself, but still. My PCI implementation is thus incorrect. >> >> Changes to enum wouldn't break ABI as far as I understand. Adding a new >> entry only expands it to a new declaration without impacting its size or >> signature. >> >>> >>> Therefore I think that it would be best to remove this ITEM_MAX altogether, >>> forcing bus developpers to use other ways that would not break their >>> ABIs every other release. >>> >> >> Removing ITEM_MAX is OK from my side. It doesn't serve much purpose. But, >> not for the "ABI break" reason. > > Adding the enum would not break ABI indeed, but I was thinking about the > way the bus control structure would be declared. > > However, upon second inspection on the my PCI implementation, I did not > actually use ITEM_MAX: > >> static rte_bus_ctrl_t pci_ctrl_ops[][RTE_BUS_CTRL_OP_MAX] = { >> » [RTE_BUS_CTRL_PROBE_MODE] = { >> » » [RTE_BUS_CTRL_GET] = pci_probe_mode_get, >> » » [RTE_BUS_CTRL_SET] = pci_probe_mode_set, >> » }, >> }; > > I just thought I had used RTE_BUS_CTRL_ITEM_MAX instead of RTE_BUS_CTRL_OP_MAX. > So my line of thought was simply that if any new item was declared, the control > structure would then change size. > > But I was mistaken, so that's actually not a problem :) > > Having ITEM_MAX available would still make those kind of mistakes > possible however. It might be better to prevent it completely by > removing it. This would however also prevent a custom control section. It might be a naive question, but, why do you think it would prevent a custom control section? Assuming that only RTE_BUS_CTRL_ITEM_MAX is not available (RTE_BUS_CTRL_OPS_MAX is available), Bus driver can still define: static rte_bus_ctrl_t pci_ctrl_ops[][RTE_BUS_CTRL_OP_MAX] = { [ITEM_1] = { {...}, {...}, }, [ITEM_2] = { {...}, {...}, }, [ITEM_NOT_IN_RTE_BUS.H] = { {...}, {...}, }, } (I know you disagree with third element of above definition - but somehow I feel it is a good addition for defining a knob which doesn't require an additional API call. Just assume as an example for now, please!) > > Do you think this would be useful enough to justify the slightly more > complex maintenance and review of bus implementations? > Having RTE_BUS_CTRL_ITEM_MAX is helpful if one has to iterate over all ctrl_items - but, that might never be required in my perception. Other than that, I don't have a strong reason to say that ITEM_MAX is required. Though, same is not true for OP_MAX - which is required for definitions like above.