From patchwork Fri Mar 3 18:40:59 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Ben Pfaff X-Patchwork-Id: 735202 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 3vZdLH6lVbz9s3v for ; Sat, 4 Mar 2017 05:41:11 +1100 (AEDT) Received: from mail.linux-foundation.org (localhost [127.0.0.1]) by mail.linuxfoundation.org (Postfix) with ESMTP id B366BC47; Fri, 3 Mar 2017 18:41:09 +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 618DCC46 for ; Fri, 3 Mar 2017 18:41:08 +0000 (UTC) X-Greylist: domain auto-whitelisted by SQLgrey-1.7.6 Received: from relay3-d.mail.gandi.net (relay3-d.mail.gandi.net [217.70.183.195]) by smtp1.linuxfoundation.org (Postfix) with ESMTPS id 8A73EF4 for ; Fri, 3 Mar 2017 18:41:07 +0000 (UTC) Received: from mfilter21-d.gandi.net (mfilter21-d.gandi.net [217.70.178.149]) by relay3-d.mail.gandi.net (Postfix) with ESMTP id 32BDFA80D3; Fri, 3 Mar 2017 19:41:06 +0100 (CET) X-Virus-Scanned: Debian amavisd-new at mfilter21-d.gandi.net Received: from relay3-d.mail.gandi.net ([IPv6:::ffff:217.70.183.195]) by mfilter21-d.gandi.net (mfilter21-d.gandi.net [::ffff:10.0.15.180]) (amavisd-new, port 10024) with ESMTP id 7p35OR7xb2Bd; Fri, 3 Mar 2017 19:41:04 +0100 (CET) X-Originating-IP: 208.91.2.3 Received: from ovn.org (unknown [208.91.2.3]) (Authenticated sender: blp@ovn.org) by relay3-d.mail.gandi.net (Postfix) with ESMTPSA id 7D27CA80CE; Fri, 3 Mar 2017 19:41:02 +0100 (CET) Date: Fri, 3 Mar 2017 10:40:59 -0800 From: Ben Pfaff To: Jan Scheurich Message-ID: <20170303184059.GT29912@ovn.org> References: MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.23 (2014-03-12) X-Spam-Status: No, score=-2.6 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_LOW autolearn=ham version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on smtp1.linux-foundation.org Cc: "dev@openvswitch.org" Subject: Re: [ovs-dev] [PATCH 3/7] userspace: Switching of L3 packets in L2 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 On Fri, Feb 03, 2017 at 10:40:09AM +0000, Jan Scheurich wrote: > Ports have a new layer3 attribute if they send/receive L3 packets. > > The packet_type included in structs dp_packet and flow is considered in > ofproto-dpif. The classical L2 match fields (dl_src, dl_dst, dl_type, and > vlan_tci, vlan_vid, vlan_pcp) now have Ethernet as pre-requisite. > > A dummy ethernet header is pushed to L3 packets received from L3 ports > before the pipeline processing starts. The ethernet header is popped > before sending a packet to a L3 port. > > For datapath ports that can receive L2 or L3 packets, the packet_type > becomes part of the flow key for datapath flows and is handled > appropriately in dpif-netdev. > > Signed-off-by: Lorand Jakab > Signed-off-by: Simon Horman > Signed-off-by: Jiri Benc > Signed-off-by: Yi Yang > Signed-off-by: Jan Scheurich > Co-authored-by: Zoltan Balogh dp_netdev_flow_add() should use VLOG_DROP_DBG() once, and then VLOG_DBG() twice if it OKs it, instead of VLOG_DBG_RL twice. Otherwise you can get the first log message but not the second one, which will be less useful. s/isntalled/installed/ in the comment in the same function. This patch makes some white space only changes to dp_netdev_flow_add(). Please don't. Please don't add comments about changes, like this one. Readers should not have to understand the history to understand the code: + if (put->flags & DPIF_FP_MODIFY) { + /* Removed the additional check + * flow_equal(&match.flow, &netdev_flow->flow) as a) the + * dpcls lookup is sufficient to uniquely identify a flow + * and b) it caused false negatives because the flow in + * netdev->flow may not properly be masked. */ The right place to explain changes is in the commit message. If there needs to be extra explanation of an algorithm in comments, that's perfectly appropriate, but it should talk about the code as it exists, not as it once did. I see that match_format() prints packet_type as a hex number. I doubt that's the best for human consumption. At the very least, it seems like it's better printed as a pair of numbers, and probably it would be more friendly to say what packet type it is. Similarly in format_odp_key_attr(). What is the value of this change? format_eth_masked() incorporates the same check that it adds: @@ -1231,12 +1242,16 @@ match_format(const struct match *match, struct ds *s, int priority) ds_put_format(s, "%svlan_tci=%s0x%04"PRIx16"/0x%04"PRIx16",", colors.param, colors.end, ntohs(f->vlan_tci), ntohs(wc->masks.vlan_tci)); } } - format_eth_masked(s, "dl_src", f->dl_src, wc->masks.dl_src); - format_eth_masked(s, "dl_dst", f->dl_dst, wc->masks.dl_dst); + if (!eth_addr_is_zero(wc->masks.dl_src)) { + format_eth_masked(s, "dl_src", f->dl_src, wc->masks.dl_src); + } + if (!eth_addr_is_zero(wc->masks.dl_dst)) { + format_eth_masked(s, "dl_dst", f->dl_dst, wc->masks.dl_dst); + } We generally avoid memset when another choice is available, e.g.: Can we fix whatever problem this is instead of working around it? + /* Wildcard ethernet addresses if the original packet type was not + * Ethernet. + * XXX: This is a work-around. ofproto shouldn't unwildcard the Ethernet + * addresses at all. */ + if (ctx->xin->upcall_flow->packet_type != PT_ETH) { + memset(&ctx->wc->masks.dl_dst, 0, ETH_ADDR_LEN); + memset(&ctx->wc->masks.dl_src, 0, ETH_ADDR_LEN); + } + and this? + /* XXX: ONLY FOR NON-PTAP BRIDGE! */ + if (flow->packet_type != PT_ETH && in_port && in_port->is_layer3 && + ctx.table_id == 0) { + /* Add dummy Ethernet header to non-L2 packet if it's coming from a + * L3 port. So all packets will be L2 packets for lookup. + * The dl_type has already been set from the packet_type. */ + flow->packet_type = PT_ETH; + memset(&flow->dl_src, 0, ETH_ADDR_LEN); + memset(&flow->dl_dst, 0, ETH_ADDR_LEN); + } Thanks, Ben. diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c index 5d0ed86d46df..b571d83d56a8 100644 --- a/ofproto/ofproto-dpif-xlate.c +++ b/ofproto/ofproto-dpif-xlate.c @@ -3149,8 +3149,8 @@ compose_output_action__(struct xlate_ctx *ctx, ofp_port_t ofp_port, else if (flow->packet_type != PT_ETH && !xport->is_layer3) { /* L2 outport and non-ethernet packet_type -> add dummy eth header. */ flow->packet_type = PT_ETH; - memset(&flow->dl_dst, 0,ETH_ADDR_LEN); - memset(&flow->dl_src, 0,ETH_ADDR_LEN); + flow->dl_dst = eth_addr_zero; + flow->dl_src = eth_addr_zero; } if (xport->peer) {