From: "Wiles, Keith" <keith.wiles@intel.com>
To: Thomas Monjalon <thomas.monjalon@6wind.com>
Cc: "Dumitrescu, Cristian" <cristian.dumitrescu@intel.com>,
DPDK <dev@dpdk.org>,
"jerin.jacob@caviumnetworks.com" <jerin.jacob@caviumnetworks.com>,
"balasubramanian.manoharan@cavium.com"
<balasubramanian.manoharan@cavium.com>,
"hemant.agrawal@nxp.com" <hemant.agrawal@nxp.com>,
"shreyansh.jain@nxp.com" <shreyansh.jain@nxp.com>,
"Richardson, Bruce" <bruce.richardson@intel.com>
Subject: Re: [dpdk-dev] [PATCH v3 1/2] ethdev: add capability control API
Date: Mon, 6 Mar 2017 20:41:27 +0000 [thread overview]
Message-ID: <77383876-70CD-4F81-B179-B95ED52933D6@intel.com> (raw)
In-Reply-To: <10140076.z0k8vql8dv@xps13>
> On Mar 6, 2017, at 2:21 PM, Thomas Monjalon <thomas.monjalon@6wind.com> wrote:
>
>> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
>>> 2017-03-06 16:35, Dumitrescu, Cristian:
>>>>>> +int rte_eth_dev_capability_ops_get(uint8_t port_id,
>>>>>> + enum rte_eth_capability cap, void *arg);
>>>>>
>>>>> What is the benefit of getting different kind of capabilities with
>>>>> the same function?
>>>>> enum + void* = ioctl
>>>>> A self-explanatory API should have a dedicated function for each kind
>>>>> of features with different argument types.
>>>>
>>>> The advantage is providing a standard interface to query the capabilities of
>>> the device rather than having each capability provide its own mechanism in a
>>> slightly different way.
>>>>
>>>> IMO this mechanism is of great help to guide the developers of future
>>> ethdev features on the clean path to add new features in a modular way,
>>> extending the ethdev functionality while doing so in a separate name space
>>> and file (that's why I tend to call this a plugin-like mechanism), as opposed to
>>> the current monolithic approach for ethdev, where we have 100+ API
>>> functions in a single name space and that are split into functional groups just
>>> by blank lines in the header file. It is simply the generalization of the
>>> mechanism introduced by rte_flow in release 17.02 (so all the credit should
>>> go to Adrien and not me).
>>>>
>>>> IMO, having a standard function as above it cleaner than having a separate
>>> and slightly different function per feature. People can quickly see the set of
>>> standard ethdev capabilities and which ones are supported by a specific
>>> device. Between A) and B) below, I definitely prefer A):
>>>> A) status = rte_eth_dev_capability_ops_get(port_id,
>>> RTE_ETH_CABABILITY_TM, &tm_ops);
>>>> B) status = rte_eth_dev_tm_ops_get(port_id, &tm_ops);
>>>
>>> I prefer B because instead of tm_ops, you can use some specific tm
>>> arguments,
>>> show their types and properly document each parameter.
>>
>> Note that rte_flow already returns the flow ops as a void * with no strong argument type checking (approach A from above). Are you saying this is wrong?
>>
>> rte_eth_dev_filter_ctrl(port_id, RTE_ETH_FILTER_GENERIC, RTE_ETH_FILTER_GET, void *eth_flow_ops);
>>
>> Personally, I am in favour of allowing the standard interface at the expense of strong build-time type checking. Especially that this API function is between ethdev and the drivers, as opposed to between app and ethdev.
>
> rte_eth_dev_filter_ctrl is going to be specialized in rte_flow operations.
> I agree with you on having independent API blocks in ethdev like rte_flow.
> But this function rte_eth_dev_capability_ops_get that you propose would be
> cross-blocks. I don't see the benefit.
> I especially don't think there is a sense in the enum
> enum rte_eth_capability {
> RTE_ETH_CAPABILITY_FLOW = 0, /**< Flow */
> RTE_ETH_CAPABILITY_TM, /**< Traffic Manager */
> RTE_ETH_CAPABILITY_MAX
> }
>
> I won't debate more on this. We have to read opinions of other reviewers.
The benefit is providing a generic API, which we do not need to alter in the future (causing ABI breakage). The PMD can add a capability to the list if not present already and then provide a API structure for the feature.
Being able to add features without having to change DPDK maybe a strong feature for companies that have special needs for its application. They just need to add a rte_eth_capability enum in a range that they want to control (which does not mean they need to change the above structure) and they can provide private features to the application especially if they are very specific features to some HW. I do not like private features, but I also do not want to stick just any old API in DPDK for any given special feature.
Today the structure is just APIs, but it could also provide some special or specific information to the application in that structure or via an API call.
Regards,
Keith
next prev parent reply other threads:[~2017-03-06 20:41 UTC|newest]
Thread overview: 52+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-03-04 1:10 [dpdk-dev] [PATCH v3 0/2] ethdev: abstraction layer for QoS hierarchical scheduler Cristian Dumitrescu
2017-03-04 1:10 ` [dpdk-dev] [PATCH v3 1/2] ethdev: add capability control API Cristian Dumitrescu
2017-03-06 10:32 ` Thomas Monjalon
2017-03-06 16:35 ` Dumitrescu, Cristian
2017-03-06 16:57 ` Thomas Monjalon
2017-03-06 18:28 ` Dumitrescu, Cristian
2017-03-06 20:21 ` Thomas Monjalon
2017-03-06 20:41 ` Wiles, Keith [this message]
2017-03-06 20:54 ` Stephen Hemminger
2017-03-07 10:14 ` Dumitrescu, Cristian
2017-03-07 12:56 ` Thomas Monjalon
2017-03-07 19:17 ` Wiles, Keith
2017-03-06 16:36 ` Dumitrescu, Cristian
2017-05-19 17:12 ` [dpdk-dev] [PATCH v4 0/2] ethdev: abstraction layer for QoS traffic management Cristian Dumitrescu
2017-05-19 17:12 ` [dpdk-dev] [PATCH v4 1/2] ethdev: add traffic management ops get API Cristian Dumitrescu
2017-06-09 16:51 ` [dpdk-dev] [PATCH v5 0/2] ethdev: abstraction layer for QoS traffic management Cristian Dumitrescu
2017-06-09 16:51 ` [dpdk-dev] [PATCH v5 1/2] ethdev: add traffic management ops get API Cristian Dumitrescu
2017-06-09 16:51 ` [dpdk-dev] [PATCH v5 2/2] ethdev: add traffic management API Cristian Dumitrescu
2017-06-12 3:36 ` Jerin Jacob
2017-06-12 10:24 ` Dumitrescu, Cristian
2017-06-12 13:35 ` [dpdk-dev] [PATCH v6 0/2] ethdev: abstraction layer for QoS traffic management Cristian Dumitrescu
2017-06-12 13:35 ` [dpdk-dev] [PATCH v6 1/2] ethdev: add traffic management ops get API Cristian Dumitrescu
2017-06-12 13:35 ` [dpdk-dev] [PATCH v6 2/2] ethdev: add traffic management API Cristian Dumitrescu
2017-06-27 13:24 ` [dpdk-dev] [PATCH v6 0/2] ethdev: abstraction layer for QoS traffic management Dumitrescu, Cristian
2017-05-19 17:12 ` [dpdk-dev] [PATCH v4 2/2] ethdev: add traffic management API Cristian Dumitrescu
2017-05-19 17:34 ` Stephen Hemminger
2017-05-22 14:25 ` Dumitrescu, Cristian
2017-05-24 11:28 ` Hemant Agrawal
2017-05-31 13:45 ` Jerin Jacob
2017-05-31 17:05 ` Manoharan, Balasubramanian
2017-03-04 1:10 ` [dpdk-dev] [PATCH v3 2/2] ethdev: add hierarchical scheduler API Cristian Dumitrescu
2017-03-06 10:38 ` Thomas Monjalon
2017-03-06 16:59 ` Dumitrescu, Cristian
2017-03-06 20:07 ` Thomas Monjalon
2017-03-07 19:29 ` Dumitrescu, Cristian
2017-03-08 9:51 ` O'Driscoll, Tim
2017-03-10 18:37 ` Dumitrescu, Cristian
2017-03-15 12:43 ` Thomas Monjalon
2017-03-16 16:23 ` Dumitrescu, Cristian
2017-03-16 17:29 ` Thomas Monjalon
2017-03-16 17:40 ` Dumitrescu, Cristian
2017-03-16 18:10 ` Thomas Monjalon
2017-03-16 19:06 ` Dumitrescu, Cristian
2017-03-24 19:55 ` Dumitrescu, Cristian
2017-03-06 16:15 ` Stephen Hemminger
2017-03-06 18:17 ` Dumitrescu, Cristian
2017-03-16 17:35 ` Thomas Monjalon
2017-03-30 10:32 ` Hemant Agrawal
2017-04-07 16:51 ` Dumitrescu, Cristian
2017-04-07 13:20 ` Jerin Jacob
2017-04-07 17:47 ` Dumitrescu, Cristian
2017-04-10 14:00 ` Jerin Jacob
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=77383876-70CD-4F81-B179-B95ED52933D6@intel.com \
--to=keith.wiles@intel.com \
--cc=balasubramanian.manoharan@cavium.com \
--cc=bruce.richardson@intel.com \
--cc=cristian.dumitrescu@intel.com \
--cc=dev@dpdk.org \
--cc=hemant.agrawal@nxp.com \
--cc=jerin.jacob@caviumnetworks.com \
--cc=shreyansh.jain@nxp.com \
--cc=thomas.monjalon@6wind.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).