From patchwork Wed Feb 3 22:19:49 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Joe Stringer X-Patchwork-Id: 578421 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from archives.nicira.com (li376-54.members.linode.com [96.126.127.54]) by ozlabs.org (Postfix) with ESMTP id 47D121409B7 for ; Thu, 4 Feb 2016 09:20:15 +1100 (AEDT) Received: from archives.nicira.com (localhost [127.0.0.1]) by archives.nicira.com (Postfix) with ESMTP id 6B4E010C2B; Wed, 3 Feb 2016 14:20:14 -0800 (PST) X-Original-To: dev@openvswitch.org Delivered-To: dev@openvswitch.org Received: from mx1e3.cudamail.com (mx1.cudamail.com [69.90.118.67]) by archives.nicira.com (Postfix) with ESMTPS id 2249C10C27 for ; Wed, 3 Feb 2016 14:20:13 -0800 (PST) Received: from bar5.cudamail.com (localhost [127.0.0.1]) by mx1e3.cudamail.com (Postfix) with ESMTPS id 99D2542068B for ; Wed, 3 Feb 2016 15:20:12 -0700 (MST) X-ASG-Debug-ID: 1454538011-09eadd5d4d1cdc10001-byXFYA Received: from mx1-pf2.cudamail.com ([192.168.24.2]) by bar5.cudamail.com with ESMTP id CAFZk5OAMAVa1Izj (version=TLSv1 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO) for ; Wed, 03 Feb 2016 15:20:11 -0700 (MST) X-Barracuda-Envelope-From: joe@ovn.org X-Barracuda-RBL-Trusted-Forwarder: 192.168.24.2 Received: from unknown (HELO relay4-d.mail.gandi.net) (217.70.183.196) by mx1-pf2.cudamail.com with ESMTPS (DHE-RSA-AES256-SHA encrypted); 3 Feb 2016 22:20:11 -0000 Received-SPF: pass (mx1-pf2.cudamail.com: SPF record at ovn.org designates 217.70.183.196 as permitted sender) X-Barracuda-Apparent-Source-IP: 217.70.183.196 X-Barracuda-RBL-IP: 217.70.183.196 Received: from mfilter42-d.gandi.net (mfilter42-d.gandi.net [217.70.178.172]) by relay4-d.mail.gandi.net (Postfix) with ESMTP id 51A521720CE for ; Wed, 3 Feb 2016 23:20:10 +0100 (CET) X-Virus-Scanned: Debian amavisd-new at mfilter42-d.gandi.net Received: from relay4-d.mail.gandi.net ([IPv6:::ffff:217.70.183.196]) by mfilter42-d.gandi.net (mfilter42-d.gandi.net [::ffff:10.0.15.180]) (amavisd-new, port 10024) with ESMTP id QOhz9YOTGtqo for ; Wed, 3 Feb 2016 23:20:08 +0100 (CET) X-Originating-IP: 74.125.82.43 Received: from mail-wm0-f43.google.com (mail-wm0-f43.google.com [74.125.82.43]) (Authenticated sender: joe@ovn.org) by relay4-d.mail.gandi.net (Postfix) with ESMTPSA id C1C3A1720BF for ; Wed, 3 Feb 2016 23:20:08 +0100 (CET) Received: by mail-wm0-f43.google.com with SMTP id p63so186367272wmp.1 for ; Wed, 03 Feb 2016 14:20:08 -0800 (PST) X-Gm-Message-State: AG10YORJjTa6sN850gH1dXRSCvsocAIq9ebdNHKKdjlShHmuU1SqI9JN/ActAT459pRUhQa180xN1/3/EyIkUg== X-Received: by 10.194.161.166 with SMTP id xt6mr4636975wjb.98.1454538008585; Wed, 03 Feb 2016 14:20:08 -0800 (PST) MIME-Version: 1.0 Received: by 10.27.147.211 with HTTP; Wed, 3 Feb 2016 14:19:49 -0800 (PST) In-Reply-To: <1454531581-50501-2-git-send-email-jarno@ovn.org> References: <1454531581-50501-1-git-send-email-jarno@ovn.org> <1454531581-50501-2-git-send-email-jarno@ovn.org> X-CudaMail-Envelope-Sender: joe@ovn.org From: Joe Stringer Date: Wed, 3 Feb 2016 14:19:49 -0800 X-Gmail-Original-Message-ID: Message-ID: X-CudaMail-Whitelist-To: dev@openvswitch.org X-CudaMail-MID: CM-E2-202091259 X-CudaMail-DTE: 020316 X-CudaMail-Originating-IP: 217.70.183.196 To: Jarno Rajahalme X-ASG-Orig-Subj: [##CM-E2-202091259##]Re: [ovs-dev] [PATCH branch-2.3 2/2] ofproto: Allow in-place modifications of datapath flows. X-Barracuda-Connect: UNKNOWN[192.168.24.2] X-Barracuda-Start-Time: 1454538011 X-Barracuda-Encrypted: DHE-RSA-AES256-SHA X-Barracuda-URL: https://web.cudamail.com:443/cgi-mod/mark.cgi X-ASG-Whitelist: Header =?UTF-8?B?eFwtY3VkYW1haWxcLXdoaXRlbGlzdFwtdG8=?= X-Virus-Scanned: by bsmtpd at cudamail.com X-Barracuda-BRTS-Status: 1 Cc: ovs dev Subject: Re: [ovs-dev] [PATCH branch-2.3 2/2] ofproto: Allow in-place modifications of datapath flows. X-BeenThere: dev@openvswitch.org X-Mailman-Version: 2.1.16 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@openvswitch.org Sender: "dev" On 3 February 2016 at 12:33, Jarno Rajahalme wrote: > From: Ethan Jackson > > There are certain use cases (such as bond rebalancing) where a > datapath flow's actions may change, while it's wildcard pattern > remains the same. Before this patch, revalidators would note the > change, delete the flow, and wait for the handlers to install an > updated version. This is inefficient, as many packets could get > punted to userspace before the new flow is finally installed. > > To improve the situation, this patch implements in place modification > of datapath flows. If the revalidators detect the only change to a > given ukey is its actions, instead of deleting it, it does a put with > the MODIFY flag set. > > This patch is a backport of commit 43b2f13 to branch-2.3. > > Signed-off-by: Ethan J. Jackson > Signed-off-by: Jarno Rajahalme This also squashes in commit c56eba3b7ab0 ("ofproto-dpif-upcall: Don't delete modified ukeys."). > @@ -1543,62 +1611,68 @@ revalidate(struct revalidator *revalidator) > } > > dpif_flow_dump_state_uninit(udpif->dpif, state); > -} > - > -/* Called with exclusive access to 'revalidator' and 'ukey'. */ > -static bool > -handle_missed_revalidation(struct revalidator *revalidator, > - struct udpif_key *ukey) > - OVS_NO_THREAD_SAFETY_ANALYSIS > -{ > - struct udpif *udpif = revalidator->udpif; > - struct nlattr *mask, *actions; > - size_t mask_len, actions_len; > - struct dpif_flow_stats stats; > - struct ofpbuf *buf; > - bool keep = false; > - > - COVERAGE_INC(revalidate_missed_dp_flow); > - > - if (!dpif_flow_get(udpif->dpif, ukey->key, ukey->key_len, &buf, > - &mask, &mask_len, &actions, &actions_len, &stats)) { > - keep = revalidate_ukey(udpif, ukey, mask, mask_len, actions, > - actions_len, &stats); > - ofpbuf_delete(buf); > - } > - > - return keep; > + ofpbuf_uninit(&odp_actions); > } > > static void > revalidator_sweep__(struct revalidator *revalidator, bool purge) > OVS_NO_THREAD_SAFETY_ANALYSIS > { > + uint64_t odp_actions_stub[1024 / 8]; > + struct ofpbuf odp_actions; > struct dump_op ops[REVALIDATE_MAX_BATCH]; > struct udpif_key *ukey, *next; > size_t n_ops; > > n_ops = 0; > > + ofpbuf_use_stub(&odp_actions, odp_actions_stub, sizeof odp_actions_stub); > + > /* During garbage collection, this revalidator completely owns its ukeys > * map, and therefore doesn't need to do any locking. */ > HMAP_FOR_EACH_SAFE (ukey, next, hmap_node, revalidator->ukeys) { > if (ukey->flow_exists) { > bool missed_flow = !ukey->mark; > + enum reval_result result; > + struct nlattr *mask; > + size_t mask_len; > + struct ofpbuf *buf = NULL; > > ukey->mark = false; > - if (purge > - || (missed_flow > - && revalidator->udpif->need_revalidate > - && !handle_missed_revalidation(revalidator, ukey))) { > - struct dump_op *op = &ops[n_ops++]; > - > - dump_op_init(op, ukey->key, ukey->key_len, ukey); > - if (n_ops == REVALIDATE_MAX_BATCH) { > - push_dump_ops(revalidator, ops, n_ops); > - n_ops = 0; > + result = purge ? UKEY_DELETE : UKEY_KEEP; > + > + if (missed_flow && revalidator->udpif->need_revalidate) { > + struct udpif *udpif = revalidator->udpif; > + struct nlattr *actions; > + size_t actions_len; > + struct dpif_flow_stats stats; > + > + COVERAGE_INC(revalidate_missed_dp_flow); > + > + if (!dpif_flow_get(udpif->dpif, ukey->key, ukey->key_len, &buf, > + &mask, &mask_len, &actions, &actions_len, > + &stats)) { > + result = revalidate_ukey(udpif, ukey, mask, mask_len, > + actions, actions_len, &stats, > + &odp_actions); handle_missed_revalidation() would previously return the equivalent of UKEY_DELETE if the dpif_flow_get() fails. This would lead on to attempting flow deletion, which I'd expect is most likely also is going to fail, but it would also result in the ukey getting deleted (which we want). I suspect that with the current version above, if one runs 'ovs-dpctl del-flow...' then the corresponding ukeys will never be deleted. Simplest (same as current) behaviour is probably this incremental: &odp_actions); Separately from this patch, I noticed a couple of other things: - I couldn't compile branch-2.3 without at least this patch for lib/type-props.h: 0f395fd366413208aac7072ef81b5aeda6a9e307 (which depends on c002791a30818c2458599f993d1711e03566e7cc) - I wonder if commit e83c93573b10 ("ofproto-dpif-upcall: Do not attribute stats when flow_del returns error.") is also a candidate for backport, but that's more of a statistics issue rather than reordering/correctness. diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c index 03b4afa7d2c5..ba0cd2f9b2d3 100644 --- a/ofproto/ofproto-dpif-upcall.c +++ b/ofproto/ofproto-dpif-upcall.c @@ -1649,9 +1649,11 @@ revalidator_sweep__(struct revalidator *revalidator, bool purge) COVERAGE_INC(revalidate_missed_dp_flow); - if (!dpif_flow_get(udpif->dpif, ukey->key, ukey->key_len, &buf, - &mask, &mask_len, &actions, &actions_len, - &stats)) { + if (dpif_flow_get(udpif->dpif, ukey->key, ukey->key_len, &buf, + &mask, &mask_len, &actions, &actions_len, + &stats)) { + result = UKEY_DELETE; + } else { result = revalidate_ukey(udpif, ukey, mask, mask_len, actions, actions_len, &stats,