Re: [dpdk-dev] [PATCH] lib/librte_ether: new filter APIs definition

["Wu, Jingjing" <jingjing.wu@intel.com>]

> -----Original Message-----
> From: Wu, Jingjing
> Sent: Saturday, October 18, 2014 12:02 AM
> To: Thomas Monjalon
> Cc: dev@dpdk.org
> Subject: RE: [dpdk-dev] [PATCH] lib/librte_ether: new filter APIs definition
> 
> 
> 
> > -----Original Message-----
> > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > Sent: Friday, October 17, 2014 5:08 PM
> > To: Wu, Jingjing
> > Cc: dev@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH] lib/librte_ether: new filter APIs
> > definition
> >
> > 2014-10-17 07:29, Jingjing Wu:
> > > Define new APIs to support configure multi-kind filters using same
> > > APIs, instead of creating each API set for each kind of filter.
> > >  - rte_eth_dev_filter_supported
> > >  - rte_eth_dev_filter_ctrl
> > >
> > > Filter types, operations, and structures are defined specifically in
> > > new header file lib/librte_eth/rte_dev_ctrl.h.
> > >
> > > As to the implementation discussion, please refer to
> > > http://dpdk.org/ml/archives/dev/2014-September/005179.html
> > [...]
> > > --- /dev/null
> > > +++ b/lib/librte_ether/rte_eth_ctrl.h
> >
> > Why this name? I think we can reserve this file for filtering API.
> > So rte_eth_rx_filter.h would be more appropriate.
> >
> Yes, the name rte_eth_ctrl.h can be extensible for XX_ctrl APIs.
> While the rte_eth_rx_filter.h only can be used for rx filter features.
> 
> > > +/**
> > > + * All generic operations to filters  */
> >
> > rewording: "Generic operations on filters"
> OK.
> > Could you elaborate on "generic"? What would mean "specific"?
> >
> OK, will add more description.

Generic operations means: the operations filter usually have, like add/delete/set ...
Specific means: different filter type may have different attributes, for example the filter's input set. The Specific stuffs are contained in the structure definition. Structure definition is not included in this patch. It need to be added in rte_eth_ctrl.h header file by owner who contributes to each kind of filter.
> 
> > > +enum rte_filter_op {
> > > +	RTE_ETH_FILTER_OP_NONE = 0,
> > > +	/**< used to check whether the type filter is supported */
> > > +	RTE_ETH_FILTER_OP_ADD,      /**< add filter entry */
> > > +	RTE_ETH_FILTER_OP_UPDATE,   /**< update filter entry */
> > > +	RTE_ETH_FILTER_OP_DELETE,   /**< delete filter entry */
> > > +	RTE_ETH_FILTER_OP_FLUSH,    /**< flush all entries */
> > > +	RTE_ETH_FILTER_OP_GET,      /**< get filter entry */
> > > +	RTE_ETH_FILTER_OP_SET,      /**< configurations */
> > > +	RTE_ETH_FILTER_OP_GET_INFO,
> >
> > Could we remove "OP", except for OP_NONE and OP_MAX?
> >
> Fine, ADD/UPDATE/... is operation definitely, OP may be redundant.
> 
> > > +	/**< get information of filter, such as status or statistics */
> > > +	RTE_ETH_FILTER_OP_MAX,
> > > +};
> >
> > > +int
> > > +rte_eth_dev_filter_supported(uint8_t port_id, enum rte_filter_type
> > > +filter_type)
> >
> > This function is really important for compatibility. Good
> >
> > > +/**
> > > + * Take operations to assigned filter type on an Ethernet device.
> > > + * All the supported operations and filter types are defined in
> 'rte_eth_ctrl.h'.
> > > + *
> > > + * @param port_id
> > > + *   The port identifier of the Ethernet device.
> > > + * @param filter_type
> > > + *   filter type.
> > > + * @param filter_op
> > > + *   The operation taken to assigned filter.
> >
> > Rewording: "Type of operation"
> >
> OK, will change.
> 
> > > + * @param arg
> > > + *   A pointer to arguments defined specifically for the operation.
> >
> > Actually, arg is specific to the filter type.
> > Could it be also specific to the operation. Maybe.
> > I think we will have to explicitly specify which operations can be
> > used with each structure (in its comments).
> >
> Yes, each owner who define structures here need to point out the specific
> filter type and operation in its comments.
> 
> > > + * @return
> > > + *   - (0) if successful.
> > > + *   - (-ENOTSUP) if hardware doesn't support.
> > > + *   - (-ENODEV) if *port_id* invalid.
> > > + *   - others depends on the specific operations implementation.
> > > + */
> > > +int rte_eth_dev_filter_ctrl(uint8_t port_id, enum rte_filter_type
> filter_type,
> > > +			enum rte_filter_op filter_op, void *arg);
> >
> > Could we add rx in the name? rte_eth_dev_rx_filter_ctrl If you agree
> > with this naming, it should be added in several other places.
> >
> We name it as rte_eth_dev_rx_filter_ctrl before. But we have no Idea
> whether we have tx_filter, or even have in future, we can share the API.
> So decided to name it as rte_eth_dev_filter_ctrl to make the name brief.
> 
> > This API is quite simple (which is a good thing).
> > Let's see how it fits when integrating filtering features.
> > If something appears to be wrongly designed when integrating a feature
> > or when implementing it in a driver, feel free to fix the API.
> >
> We think so too.
> 
> > Thanks
> > --
> > Thomas
> Thanks
> Jingjing