From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wg0-f50.google.com (mail-wg0-f50.google.com [74.125.82.50]) by dpdk.org (Postfix) with ESMTP id 5ED496A96 for ; Tue, 21 Oct 2014 12:43:04 +0200 (CEST) Received: by mail-wg0-f50.google.com with SMTP id a1so1053072wgh.9 for ; Tue, 21 Oct 2014 03:51:19 -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=hHbRILvwtNWQjheK63JYKat2NRgwLLZeHZeDsjkNoUU=; b=P9dn49a37tax5C6wLA9tks79IJy2VjaJhYB3QhQX9rII09lbixpPWpxvuvy0JkJzM9 pckrPY3KNmXTsca+S1HqA/F3zquo9NjgNDv5heE3bOEqQDPVG7NfXaWv75rg3qjkPEgb RVBA3wH+5Hb/wQ4V0SLqg2WDyjez7lDMgxg2gsB6+NrLerUzUgfGBk7vY4HJ99yTgBnL u9Irj+Kb76R3dfc/yAkVIjHNZD+OknxiKvhSgfJw5rYKieGZHi+7qTYy55BQe1i9S092 jotDB10EXOy/sNQqEmBi/s4muO8DrnGvA7WgFqDN+FoLqF/XpMoBRyK6OuhryIqipUHg LxSw== X-Gm-Message-State: ALoCoQlyffd4QyibpGPezyL0wNm/hs/RXk/NLi9OokzvWvl9uAXZvK6AwWvBO5iZK/rkmOxnQH7Q X-Received: by 10.194.172.234 with SMTP id bf10mr41203046wjc.81.1413888677935; Tue, 21 Oct 2014 03:51:17 -0700 (PDT) Received: from xps13.localnet (136-92-190-109.dsl.ovh.fr. [109.190.92.136]) by mx.google.com with ESMTPSA id p3sm8902898wjf.49.2014.10.21.03.51.16 for (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 21 Oct 2014 03:51:17 -0700 (PDT) From: Thomas Monjalon To: Jijiang Liu Date: Tue, 21 Oct 2014 12:51:01 +0200 Message-ID: <3607443.z2coKxdkI7@xps13> Organization: 6WIND User-Agent: KMail/4.14.1 (Linux/3.16.4-1-ARCH; KDE/4.14.1; x86_64; ; ) In-Reply-To: <1413881168-20239-3-git-send-email-jijiang.liu@intel.com> References: <1413881168-20239-1-git-send-email-jijiang.liu@intel.com> <1413881168-20239-3-git-send-email-jijiang.liu@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 v6 2/9] librte_ether:add VxLAN packet identification API in librte_ether 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: Tue, 21 Oct 2014 10:43:04 -0000 2014-10-21 16:46, Jijiang Liu: > There are "some" destination UDP port numbers that have unque meaning. > In terms of VxLAN, "IANA has assigned the value 4789 for the VXLAN UDP port, and this value > SHOULD be used by default as the destination UDP port. Some early implementations of VXLAN > have used other values for the destination port. To enable interoperability with these > implementations, the destination port SHOULD be configurable." > > Add two APIs in librte_ether for supporting UDP tunneling port configuration on i40e. > Currently, only VxLAN is implemented in this patch set. Actually, there are 2 different things in this patch - new tunnelling API - VXLAN macros Please split in 2 patches. > int > +rte_eth_dev_udp_tunnel_add(uint8_t port_id, > + struct rte_eth_udp_tunnel *udp_tunnel, > + uint8_t count) > +{ > + uint8_t i; > + struct rte_eth_dev *dev; > + struct rte_eth_udp_tunnel *tunnel; > + > + if (port_id >= nb_ports) { > + PMD_DEBUG_TRACE("Invalid port_id=%d\n", port_id); > + return -ENODEV; > + } > + > + if (udp_tunnel == NULL) { > + PMD_DEBUG_TRACE("Invalid udp_tunnel parameter\n"); > + return -EINVAL; > + } > + tunnel = udp_tunnel; > + > + for (i = 0; i < count; i++, tunnel++) { > + if (tunnel->prot_type >= RTE_TUNNEL_TYPE_MAX) { > + PMD_DEBUG_TRACE("Invalid tunnel type\n"); > + return -EINVAL; > + } > + } I'm not sure it's a good idea to provide a count parameter to iterate in a loop. It's probably something that the application should do by itself. > + > + dev = &rte_eth_devices[port_id]; > + FUNC_PTR_OR_ERR_RET(*dev->dev_ops->udp_tunnel_add, -ENOTSUP); > + return (*dev->dev_ops->udp_tunnel_add)(dev, udp_tunnel, count); > +} [...] > +/** > + * Tunneled type. > + */ > +enum rte_eth_tunnel_type { > + RTE_TUNNEL_TYPE_NONE = 0, > + RTE_TUNNEL_TYPE_VXLAN, > + RTE_TUNNEL_TYPE_GENEVE, > + RTE_TUNNEL_TYPE_TEREDO, > + RTE_TUNNEL_TYPE_NVGRE, > + RTE_TUNNEL_TYPE_MAX, > +}; This is moved later from rte_ethdev.h to rte_eth_ctrl.h. Please choose where is the right location in this patch. By the way, I think ethdev is the right location because it's not directly related to ctrl API, right? > struct rte_eth_conf { > + enum rte_eth_tunnel_type tunnel_type; > uint16_t link_speed; > /**< ETH_LINK_SPEED_10[0|00|000], or 0 for autonegotation */ > uint16_t link_duplex; Please don't add this field as the first. It's more logical to start port configuration with link speed, duplex, etc. Then please add a comment to explain how it should be used. But I doubt we should configure a tunnel type for a whole port. There's something weird here. > +/* VXLAN protocol header */ This comment should be doxygen'ed. > +struct vxlan_hdr { > + uint32_t vx_flags; /**< VxLAN flag. */ > + uint32_t vx_vni; /**< VxLAN ID. */ > +} __attribute__((__packed__)); > + [...] > #define ETHER_TYPE_VLAN 0x8100 /**< IEEE 802.1Q VLAN tagging. */ > #define ETHER_TYPE_1588 0x88F7 /**< IEEE 802.1AS 1588 Precise Time Protocol. */ > > +#define ETHER_VXLAN_HLEN (sizeof(struct udp_hdr) + sizeof(struct vxlan_hdr)) Please add a doxygen comment. -- Thomas