From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wi0-f171.google.com (mail-wi0-f171.google.com [209.85.212.171]) by dpdk.org (Postfix) with ESMTP id 324997E7B for ; Fri, 17 Oct 2014 09:39:55 +0200 (CEST) Received: by mail-wi0-f171.google.com with SMTP id em10so1300066wid.16 for ; Fri, 17 Oct 2014 00:47:52 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:from:to:cc:subject:date:message-id:organization :user-agent:in-reply-to:references:mime-version :content-transfer-encoding:content-type; bh=F3DqWjfWScAs/jry5vaLRSYsmYROq5Mpl20LVxsTX3U=; b=j4yhzdZuR/jLnXo4biFboE1EgukYGzGuWmzgWYwSq7CclentRVlbgamIEZsoE5nNVD fxvcJPPKCFe2tptpk7kIGrlQCtqKp/TdbtSA9pDDQjYN7xbBZvf7Hg0Fom0Lym42BY7R 2l25+Da78cOB5tXJ4nZm+gc2BxdmGh4CB43vERX1M/h1FsS/HQARLkzLCF4c9t70Qgj0 zaZHJDw7oqcpKM0rySsNOm/mkL1Owo2WvnKF+AvYlI1YOaZ8BlAUP777LFNWI5EnDvPE N1z0wAgyGObIHkD8w/kXNhDBzXHl9Jn10Tao/1N1Onw81jkjWSeBJ0KJ5SM28X4XXBV/ ZM7Q== X-Gm-Message-State: ALoCoQkwQpBFQ7BOXOa3Y14OMewWlINgf9suVggSY1x0LjzjlBSD6bW6Ss4VOPvjOO7agCn1lPp4 X-Received: by 10.180.36.38 with SMTP id n6mr11628846wij.27.1413532072111; Fri, 17 Oct 2014 00:47:52 -0700 (PDT) Received: from xps13.localnet (136-92-190-109.dsl.ovh.fr. [109.190.92.136]) by mx.google.com with ESMTPSA id ky3sm676556wjb.39.2014.10.17.00.47.50 for (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Fri, 17 Oct 2014 00:47:51 -0700 (PDT) From: Thomas Monjalon To: "Liu, Jijiang" Date: Fri, 17 Oct 2014 09:47:36 +0200 Message-ID: <5479453.a7APy3joTM@xps13> Organization: 6WIND User-Agent: KMail/4.14.1 (Linux/3.16.4-1-ARCH; KDE/4.14.1; x86_64; ; ) In-Reply-To: <1ED644BD7E0A5F4091CF203DAFB8E4CC01D776DD@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> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" 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: Fri, 17 Oct 2014 07:39:55 -0000 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 = 0, > > > + RTE_TUNNEL_FILTER_OIP = ETH_TUNNEL_FILTER_OIP, > > > + RTE_TUNNEL_FILTER_IMAC_IVLAN = > > > + ETH_TUNNEL_FILTER_IMAC | ETH_TUNNEL_FILTER_IVLAN, > > > + RTE_TUNNEL_FILTER_IMAC_IVLAN_TENID = > > > + ETH_TUNNEL_FILTER_IMAC | ETH_TUNNEL_FILTER_IVLAN | > > > + ETH_TUNNEL_FILTER_TENID, > > > + RTE_TUNNEL_FILTER_IMAC_TENID = > > > + ETH_TUNNEL_FILTER_IMAC | ETH_TUNNEL_FILTER_TENID, > > > + RTE_TUNNEL_FILTER_IMAC = ETH_TUNNEL_FILTER_IMAC, > > > + RTE_TUNNEL_FILTER_OMAC_TENID_IMAC = > > > + ETH_TUNNEL_FILTER_OMAC | ETH_TUNNEL_FILTER_TENID | > > > + ETH_TUNNEL_FILTER_IMAC, > > > + RTE_TUNNEL_FILTER_IIP = 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. Yes, you can just replace this "enum rte_tunnel_filter_type" by an integer like uint16_t. It won't change your tests. -- Thomas