From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by dpdk.org (Postfix) with ESMTP id 3C75D7E80 for ; Mon, 20 Oct 2014 03:16:05 +0200 (CEST) Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by fmsmga101.fm.intel.com with ESMTP; 19 Oct 2014 18:24:13 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="4.97,862,1389772800"; d="scan'208";a="402689849" Received: from pgsmsx103.gar.corp.intel.com ([10.221.44.82]) by FMSMGA003.fm.intel.com with ESMTP; 19 Oct 2014 18:16:42 -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 09:22:51 +0800 Received: from shsmsx104.ccr.corp.intel.com (10.239.4.70) by PGSMSX102.gar.corp.intel.com (10.221.44.80) with Microsoft SMTP Server (TLS) id 14.3.195.1; Mon, 20 Oct 2014 09:22:50 +0800 Received: from shsmsx101.ccr.corp.intel.com ([169.254.1.202]) by SHSMSX104.ccr.corp.intel.com ([169.254.5.174]) with mapi id 14.03.0195.001; Mon, 20 Oct 2014 09:22:49 +0800 From: "Liu, Jijiang" To: Thomas Monjalon Thread-Topic: [dpdk-dev] [PATCH v5 4/8]librte_ether:add a common filter API Thread-Index: AQHP6X0lfROQBkzJQEK2KXnfI24S+pwzz6kw//+UfACABNFuwA== Date: Mon, 20 Oct 2014 01:22:49 +0000 Message-ID: <1ED644BD7E0A5F4091CF203DAFB8E4CC01D7FDAE@SHSMSX101.ccr.corp.intel.com> References: <1413006935-22535-1-git-send-email-jijiang.liu@intel.com> <1584946.LFzgr7T2Dy@xps13> <1ED644BD7E0A5F4091CF203DAFB8E4CC01D776DD@SHSMSX101.ccr.corp.intel.com> <5479453.a7APy3joTM@xps13> In-Reply-To: <5479453.a7APy3joTM@xps13> 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 v5 4/8]librte_ether:add a common filter API 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 01:16:05 -0000 > -----Original Message----- > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com] > Sent: Friday, October 17, 2014 3:48 PM > To: Liu, Jijiang > Cc: dev@dpdk.org > Subject: Re: [dpdk-dev] [PATCH v5 4/8]librte_ether:add a common filter AP= I >=20 > 2014-10-17 06:53, Liu, Jijiang: > > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com] > > > 2014-10-11 13:55, Jijiang Liu: > > > > +#define ETH_TUNNEL_FILTER_OMAC 0x01 > > > > +#define ETH_TUNNEL_FILTER_OIP 0x02 > > > > +#define ETH_TUNNEL_FILTER_TENID 0x04 #define > > > > +ETH_TUNNEL_FILTER_IMAC 0x08 #define ETH_TUNNEL_FILTER_IVLAN > 0x10 > > > > +#define ETH_TUNNEL_FILTER_IIP 0x20 > > > > + > > > > +#define RTE_TUNNEL_FLAGS_TO_QUEUE 1 > > > > > > These values requires some comments. > > OK, add comments for these MACROs > > > > +/* > > > > + * Tunneled filter type > > > > + */ > > > > +enum rte_tunnel_filter_type { > > > > + RTE_TUNNEL_FILTER_TYPE_NONE =3D 0, > > > > + RTE_TUNNEL_FILTER_OIP =3D ETH_TUNNEL_FILTER_OIP, > > > > + RTE_TUNNEL_FILTER_IMAC_IVLAN =3D > > > > + ETH_TUNNEL_FILTER_IMAC | ETH_TUNNEL_FILTER_IVLAN, > > > > + RTE_TUNNEL_FILTER_IMAC_IVLAN_TENID =3D > > > > + ETH_TUNNEL_FILTER_IMAC | ETH_TUNNEL_FILTER_IVLAN | > > > > + ETH_TUNNEL_FILTER_TENID, > > > > + RTE_TUNNEL_FILTER_IMAC_TENID =3D > > > > + ETH_TUNNEL_FILTER_IMAC | ETH_TUNNEL_FILTER_TENID, > > > > + RTE_TUNNEL_FILTER_IMAC =3D ETH_TUNNEL_FILTER_IMAC, > > > > + RTE_TUNNEL_FILTER_OMAC_TENID_IMAC =3D > > > > + ETH_TUNNEL_FILTER_OMAC | ETH_TUNNEL_FILTER_TENID | > > > > + ETH_TUNNEL_FILTER_IMAC, > > > > + RTE_TUNNEL_FILTER_IIP =3D ETH_TUNNEL_FILTER_IIP, > > > > + RTE_TUNNEL_FILTER_TYPE_MAX, > > > > +}; > > > > > > It's absolutely impossible to understand. Keep in mind the first > > > goal of an > > > API: be used (which imply to be understood by users). > > > And I really don't understand why you define values for combination > > > of previous flags. Please, keep it simple. > > > > The goal of defining values for combination of filter type in order to > > easily distinguish/check if the mandatory parameters are valid for a > > specific filter type, for example, if the filter type is > > RTE_TUNNEL_FILTER_IMAC_IVLAN, we just need to check if the inner MAC > > address and inner VLAN ID are valid. > > To limit sanity checks to valid parameters the rte_tunnel_filter_type > > enumeration can be replaced/initialized by bit mask. > > > > Furthermore, please look at i40e_tunnel_filter_param_check () function > > in "[PATCH v5 5/8]i40e:implement API of VxLAN packet filter in > librte_pmd_i40e" patch. > > static int > > +i40e_tunnel_filter_param_check(struct i40e_pf *pf, > > + struct rte_eth_tunnel_filter_conf *filter) { > > + ... > > > > + if ((filter->filter_type & ETH_TUNNEL_FILTER_OMAC) && > > + (is_zero_ether_addr(filter->outer_mac))) { > > + PMD_DRV_LOG(ERR, "Cannot add NULL outer MAC > address\n"); > > + return -EINVAL; > > + } > > + > > + if ((filter->filter_type & ETH_TUNNEL_FILTER_IMAC) && > > + (is_zero_ether_addr(filter->inner_mac))) { > > + PMD_DRV_LOG(ERR, "Cannot add NULL inner MAC > address\n"); > > + return -EINVAL; > > + } > > + > > + return 0; > > +} > > > > Actually, If you really don't like rte_tunnel_filter_type definition > > style, and I can change it. >=20 > Yes, you can just replace this "enum rte_tunnel_filter_type" by an integ= er > like uint16_t. It won't change your tests. Ok,thanks. > -- > Thomas