[dpdk-dev,v3,1/2] ethdev: add capability control API

  The rte_flow feature breaks the current monolithic approach for ethdev and
introduces the new generic flow API to ethdev using a plugin-like approach.

Basically, the rte_flow API is still logically part of ethdev:
- It extends the ethdev functionality: rte_flow is a new feature/capability
  of ethdev;
- all its functions work on an Ethernet device: the first parameter of the
  rte_flow functions is Ethernet device port ID.

At the same time, the rte_flow API is a sort of capability plugin for ethdev:
- the rte_flow API functions have their own name space: they are called
  rte_flow_operationXYZ() as opposed to rte_eth_dev_flow_operationXYZ());
- the rte_flow API functions are placed in separate files in the same
  librte_ether folder as opposed to rte_ethdev.[hc].

The way it works is by using the existing ethdev API function
rte_eth_dev_filter_ctrl() to query the current Ethernet device port ID for the
support of the rte_flow capability and return the pointer to the
rte_flow operations when supported and NULL otherwise:

struct rte_flow_ops *eth_flow_ops;
int rte = rte_eth_dev_filter_ctrl(eth_port_id,
	RTE_ETH_FILTER_GENERIC, RTE_ETH_FILTER_GET, &eth_flow_ops);

Unfortunately, the rte_flow opportunistically uses the rte_eth_dev_filter_ctrl()
API function, which is applicable just to RX-side filters as opposed to
introducing a mechanism that could be used by any capability in a generic way.

This is the gap that addressed by the current patch. This mechanism is intended
to be used to introduce new capabilities into ethdev in a modular plugin-like
approach, such as hierarchical scheduler. Over time, if agreed, it can also be
used for exposing the existing Ethernet device capabilities in a modular way,
such as: xstats, filters, multicast, mirroring, tunnels, time stamping, eeprom,
bypass, etc.

Changes in v3:
-Followed up on suggestion from Jerin: renamed capability from Hierarchical
 Scheduler (sched) to Traffic Manager (tm)

Changes in v2:
-Followed up on suggestion from Jerin and Hemant: renamed capability_control()
 to capability_ops_get()
-Added ACK from Keith, Jerin and Hemant

Signed-off-by: Cristian Dumitrescu <cristian.dumitrescu@intel.com>
Acked-by: Keith Wiles <keith.wiles@intel.com>
Acked-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
Acked-by: Hemant Agrawal <hemant.agrawal@nxp.com>
---
 lib/librte_ether/rte_ethdev.c          | 13 +++++++++++++
 lib/librte_ether/rte_ethdev.h          | 29 +++++++++++++++++++++++++++++
 lib/librte_ether/rte_ether_version.map |  7 +++++++
 3 files changed, 49 insertions(+)
  

Hi Cristian,

2017-03-04 01:10, Cristian Dumitrescu:
> struct rte_flow_ops *eth_flow_ops;
> int rte = rte_eth_dev_filter_ctrl(eth_port_id,
> 	RTE_ETH_FILTER_GENERIC, RTE_ETH_FILTER_GET, &eth_flow_ops);
> 
> Unfortunately, the rte_flow opportunistically uses the rte_eth_dev_filter_ctrl()
> API function, which is applicable just to RX-side filters as opposed to
> introducing a mechanism that could be used by any capability in a generic way.
> 
> This is the gap that addressed by the current patch. This mechanism is intended
> to be used to introduce new capabilities into ethdev in a modular plugin-like
> approach, such as hierarchical scheduler. Over time, if agreed, it can also be
> used for exposing the existing Ethernet device capabilities in a modular way,
> such as: xstats, filters, multicast, mirroring, tunnels, time stamping, eeprom,
> bypass, etc.
> 
> Changes in v3:
> -Followed up on suggestion from Jerin: renamed capability from Hierarchical
>  Scheduler (sched) to Traffic Manager (tm)
> 
> Changes in v2:
> -Followed up on suggestion from Jerin and Hemant: renamed capability_control()
>  to capability_ops_get()
> -Added ACK from Keith, Jerin and Hemant

It is difficult to follow previous discussions as you do not
keep threading with --in-reply-to.

> Signed-off-by: Cristian Dumitrescu <cristian.dumitrescu@intel.com>
> Acked-by: Keith Wiles <keith.wiles@intel.com>
> Acked-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> Acked-by: Hemant Agrawal <hemant.agrawal@nxp.com>
[...]
> +enum rte_eth_capability {
> +	RTE_ETH_CAPABILITY_FLOW = 0, /**< Flow */
> +	RTE_ETH_CAPABILITY_TM, /**< Traffic Manager */
> +	RTE_ETH_CAPABILITY_MAX
> +};
[...]
>  /**
> + * Take capability operations on an Ethernet device.
> + *
> + * @param port_id
> + *   The port identifier of the Ethernet device.
> + * @param cap
> + *   The capability of the Ethernet device
> + * @param arg
> + *   A pointer to arguments defined specifically for the operation.
> + * @return
> + *   - (0) if successful.
> + *   - (-ENOTSUP) if hardware doesn't support.
> + *   - (-ENODEV) if *port_id* invalid.
> + */
> +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.
  

Hi Thomas,

Thanks for reviewing this proposal.


> > Signed-off-by: Cristian Dumitrescu <cristian.dumitrescu@intel.com>
> > Acked-by: Keith Wiles <keith.wiles@intel.com>
> > Acked-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> > Acked-by: Hemant Agrawal <hemant.agrawal@nxp.com>
> [...]
> > +enum rte_eth_capability {
> > +	RTE_ETH_CAPABILITY_FLOW = 0, /**< Flow */
> > +	RTE_ETH_CAPABILITY_TM, /**< Traffic Manager */
> > +	RTE_ETH_CAPABILITY_MAX
> > +};
> [...]
> >  /**
> > + * Take capability operations on an Ethernet device.
> > + *
> > + * @param port_id
> > + *   The port identifier of the Ethernet device.
> > + * @param cap
> > + *   The capability of the Ethernet device
> > + * @param arg
> > + *   A pointer to arguments defined specifically for the operation.
> > + * @return
> > + *   - (0) if successful.
> > + *   - (-ENOTSUP) if hardware doesn't support.
> > + *   - (-ENODEV) if *port_id* invalid.
> > + */
> > +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);

Regards,
Cristian
  

> 
> It is difficult to follow previous discussions as you do not
> keep threading with --in-reply-to.
> 

Apologies, will do in the future.
  

2017-03-06 16:35, Dumitrescu, Cristian:
> Hi Thomas,
> 
> Thanks for reviewing this proposal.
> 
> 
> > > Signed-off-by: Cristian Dumitrescu <cristian.dumitrescu@intel.com>
> > > Acked-by: Keith Wiles <keith.wiles@intel.com>
> > > Acked-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> > > Acked-by: Hemant Agrawal <hemant.agrawal@nxp.com>
> > [...]
> > > +enum rte_eth_capability {
> > > +	RTE_ETH_CAPABILITY_FLOW = 0, /**< Flow */
> > > +	RTE_ETH_CAPABILITY_TM, /**< Traffic Manager */
> > > +	RTE_ETH_CAPABILITY_MAX
> > > +};
> > [...]
> > >  /**
> > > + * Take capability operations on an Ethernet device.
> > > + *
> > > + * @param port_id
> > > + *   The port identifier of the Ethernet device.
> > > + * @param cap
> > > + *   The capability of the Ethernet device
> > > + * @param arg
> > > + *   A pointer to arguments defined specifically for the operation.
> > > + * @return
> > > + *   - (0) if successful.
> > > + *   - (-ENOTSUP) if hardware doesn't support.
> > > + *   - (-ENODEV) if *port_id* invalid.
> > > + */
> > > +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.
  

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Monday, March 6, 2017 4:57 PM
> To: Dumitrescu, Cristian <cristian.dumitrescu@intel.com>
> Cc: dev@dpdk.org; jerin.jacob@caviumnetworks.com;
> balasubramanian.manoharan@cavium.com; hemant.agrawal@nxp.com;
> shreyansh.jain@nxp.com; Wiles, Keith <keith.wiles@intel.com>; Richardson,
> Bruce <bruce.richardson@intel.com>
> Subject: Re: [PATCH v3 1/2] ethdev: add capability control API
> 
> 2017-03-06 16:35, Dumitrescu, Cristian:
> > Hi Thomas,
> >
> > Thanks for reviewing this proposal.
> >
> >
> > > > Signed-off-by: Cristian Dumitrescu <cristian.dumitrescu@intel.com>
> > > > Acked-by: Keith Wiles <keith.wiles@intel.com>
> > > > Acked-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> > > > Acked-by: Hemant Agrawal <hemant.agrawal@nxp.com>
> > > [...]
> > > > +enum rte_eth_capability {
> > > > +	RTE_ETH_CAPABILITY_FLOW = 0, /**< Flow */
> > > > +	RTE_ETH_CAPABILITY_TM, /**< Traffic Manager */
> > > > +	RTE_ETH_CAPABILITY_MAX
> > > > +};
> > > [...]
> > > >  /**
> > > > + * Take capability operations on an Ethernet device.
> > > > + *
> > > > + * @param port_id
> > > > + *   The port identifier of the Ethernet device.
> > > > + * @param cap
> > > > + *   The capability of the Ethernet device
> > > > + * @param arg
> > > > + *   A pointer to arguments defined specifically for the operation.
> > > > + * @return
> > > > + *   - (0) if successful.
> > > > + *   - (-ENOTSUP) if hardware doesn't support.
> > > > + *   - (-ENODEV) if *port_id* invalid.
> > > > + */
> > > > +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.
  

> 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.
  

> 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
  

On Mon, 6 Mar 2017 20:41:27 +0000
"Wiles, Keith" <keith.wiles@intel.com> wrote:

> 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.


I understand why you make that argument, but in practice it doesn't work that way.
When new features get added to DPDK, then the application must request those features through configration and other
API's. Therefore building everything into eth_dev doesn't seem to be helpful.
  

> -----Original Message-----
> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> Sent: Monday, March 6, 2017 8:54 PM
> To: Wiles, Keith <keith.wiles@intel.com>
> Cc: Thomas Monjalon <thomas.monjalon@6wind.com>; Dumitrescu, Cristian
> <cristian.dumitrescu@intel.com>; DPDK <dev@dpdk.org>;
> jerin.jacob@caviumnetworks.com;
> balasubramanian.manoharan@cavium.com; hemant.agrawal@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
> 
> On Mon, 6 Mar 2017 20:41:27 +0000
> "Wiles, Keith" <keith.wiles@intel.com> wrote:
> 
> > 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.
> 
> 
> I understand why you make that argument, but in practice it doesn't work
> that way.
> When new features get added to DPDK, then the application must request
> those features through configration and other
> API's. Therefore building everything into eth_dev doesn't seem to be
> helpful.

Stephen, I think we are all aligned here. Question is: do you want the application to discover the supported capabilities through a standard API or do you want each capability to provide its own specific discovery mechanism (if any)? This patch proposes a standard API.
  

2017-03-07 10:14, Dumitrescu, Cristian:
> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> > On Mon, 6 Mar 2017 20:41:27 +0000
> > "Wiles, Keith" <keith.wiles@intel.com> wrote:
> > 
> > > 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.
> > 
> > 
> > I understand why you make that argument, but in practice it doesn't work
> > that way.
> > When new features get added to DPDK, then the application must request
> > those features through configration and other
> > API's. Therefore building everything into eth_dev doesn't seem to be
> > helpful.
> 
> Stephen, I think we are all aligned here. Question is: do you want the application to discover the supported capabilities through a standard API or do you want each capability to provide its own specific discovery mechanism (if any)? This patch proposes a standard API.

Just a precision: A function with a void* parameter is not a fully defined API.
We still need to know how to interpret the void* in each case.
  

> On Mar 7, 2017, at 6:56 AM, Thomas Monjalon <thomas.monjalon@6wind.com> wrote:
> 
> 2017-03-07 10:14, Dumitrescu, Cristian:
>> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
>>> On Mon, 6 Mar 2017 20:41:27 +0000
>>> "Wiles, Keith" <keith.wiles@intel.com> wrote:
>>> 
>>>> 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.
>>> 
>>> 
>>> I understand why you make that argument, but in practice it doesn't work
>>> that way.
>>> When new features get added to DPDK, then the application must request
>>> those features through configration and other
>>> API's. Therefore building everything into eth_dev doesn't seem to be
>>> helpful.
>> 
>> Stephen, I think we are all aligned here. Question is: do you want the application to discover the supported capabilities through a standard API or do you want each capability to provide its own specific discovery mechanism (if any)? This patch proposes a standard API.
> 
> Just a precision: A function with a void* parameter is not a fully defined API.
> We still need to know how to interpret the void* in each case.

One simple solution is to create inline function with the correct prototypes and a reasonable name for that function. The inline will just call the generic API providing the enum and the structure pointer converted into a void *. Using this simple method we get both solutions and adding a strong type check.

Regards,
Keith
  

This patch set introduces an ethdev-based abstraction layer for Quality of
Service (QoS) Traffic Management, which includes: hierarchical scheduling,
traffic shaping, congestion management, packet marking. The goal is to
provide a simple generic API that is agnostic of the underlying HW, SW or
mixed HW-SW implementation.

Patch 1 uses the approach introduced by rte_flow in DPDK to extend the
ethdev functionality in a modular way for traffic management.

Patch 2 introduces the generic ethdev API for traffic management.

Cristian Dumitrescu (2):
  ethdev: add traffic management ops get API
  ethdev: add traffic management API

 MAINTAINERS                            |    4 +
 lib/librte_ether/Makefile              |    5 +-
 lib/librte_ether/rte_ethdev.c          |   12 +
 lib/librte_ether/rte_ethdev.h          |   20 +
 lib/librte_ether/rte_ether_version.map |   36 +
 lib/librte_ether/rte_tm.c              |  448 ++++++++
 lib/librte_ether/rte_tm.h              | 1923 ++++++++++++++++++++++++++++++++
 lib/librte_ether/rte_tm_driver.h       |  373 +++++++
 8 files changed, 2820 insertions(+), 1 deletion(-)
 create mode 100644 lib/librte_ether/rte_tm.c
 create mode 100644 lib/librte_ether/rte_tm.h
 create mode 100644 lib/librte_ether/rte_tm_driver.h