diff mbox

[ovs-dev,v2,2/4] ovn: Implement the ability to send a packet back out its input port.

Message ID 1445116064-20782-3-git-send-email-blp@nicira.com
State Superseded
Headers show

Commit Message

Ben Pfaff Oct. 17, 2015, 9:07 p.m. UTC
Otherwise logical router ARP replies won't work as implemented.

Signed-off-by: Ben Pfaff <blp@nicira.com>
---
 ovn/TODO       | 35 -----------------------------------
 ovn/lib/expr.c | 10 ++++++++++
 ovn/ovn-sb.xml |  6 +++++-
 3 files changed, 15 insertions(+), 36 deletions(-)

Comments

Justin Pettit Oct. 18, 2015, 5:58 p.m. UTC | #1
> On Oct 17, 2015, at 2:07 PM, Ben Pfaff <blp@nicira.com> wrote:
> 
> 
> +            if (!port && sf->field->id == MFF_REG6) {

Is there a reason you're using MFF_REG6 instead of MFF_IN_PORT?

> +                sf = ofpact_put_SET_FIELD(ofpacts);
> +                sf->field = mf_from_id(MFF_IN_PORT);
> +                bitwise_put(UINT64_MAX, &sf->mask, sf->field->n_bytes, 0,
> +                            sf->field->n_bits);

Is there a reason to use this instead of bitwise_one(), which seems to have clearer intent?  The code right above doesn't do it either, so I'm just curious if it's not equivalent.

Acked-by: Justin Pettit <jpettit@nicira.com>

--Justin
Ben Pfaff Oct. 18, 2015, 7:49 p.m. UTC | #2
On Sun, Oct 18, 2015 at 10:58:35AM -0700, Justin Pettit wrote:
> 
> > On Oct 17, 2015, at 2:07 PM, Ben Pfaff <blp@nicira.com> wrote:
> > 
> > 
> > +            if (!port && sf->field->id == MFF_REG6) {
> 
> Is there a reason you're using MFF_REG6 instead of MFF_IN_PORT?

MFF_LOG_INPORT, which is what I want, wasn't available here.  We've run
into this problem before.  I'm going to propose moving the MFF_LOG_*
definitions into a new header.  I'll post a v3 of the series that does
that.

> > +                sf = ofpact_put_SET_FIELD(ofpacts);
> > +                sf->field = mf_from_id(MFF_IN_PORT);
> > +                bitwise_put(UINT64_MAX, &sf->mask, sf->field->n_bytes, 0,
> > +                            sf->field->n_bits);
> 
> Is there a reason to use this instead of bitwise_one(), which seems to
> have clearer intent?  The code right above doesn't do it either, so
> I'm just curious if it's not equivalent.

bitwise_one() works nicely too.  Previously, I guess I used
bitwise_put() because it had the same form as the bitwise_put() in the
pairing of value and mask:

            bitwise_put(port, &sf->value,
                        sf->field->n_bytes, 0, sf->field->n_bits);
            bitwise_put(UINT64_MAX, &sf->mask,
                        sf->field->n_bytes, 0, sf->field->n_bits);

and then in this case I just cut-and-paste the code.

I'll change them both to bitwise_one().

> Acked-by: Justin Pettit <jpettit@nicira.com>

Thanks, I'll post v3 in a few minutes.
diff mbox

Patch

diff --git a/ovn/TODO b/ovn/TODO
index 10c3adf..7f69508 100644
--- a/ovn/TODO
+++ b/ovn/TODO
@@ -12,41 +12,6 @@  one router to another, this doesn't seem to matter (just put more than
 one connection between them), but for connections between a router and
 a switch it might matter because a switch has only one router port.
 
-** OVN_SB schema
-
-*** Allow output to ingress port
-
-Sometimes when a packet ingresses into a router, it has to egress the
-same port.  One example is a "one-armed" router that has multiple
-routes on a single port (or in which a host is (mis)configured to send
-every IP packet to the router, e.g. due to a bad netmask).  Another is
-when a router needs to send an ICMP reply to an ingressing packet.
-
-To some degree this problem is layered, because there are two
-different notions of "ingress port".  The first is the OpenFlow
-ingress port, essentially a physical port identifier.  This is
-implemented as part of ovs-vswitchd's OpenFlow implementation.  It
-prevents a reply from being sent across the tunnel on which it
-arrived.  It is questionable whether this OpenFlow feature is useful
-to OVN.  (OVN already has to override it to allow a packet from one
-nested container to be forwarded to a different nested container.)
-OVS make it possible to disable this feature of OpenFlow by setting
-the OpenFlow input port field to 0.  (If one does this too early, of
-course, it means that there's no way to actually match on the input
-port in the OpenFlow flow tables, but one can work around that by
-instead setting the input port just before the output action, possibly
-wrapping these actions in push/pop pairs to preserve the input port
-for later.)
-
-The second is the OVN logical ingress port, which is implemented in
-ovn-controller as part of the logical abstraction, using an OVS
-register.  Dropping packets directed to the logical ingress port is
-implemented through an OpenFlow table not directly visible to the
-logical flow table.  Currently this behavior can't be disabled, but
-various ways to ensure it could be implemented, e.g. the same as for
-OpenFlow by allowing the logical inport to be zeroed, or by
-introducing a new action that ignores the inport.
-
 ** New OVN logical actions
 
 *** arp
diff --git a/ovn/lib/expr.c b/ovn/lib/expr.c
index 8a69e3e..a970b12 100644
--- a/ovn/lib/expr.c
+++ b/ovn/lib/expr.c
@@ -2812,6 +2812,16 @@  parse_assignment(struct expr_context *ctx, const struct simap *ports,
                         sf->field->n_bytes, 0, sf->field->n_bits);
             bitwise_put(UINT64_MAX, &sf->mask,
                         sf->field->n_bytes, 0, sf->field->n_bits);
+
+            /* If the logical input port is being zeroed, clear the OpenFlow
+             * ingress port also, to allow a packet to be sent back to its
+             * origin. */
+            if (!port && sf->field->id == MFF_REG6) {
+                sf = ofpact_put_SET_FIELD(ofpacts);
+                sf->field = mf_from_id(MFF_IN_PORT);
+                bitwise_put(UINT64_MAX, &sf->mask, sf->field->n_bytes, 0,
+                            sf->field->n_bits);
+            }
         }
 
     exit_destroy_cs:
diff --git a/ovn/ovn-sb.xml b/ovn/ovn-sb.xml
index 1d9104e..9c2d411 100644
--- a/ovn/ovn-sb.xml
+++ b/ovn/ovn-sb.xml
@@ -782,7 +782,11 @@ 
           <p>
             Output to the input port is implicitly dropped, that is,
             <code>output</code> becomes a no-op if <code>outport</code> ==
-            <code>inport</code>.
+            <code>inport</code>.  Occasionally it may be useful to override
+            this behavior, e.g. to send an ARP reply to an ARP request; to do
+            so, use <code>inport = "";</code> to set the logical input port to
+            an empty string (which should not be used as the name of any
+            logical port).
           </p>
         </dd>