From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by dpdk.org (Postfix) with ESMTP id 8CC826A96 for ; Mon, 20 Oct 2014 05:31:42 +0200 (CEST) Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by fmsmga102.fm.intel.com with ESMTP; 19 Oct 2014 20:39:50 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.04,749,1406617200"; d="scan'208";a="617002979" Received: from pgsmsx103.gar.corp.intel.com ([10.221.44.82]) by fmsmga002.fm.intel.com with ESMTP; 19 Oct 2014 20:39:49 -0700 Received: from pgsmsx102.gar.corp.intel.com (10.221.44.80) by PGSMSX103.gar.corp.intel.com (10.221.44.82) with Microsoft SMTP Server (TLS) id 14.3.195.1; Mon, 20 Oct 2014 11:38:10 +0800 Received: from shsmsx151.ccr.corp.intel.com (10.239.6.50) by PGSMSX102.gar.corp.intel.com (10.221.44.80) with Microsoft SMTP Server (TLS) id 14.3.195.1; Mon, 20 Oct 2014 11:38:09 +0800 Received: from shsmsx104.ccr.corp.intel.com ([169.254.5.174]) by SHSMSX151.ccr.corp.intel.com ([169.254.3.44]) with mapi id 14.03.0195.001; Mon, 20 Oct 2014 11:38:08 +0800 From: "Wu, Jingjing" To: 'Thomas Monjalon' Thread-Topic: [dpdk-dev] [PATCH] lib/librte_ether: new filter APIs definition Thread-Index: AQHP6ZkIZfKKcQfqsEWMKMurTaOZUJwzeleAgADzjeCAA+ubMA== Date: Mon, 20 Oct 2014 03:38:07 +0000 Message-ID: <9BB6961774997848B5B42BEC655768F8B1BFD9@SHSMSX104.ccr.corp.intel.com> References: <1413502161-31403-1-git-send-email-jingjing.wu@intel.com> <4027817.y7ZgbDzL4P@xps13> <9BB6961774997848B5B42BEC655768F8B104C0@SHSMSX104.ccr.corp.intel.com> In-Reply-To: <9BB6961774997848B5B42BEC655768F8B104C0@SHSMSX104.ccr.corp.intel.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Cc: "'dev@dpdk.org'" Subject: Re: [dpdk-dev] [PATCH] lib/librte_ether: new filter APIs definition X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 20 Oct 2014 03:31:43 -0000 > -----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 definit= ion >=20 >=20 >=20 > > -----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. >=20 > > > +/** > > > + * 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/dele= te/set ... Specific means: different filter type may have different attributes, for ex= ample the filter's input set. The Specific stuffs are contained in the stru= cture definition. Structure definition is not included in this patch. It ne= ed to be added in rte_eth_ctrl.h header file by owner who contributes to ea= ch kind of filter. >=20 > > > +enum rte_filter_op { > > > + RTE_ETH_FILTER_OP_NONE =3D 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. >=20 > > > + /**< 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. >=20 > > > + * @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. >=20 > > > + * @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. >=20 > > 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. >=20 > > Thanks > > -- > > Thomas > Thanks > Jingjing