From 27367ab292c58761266380add57c1c57a99a4f10 Mon Sep 17 00:00:00 2001
From: David Wragg <david@wragg.org>
Date: Mon, 29 Feb 2016 20:32:24 +0000
Subject: [PATCH] Process DiscardingFlowOp correctly in FastDatapath send()

The nil FlowOp check in collectFlowOps was not addressed with the
introduction of DiscardingFlowOp.  This meant that DiscardingFlowOps
produced in the router core would get handled by fastdp as if they were
foreign FlowOps, preventing flow creation and so leading to unwarranted
ODP misses.

To fix this, collectFlowOps cannot simply eliminate FlowOps for which
Discards() is true, because it's true for odpFlowKeyFlowOp.  So the
Discards() case has to be handled in FastDatapath send().
---
 router/fastdp.go | 12 ++++++++----
 router/flow.go   |  5 +----
 2 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/router/fastdp.go b/router/fastdp.go
index c23f1a38..6cfd0f49 100644
--- a/router/fastdp.go
+++ b/router/fastdp.go
@@ -775,10 +775,10 @@ func (fwd *fastDatapathForwarder) Forward(key ForwardPacketKey) FlowOp {
 	defer fwd.lock.RUnlock()
 
 	if fwd.remoteAddr == nil {
-		// Returning nil would discard the packet, but also
-		// result in a flow rule, which we would have to
-		// invalidate when we learn the remote IP.  So for
-		// now, just prevent flows.
+		// Returning a DiscardingFlowOp would discard the
+		// packet, but also result in a flow rule, which we
+		// would have to invalidate when we learn the remote
+		// IP.  So for now, just prevent flows.
 		return vetoFlowCreationFlowOp{}
 	}
 
@@ -1086,6 +1086,10 @@ func (fastdp *FastDatapath) send(fops FlowOp, frame []byte, lock *fastDatapathLo
 		case vetoFlowCreationFlowOp:
 			createFlow = false
 		default:
+			if xfop.Discards() {
+				continue
+			}
+
 			// A foreign FlowOp (e.g. a sleeve forwarding
 			// FlowOp), so send the packet through the
 			// FlowOp interface, decoding the packet
diff --git a/router/flow.go b/router/flow.go
index e523a255..8bc81bf3 100644
--- a/router/flow.go
+++ b/router/flow.go
@@ -84,15 +84,12 @@ func (mfop *MultiFlowOp) Discards() bool {
 	return true
 }
 
+// Flatten out a FlowOp to eliminate any MultiFlowOps
 func FlattenFlowOp(fop FlowOp) []FlowOp {
 	return collectFlowOps(nil, fop)
 }
 
 func collectFlowOps(into []FlowOp, fop FlowOp) []FlowOp {
-	if fop == nil {
-		return into
-	}
-
 	if mfop, ok := fop.(*MultiFlowOp); ok {
 		for _, op := range mfop.ops {
 			into = collectFlowOps(into, op)