From patchwork Mon Jun 19 10:57:58 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: "Yang, Yi" X-Patchwork-Id: 777703 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from mail.linuxfoundation.org (mail.linuxfoundation.org [140.211.169.12]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 3wrny95WfYz9s71 for ; Mon, 19 Jun 2017 20:58:09 +1000 (AEST) Received: from mail.linux-foundation.org (localhost [127.0.0.1]) by mail.linuxfoundation.org (Postfix) with ESMTP id 8DD33BC6; Mon, 19 Jun 2017 10:58:06 +0000 (UTC) X-Original-To: dev@openvswitch.org Delivered-To: ovs-dev@mail.linuxfoundation.org Received: from smtp1.linuxfoundation.org (smtp1.linux-foundation.org [172.17.192.35]) by mail.linuxfoundation.org (Postfix) with ESMTPS id 0C833BC1 for ; Mon, 19 Jun 2017 10:58:06 +0000 (UTC) X-Greylist: domain auto-whitelisted by SQLgrey-1.7.6 Received: from mga06.intel.com (mga06.intel.com [134.134.136.31]) by smtp1.linuxfoundation.org (Postfix) with ESMTPS id 6803D1F0 for ; Mon, 19 Jun 2017 10:58:05 +0000 (UTC) Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by orsmga104.jf.intel.com with ESMTP; 19 Jun 2017 03:58:04 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos; i="5.39,360,1493708400"; d="scan'208"; a="1162107315" Received: from fmsmsx106.amr.corp.intel.com ([10.18.124.204]) by fmsmga001.fm.intel.com with ESMTP; 19 Jun 2017 03:58:03 -0700 Received: from fmsmsx116.amr.corp.intel.com (10.18.116.20) by FMSMSX106.amr.corp.intel.com (10.18.124.204) with Microsoft SMTP Server (TLS) id 14.3.319.2; Mon, 19 Jun 2017 03:58:01 -0700 Received: from shsmsx151.ccr.corp.intel.com (10.239.6.50) by fmsmsx116.amr.corp.intel.com (10.18.116.20) with Microsoft SMTP Server (TLS) id 14.3.319.2; Mon, 19 Jun 2017 03:58:01 -0700 Received: from shsmsx103.ccr.corp.intel.com ([169.254.4.116]) by SHSMSX151.ccr.corp.intel.com ([169.254.3.56]) with mapi id 14.03.0319.002; Mon, 19 Jun 2017 18:57:58 +0800 From: "Yang, Yi Y" To: =?iso-8859-1?Q?Zolt=E1n_Balogh?= , Ben Pfaff , "dev@openvswitch.org" Thread-Topic: [ovs-dev] [PATCH v2 00/12] Packet type aware pipeline Thread-Index: AQHS6IrVS/5rTYKUgkivnZrMYyBOvqIqv14AgAC2lgCAAI0csA== Date: Mon, 19 Jun 2017 10:57:58 +0000 Message-ID: <79BBBFE6CB6C9B488C1A45ACD284F51961BDA1B8@SHSMSX103.ccr.corp.intel.com> References: <20170618232941.16727-1-blp@ovn.org> <20170618233326.GZ2820@ovn.org> In-Reply-To: Accept-Language: zh-CN, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: dlp-product: dlpe-windows dlp-version: 10.0.102.7 dlp-reaction: no-action x-originating-ip: [10.239.127.40] MIME-Version: 1.0 X-Spam-Status: No, score=-4.2 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_MED, T_RP_MATCHES_RCVD autolearn=ham version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on smtp1.linux-foundation.org Subject: Re: [ovs-dev] [PATCH v2 00/12] Packet type aware pipeline X-BeenThere: ovs-dev@openvswitch.org X-Mailman-Version: 2.1.12 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: ovs-dev-bounces@openvswitch.org Errors-To: ovs-dev-bounces@openvswitch.org Hi, guys I'm working on NSH patches for kernel path, I think we can change it with new NSH kernel patches together. You ovs guys have confirmed several times userspace part can be merged before kernel part isn't there. So I propose we review and merge userspace part first, kernel part will be posted in next series, does it make sense? -----Original Message----- From: ovs-dev-bounces@openvswitch.org [mailto:ovs-dev-bounces@openvswitch.org] On Behalf Of Zoltán Balogh Sent: Monday, June 19, 2017 6:27 PM To: Ben Pfaff ; dev@openvswitch.org Subject: Re: [ovs-dev] [PATCH v2 00/12] Packet type aware pipeline > > A new concern came up while thinking about this series. The > OVS_ATTR_PACKET_TYPE does not appear to be implemented in the kernel > module, and what's more, because of #ifdefs, OVS_ATTR_PACKET_TYPE will > actually have a different value in the kernel module than in userspace. > What's the plan here? Hi Ben, OVS_KEY_ATTR_PACKET_TYPE is represented with OVS_KEY_ATTR_ETHERNET and OVS_KEY_ATTR_ETHERTYPE in the kernel. From Google doc: "The presence of netlink key attribute OVS_KEY_ATTR_ETHERNET is used to indicate if it's about L2 or L3 packet on the netlink interface. The "L3" packet type is encoded in the OVS_KEY_ATTR_ETHERTYPE netlink attribute." The plan was to do a conversion between OVS_KEY_ATTR_PACKET_TYPE and the pair of OVS_KEY_ATTR_ETHERNET + OVS_KEY_ATTR_ETHERTYPE attributes before/after transmission over netlink. Currently the PACKET_TYPE attribute is filtered out in the put_exclude_packet_type() function in lib/dpif-netlink.c. put_exclude_packet_type(struct ofpbuf *buf, uint16_t type, const struct nlattr *data, uint16_t data_len) { const struct nlattr *packet_type; packet_type = nl_attr_find__(data, data_len, OVS_KEY_ATTR_PACKET_TYPE); if (packet_type) { /* exclude PACKET_TYPE Netlink attribute. */ ovs_assert(NLA_ALIGN(packet_type->nla_len) == NL_A_U32_SIZE); size_t packet_type_len = NL_A_U32_SIZE; size_t first_chunk_size = (uint8_t *)packet_type - (uint8_t *)data; size_t second_chunk_size = data_len - first_chunk_size - packet_type_len; uint8_t *first_attr = NULL; struct nlattr *next_attr = nl_attr_next(packet_type); first_attr = nl_msg_put_unspec_uninit(buf, type, data_len - packet_type_len); memcpy(first_attr, data, first_chunk_size); memcpy(first_attr + first_chunk_size, next_attr, second_chunk_size); } else { nl_msg_put_unspec(buf, type, data, data_len); } } This could be modified to do more verification and put OVS_KEY_ATTR_ETHERTYPE if needed: --- packet_type = nl_attr_find__(data, data_len, OVS_KEY_ATTR_PACKET_TYPE); if (packet_type) { - /* exclude PACKET_TYPE Netlink attribute. */ + /* convert PACKET_TYPE Netlink attribute. */ ovs_assert(NLA_ALIGN(packet_type->nla_len) == NL_A_U32_SIZE); + ovs_be32 value = nl_attr_get_be32(packet_type); size_t packet_type_len = NL_A_U32_SIZE; size_t first_chunk_size = (uint8_t *)packet_type - (uint8_t *)data; size_t second_chunk_size = data_len - first_chunk_size @@ -3455,10 +3456,31 @@ put_exclude_packet_type(struct ofpbuf *buf, uint16_t type, uint8_t *first_attr = NULL; struct nlattr *next_attr = nl_attr_next(packet_type); + bool ethernet_present = nl_attr_find__(data, data_len, + OVS_KEY_ATTR_ETHERNET); + first_attr = nl_msg_put_unspec_uninit(buf, type, data_len - packet_type_len); memcpy(first_attr, data, first_chunk_size); memcpy(first_attr + first_chunk_size, next_attr, second_chunk_size); + + /* Put OVS_KEY_ATTR_ETHERTYPE if needed. */ + if (ntohl(value) == PT_ETH) { + ovs_assert(ethernet_present); + } else { + const struct nlattr *eth_type; + ovs_be16 ns_type = pt_ns_type_be(value); + + ovs_assert(ethernet_present == false); + + eth_type = nl_attr_find__(data, data_len, OVS_KEY_ATTR_ETHERTYPE); + if (eth_type) { + ovs_assert(nl_attr_get_be16(eth_type) == ns_type); + } else { + nl_msg_put_be16(buf, OVS_KEY_ATTR_ETHERTYPE, ns_type); + } + } + } else { nl_msg_put_unspec(buf, type, data, data_len); } @@ -3489,11 +3511,11 @@ dpif_netlink_flow_to_ofpbuf(const struct dpif_netlink_flow *flow, } if (!flow->ufid_terse || !flow->ufid_present) { if (flow->key_len) { - put_exclude_packet_type(buf, OVS_FLOW_ATTR_KEY, flow->key, + put_convert_packet_type(buf, OVS_FLOW_ATTR_KEY, flow->key, flow->key_len); } if (flow->mask_len) { - put_exclude_packet_type(buf, OVS_FLOW_ATTR_MASK, flow->mask, + put_convert_packet_type(buf, OVS_FLOW_ATTR_MASK, + flow->mask, flow->mask_len); } if (flow->actions || flow->actions_len) { --- On the Rx side, upcall->packet.packet_type is set in the parse_odp_packet() called by dpif_netlink_recv__(). I'm not sure, if this is enough. Could someone double check, please? Best regards, Zoltan diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c index 562f6134c..bdcc76c7b 100644 --- a/lib/dpif-netlink.c +++ b/lib/dpif-netlink.c @@ -3434,11 +3434,11 @@ dpif_netlink_flow_from_ofpbuf(struct dpif_netlink_flow *flow, /* - * If PACKET_TYPE attribute is present in 'data', it filters PACKET_TYPE out, - * then puts 'data' to 'buf'. + * If PACKET_TYPE attribute is present in 'data', converts it to + ETHERNET and + * ETHERTYPE attributes, then puts 'data' to 'buf'. */ static void -put_exclude_packet_type(struct ofpbuf *buf, uint16_t type, +put_convert_packet_type(struct ofpbuf *buf, uint16_t type, const struct nlattr *data, uint16_t data_len) { const struct nlattr *packet_type; @@ -3446,8 +3446,9 @@ put_exclude_packet_type(struct ofpbuf *buf, uint16_t type,