diff mbox

[ovs-dev,v4,1/2] ovn-controller: Tie OpenFlow and logical flows using OpenFlow cookie.

Message ID 20161221232539.13688-2-blp@ovn.org
State Accepted
Headers show

Commit Message

Ben Pfaff Dec. 21, 2016, 11:25 p.m. UTC 
This makes it easy to find the logical flow that generated a particular
OpenFlow flow, by running "ovn-sbctl dump-flows <cookie>".

Later, this can be refined (and automated for "ofproto/trace"), but this
is still a significant advance.

Signed-off-by: Ben Pfaff <blp@ovn.org>
---
 lib/uuid.c                   |  22 +++++++-
 lib/uuid.h                   |   3 +-
 ovn/controller/lflow.c       |   6 +-
 ovn/controller/ofctrl.c      |   6 +-
 ovn/controller/ofctrl.h      |   2 +-
 ovn/controller/physical.c    |  38 +++++++------
 ovn/utilities/ovn-sbctl.8.in |  13 ++++-
 ovn/utilities/ovn-sbctl.c    | 129 ++++++++++++++++++++++++++++++++++++-------
 8 files changed, 172 insertions(+), 47 deletions(-)

Comments

Justin Pettit Dec. 28, 2016, 6:28 a.m. UTC | #1 
> On Dec 21, 2016, at 3:25 PM, Ben Pfaff <blp@ovn.org> wrote:
> 
> diff --git a/ovn/controller/ofctrl.c b/ovn/controller/ofctrl.c
> index 1d8fbf3..7449293 100644
> --- a/ovn/controller/ofctrl.c
> +++ b/ovn/controller/ofctrl.c
> @@ -57,6 +57,7 @@ struct ovn_flow {
>     /* Data. */
>     struct ofpact *ofpacts;
>     size_t ofpacts_len;
> +    uint64_t cookie;
> };
> 
> static uint32_t ovn_flow_hash(const struct ovn_flow *);
> @@ -602,7 +603,8 @@ ofctrl_recv(const struct ofp_header *oh, enum ofptype type)
> void
> ofctrl_add_flow(struct hmap *desired_flows,
>                 uint8_t table_id, uint16_t priority,
> -                const struct match *match, const struct ofpbuf *actions)
> +                const struct match *match, uint64_t cookie,
> +                const struct ofpbuf *actions)
> {

All the other arguments for ofctrl_add_flow() are documented in the comment above it, so it would be nice to add 'cookie'.

This is really minor, but all the metadata for the flow is before 'match', so it might be more consistent to put 'cookie' there.

> diff --git a/ovn/controller/physical.c b/ovn/controller/physical.c
> index 9d37410..a84671e 100644
> --- a/ovn/controller/physical.c
> +++ b/ovn/controller/physical.c
> 
> @@ -935,7 +937,8 @@ physical_run(struct controller_ctx *ctx, enum mf_field_id mff_ovn_geneve,
> 
>     /* Resubmit to table 33. */
>     put_resubmit(OFTABLE_LOCAL_OUTPUT, &ofpacts);
> -    ofctrl_add_flow(flow_table, OFTABLE_REMOTE_OUTPUT, 150, &match, &ofpacts);
> +    ofctrl_add_flow(flow_table, OFTABLE_REMOTE_OUTPUT, 150,
> +                    &match, 0, &ofpacts);

It might be nice to document in ovn-architecture how cookies are used and defined--particularly on when they are "0" versus having a value (and how the value was defined).  (Thinking more about it, maybe it belongs in the ovn-controller man page, since it's really an implementation detail.)

> diff --git a/ovn/utilities/ovn-sbctl.c b/ovn/utilities/ovn-sbctl.c
> index 92ae3e5..2c9c4a1 100644
> --- a/ovn/utilities/ovn-sbctl.c
> +++ b/ovn/utilities/ovn-sbctl.c
> ...
> +static const struct sbrec_datapath_binding *
> +lookup_datapath(struct ovsdb_idl *idl, const char *s)
> +{
> +    const struct sbrec_datapath_binding *datapath;
> +
> +    struct uuid uuid;
> +    if (uuid_from_string(&uuid, s)) {
> +        datapath = sbrec_datapath_binding_get_for_uuid(idl, &uuid);
> +        if (datapath) {
> +            return datapath;
> +        }
> +    }
> +
> +    SBREC_DATAPATH_BINDING_FOR_EACH (datapath, idl) {
> +        const char *name = smap_get(&datapath->external_ids, "name");
> +        if (name && !strcmp(name, s)) {
> +            return datapath;
> +        }
> +    }
> +
> +    return NULL;
> +}

I don't think there's any requirement that the datapath name be unique, so it may be worth noting here and in the documentation that if there are multiple datapaths with the same name, it will only choose one at random.

> static void
> cmd_lflow_list(struct ctl_context *ctx)
> {

I'd think it would be helpful to provide an option to print the first 32-bits of the logical flow's UUID when calling "ovn-sbctl lflow-list".  That way, there's a pretty clear way to look for those matching flows from "ovs-ofctl dump-flows" without have to go to lower level commands.

Signed-off-by: Justin Pettit <jpettit@ovn.org>

--Justin
Ben Pfaff Dec. 28, 2016, 5:27 p.m. UTC | #2 
On Tue, Dec 27, 2016 at 10:28:18PM -0800, Justin Pettit wrote:
> 
> > On Dec 21, 2016, at 3:25 PM, Ben Pfaff <blp@ovn.org> wrote:
> > 
> > diff --git a/ovn/controller/ofctrl.c b/ovn/controller/ofctrl.c
> > index 1d8fbf3..7449293 100644
> > --- a/ovn/controller/ofctrl.c
> > +++ b/ovn/controller/ofctrl.c
> > @@ -57,6 +57,7 @@ struct ovn_flow {
> >     /* Data. */
> >     struct ofpact *ofpacts;
> >     size_t ofpacts_len;
> > +    uint64_t cookie;
> > };
> > 
> > static uint32_t ovn_flow_hash(const struct ovn_flow *);
> > @@ -602,7 +603,8 @@ ofctrl_recv(const struct ofp_header *oh, enum ofptype type)
> > void
> > ofctrl_add_flow(struct hmap *desired_flows,
> >                 uint8_t table_id, uint16_t priority,
> > -                const struct match *match, const struct ofpbuf *actions)
> > +                const struct match *match, uint64_t cookie,
> > +                const struct ofpbuf *actions)
> > {
> 
> All the other arguments for ofctrl_add_flow() are documented in the
> comment above it, so it would be nice to add 'cookie'.
> 
> This is really minor, but all the metadata for the flow is before
> 'match', so it might be more consistent to put 'cookie' there.

OK, I made those changes.

> > diff --git a/ovn/controller/physical.c b/ovn/controller/physical.c
> > index 9d37410..a84671e 100644
> > --- a/ovn/controller/physical.c
> > +++ b/ovn/controller/physical.c
> > 
> > @@ -935,7 +937,8 @@ physical_run(struct controller_ctx *ctx, enum mf_field_id mff_ovn_geneve,
> > 
> >     /* Resubmit to table 33. */
> >     put_resubmit(OFTABLE_LOCAL_OUTPUT, &ofpacts);
> > -    ofctrl_add_flow(flow_table, OFTABLE_REMOTE_OUTPUT, 150, &match, &ofpacts);
> > +    ofctrl_add_flow(flow_table, OFTABLE_REMOTE_OUTPUT, 150,
> > +                    &match, 0, &ofpacts);
> 
> It might be nice to document in ovn-architecture how cookies are used
> and defined--particularly on when they are "0" versus having a value
> (and how the value was defined).  (Thinking more about it, maybe it
> belongs in the ovn-controller man page, since it's really an
> implementation detail.)

This was described pretty well in ovn-trace(8) in the second patch, so I
moved the appropriate bits of it to ovn-architecture(7) and changed
ovn-trace(8) to refer back to ovn-architecture(7).

> > diff --git a/ovn/utilities/ovn-sbctl.c b/ovn/utilities/ovn-sbctl.c
> > index 92ae3e5..2c9c4a1 100644
> > --- a/ovn/utilities/ovn-sbctl.c
> > +++ b/ovn/utilities/ovn-sbctl.c
> > ...
> > +static const struct sbrec_datapath_binding *
> > +lookup_datapath(struct ovsdb_idl *idl, const char *s)
> > +{
> > +    const struct sbrec_datapath_binding *datapath;
> > +
> > +    struct uuid uuid;
> > +    if (uuid_from_string(&uuid, s)) {
> > +        datapath = sbrec_datapath_binding_get_for_uuid(idl, &uuid);
> > +        if (datapath) {
> > +            return datapath;
> > +        }
> > +    }
> > +
> > +    SBREC_DATAPATH_BINDING_FOR_EACH (datapath, idl) {
> > +        const char *name = smap_get(&datapath->external_ids, "name");
> > +        if (name && !strcmp(name, s)) {
> > +            return datapath;
> > +        }
> > +    }
> > +
> > +    return NULL;
> > +}
> 
> I don't think there's any requirement that the datapath name be
> unique, so it may be worth noting here and in the documentation that
> if there are multiple datapaths with the same name, it will only
> choose one at random.

I changed this to give an error if there are multiple matches.

> > static void
> > cmd_lflow_list(struct ctl_context *ctx)
> > {
> 
> I'd think it would be helpful to provide an option to print the first
> 32-bits of the logical flow's UUID when calling "ovn-sbctl
> lflow-list".  That way, there's a pretty clear way to look for those
> matching flows from "ovs-ofctl dump-flows" without have to go to lower
> level commands.

OK, I added a --uuid option.

> Signed-off-by: Justin Pettit <jpettit@ovn.org>

Thanks, I converted this to an Acked-by.

I'll apply this in a few minutes.
diff mbox

Patch

diff --git a/lib/uuid.c b/lib/uuid.c
index 0f2a58e..bd98d40 100644
--- a/lib/uuid.c
+++ b/lib/uuid.c
@@ -1,4 +1,4 @@ 
-/* Copyright (c) 2008, 2009, 2010, 2011, 2013 Nicira, Inc.
+/* Copyright (c) 2008, 2009, 2010, 2011, 2013, 2016 Nicira, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -210,6 +210,26 @@  error:
     uuid_zero(uuid);
     return false;
 }
+
+/* Returns the number of characters at the beginning of 's' that are valid for
+ * a UUID.  For example, the "123" at the beginning of "123xyzzy" could begin a
+ * UUID, so uuid_is_partial_string() would return 3; for "xyzzy", this function
+ * would return 0, since "x" can't start a UUID. */
+int
+uuid_is_partial_string(const char *s)
+{
+    static const char tmpl[UUID_LEN] = "xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx";
+    size_t i;
+    for (i = 0; i < UUID_LEN; i++) {
+        if (tmpl[i] == 'x'
+            ? hexit_value(s[i]) < 0
+            : s[i] != '-') {
+            break;
+        }
+    }
+    return i;
+}
+
 
 static void
 sha1_update_int(struct sha1_ctx *sha1_ctx, uintmax_t x)
diff --git a/lib/uuid.h b/lib/uuid.h
index a5792b0..113574c 100644
--- a/lib/uuid.h
+++ b/lib/uuid.h
@@ -1,4 +1,4 @@ 
-/* Copyright (c) 2008, 2009, 2010 Nicira, Inc.
+/* Copyright (c) 2008, 2009, 2010, 2016 Nicira, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -61,6 +61,7 @@  bool uuid_is_zero(const struct uuid *);
 int uuid_compare_3way(const struct uuid *, const struct uuid *);
 bool uuid_from_string(struct uuid *, const char *);
 bool uuid_from_string_prefix(struct uuid *, const char *);
+int uuid_is_partial_string(const char *);
 void uuid_set_bits_v4(struct uuid *);
 
 #endif /* uuid.h */
diff --git a/ovn/controller/lflow.c b/ovn/controller/lflow.c
index d913998..e3c99ad 100644
--- a/ovn/controller/lflow.c
+++ b/ovn/controller/lflow.c
@@ -266,7 +266,7 @@  consider_logical_flow(const struct lport_index *lports,
         }
         if (!m->n) {
             ofctrl_add_flow(flow_table, ptable, lflow->priority, &m->match,
-                            &ofpacts);
+                            lflow->header_.uuid.parts[0], &ofpacts);
         } else {
             uint64_t conj_stubs[64 / 8];
             struct ofpbuf conj;
@@ -282,7 +282,7 @@  consider_logical_flow(const struct lport_index *lports,
                 dst->n_clauses = src->n_clauses;
             }
             ofctrl_add_flow(flow_table, ptable, lflow->priority, &m->match,
-                            &conj);
+                            0, &conj);
             ofpbuf_uninit(&conj);
         }
     }
@@ -350,7 +350,7 @@  consider_neighbor_flow(const struct lport_index *lports,
     uint64_t stub[1024 / 8];
     struct ofpbuf ofpacts = OFPBUF_STUB_INITIALIZER(stub);
     put_load(mac.ea, sizeof mac.ea, MFF_ETH_DST, 0, 48, &ofpacts);
-    ofctrl_add_flow(flow_table, OFTABLE_MAC_BINDING, 100, &match, &ofpacts);
+    ofctrl_add_flow(flow_table, OFTABLE_MAC_BINDING, 100, &match, 0, &ofpacts);
     ofpbuf_uninit(&ofpacts);
 }
 
diff --git a/ovn/controller/ofctrl.c b/ovn/controller/ofctrl.c
index 1d8fbf3..7449293 100644
--- a/ovn/controller/ofctrl.c
+++ b/ovn/controller/ofctrl.c
@@ -57,6 +57,7 @@  struct ovn_flow {
     /* Data. */
     struct ofpact *ofpacts;
     size_t ofpacts_len;
+    uint64_t cookie;
 };
 
 static uint32_t ovn_flow_hash(const struct ovn_flow *);
@@ -602,7 +603,8 @@  ofctrl_recv(const struct ofp_header *oh, enum ofptype type)
 void
 ofctrl_add_flow(struct hmap *desired_flows,
                 uint8_t table_id, uint16_t priority,
-                const struct match *match, const struct ofpbuf *actions)
+                const struct match *match, uint64_t cookie,
+                const struct ofpbuf *actions)
 {
     struct ovn_flow *f = xmalloc(sizeof *f);
     f->table_id = table_id;
@@ -611,6 +613,7 @@  ofctrl_add_flow(struct hmap *desired_flows,
     f->ofpacts = xmemdup(actions->data, actions->size);
     f->ofpacts_len = actions->size;
     f->hmap_node.hash = ovn_flow_hash(f);
+    f->cookie = cookie;
 
     if (ovn_flow_lookup(desired_flows, f)) {
         static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 5);
@@ -935,6 +938,7 @@  ofctrl_put(struct hmap *flow_table, struct shash *pending_ct_zones,
             .table_id = d->table_id,
             .ofpacts = d->ofpacts,
             .ofpacts_len = d->ofpacts_len,
+            .new_cookie = htonll(d->cookie),
             .command = OFPFC_ADD,
         };
         add_flow_mod(&fm, &msgs);
diff --git a/ovn/controller/ofctrl.h b/ovn/controller/ofctrl.h
index 5308b61..67d6c59 100644
--- a/ovn/controller/ofctrl.h
+++ b/ovn/controller/ofctrl.h
@@ -46,7 +46,7 @@  void ofctrl_ct_flush_zone(uint16_t zone_id);
 /* Flow table interfaces to the rest of ovn-controller. */
 void ofctrl_add_flow(struct hmap *desired_flows, uint8_t table_id,
                      uint16_t priority, const struct match *,
-                     const struct ofpbuf *ofpacts);
+                     uint64_t cookie, const struct ofpbuf *ofpacts);
 
 void ofctrl_flow_table_clear(void);
 
diff --git a/ovn/controller/physical.c b/ovn/controller/physical.c
index 9d37410..a84671e 100644
--- a/ovn/controller/physical.c
+++ b/ovn/controller/physical.c
@@ -219,7 +219,7 @@  put_local_common_flows(uint32_t dp_key, uint32_t port_key,
     /* Resubmit to table 34. */
     put_resubmit(OFTABLE_CHECK_LOOPBACK, ofpacts_p);
     ofctrl_add_flow(flow_table, OFTABLE_LOCAL_OUTPUT, 100,
-                    &match, ofpacts_p);
+                    &match, 0, ofpacts_p);
 
     /* Table 34, Priority 100.
      * =======================
@@ -234,7 +234,7 @@  put_local_common_flows(uint32_t dp_key, uint32_t port_key,
     match_set_reg(&match, MFF_LOG_INPORT - MFF_REG0, port_key);
     match_set_reg(&match, MFF_LOG_OUTPORT - MFF_REG0, port_key);
     ofctrl_add_flow(flow_table, OFTABLE_CHECK_LOOPBACK, 100,
-                    &match, ofpacts_p);
+                    &match, 0, ofpacts_p);
 
     /* Table 64, Priority 100.
      * =======================
@@ -257,7 +257,8 @@  put_local_common_flows(uint32_t dp_key, uint32_t port_key,
     put_load(0, MFF_IN_PORT, 0, 16, ofpacts_p);
     put_resubmit(OFTABLE_LOG_TO_PHY, ofpacts_p);
     put_stack(MFF_IN_PORT, ofpact_put_STACK_POP(ofpacts_p));
-    ofctrl_add_flow(flow_table, OFTABLE_SAVE_INPORT, 100, &match, ofpacts_p);
+    ofctrl_add_flow(flow_table, OFTABLE_SAVE_INPORT, 100,
+                    &match, 0, ofpacts_p);
 }
 
 static void
@@ -347,7 +348,7 @@  consider_port_binding(enum mf_field_id mff_ovn_geneve,
         ofpact_finish_CLONE(ofpacts_p, &clone);
 
         ofctrl_add_flow(flow_table, OFTABLE_LOG_TO_PHY, 100,
-                        &match, ofpacts_p);
+                        &match, 0, ofpacts_p);
         return;
     }
 
@@ -471,7 +472,7 @@  consider_port_binding(enum mf_field_id mff_ovn_geneve,
         /* Resubmit to first logical ingress pipeline table. */
         put_resubmit(OFTABLE_LOG_INGRESS_PIPELINE, ofpacts_p);
         ofctrl_add_flow(flow_table, OFTABLE_PHY_TO_LOG,
-                        tag ? 150 : 100, &match, ofpacts_p);
+                        tag ? 150 : 100, &match, 0, ofpacts_p);
 
         if (!tag && (!strcmp(binding->type, "localnet")
                      || !strcmp(binding->type, "l2gateway"))) {
@@ -481,7 +482,7 @@  consider_port_binding(enum mf_field_id mff_ovn_geneve,
              * action. */
             ofpbuf_pull(ofpacts_p, ofpacts_orig_size);
             match_set_dl_tci_masked(&match, 0, htons(VLAN_CFI));
-            ofctrl_add_flow(flow_table, 0, 100, &match, ofpacts_p);
+            ofctrl_add_flow(flow_table, 0, 100, &match, 0, ofpacts_p);
         }
 
         /* Table 65, Priority 100.
@@ -509,7 +510,7 @@  consider_port_binding(enum mf_field_id mff_ovn_geneve,
             ofpact_put_STRIP_VLAN(ofpacts_p);
         }
         ofctrl_add_flow(flow_table, OFTABLE_LOG_TO_PHY, 100,
-                        &match, ofpacts_p);
+                        &match, 0, ofpacts_p);
     } else if (!tun) {
         /* Remote port connected by localnet port */
         /* Table 33, priority 100.
@@ -532,7 +533,7 @@  consider_port_binding(enum mf_field_id mff_ovn_geneve,
         /* Resubmit to table 33. */
         put_resubmit(OFTABLE_LOCAL_OUTPUT, ofpacts_p);
         ofctrl_add_flow(flow_table, OFTABLE_LOCAL_OUTPUT, 100,
-                        &match, ofpacts_p);
+                        &match, 0, ofpacts_p);
     } else {
         /* Remote port connected by tunnel */
 
@@ -556,7 +557,7 @@  consider_port_binding(enum mf_field_id mff_ovn_geneve,
         /* Output to tunnel. */
         ofpact_put_OUTPUT(ofpacts_p)->port = ofport;
         ofctrl_add_flow(flow_table, OFTABLE_REMOTE_OUTPUT, 100,
-                        &match, ofpacts_p);
+                        &match, 0, ofpacts_p);
     }
 }
 
@@ -644,7 +645,7 @@  consider_mc_group(enum mf_field_id mff_ovn_geneve,
         put_load(mc->tunnel_key, MFF_LOG_OUTPORT, 0, 32, ofpacts_p);
 
         ofctrl_add_flow(flow_table, OFTABLE_LOCAL_OUTPUT, 100,
-                        &match, ofpacts_p);
+                        &match, 0, ofpacts_p);
     }
 
     /* Table 32, priority 100.
@@ -682,7 +683,7 @@  consider_mc_group(enum mf_field_id mff_ovn_geneve,
                 put_resubmit(OFTABLE_LOCAL_OUTPUT, remote_ofpacts_p);
             }
             ofctrl_add_flow(flow_table, OFTABLE_REMOTE_OUTPUT, 100,
-                            &match, remote_ofpacts_p);
+                            &match, 0, remote_ofpacts_p);
         }
     }
     sset_destroy(&remote_chassis);
@@ -882,7 +883,8 @@  physical_run(struct controller_ctx *ctx, enum mf_field_id mff_ovn_geneve,
 
         put_resubmit(OFTABLE_LOCAL_OUTPUT, &ofpacts);
 
-        ofctrl_add_flow(flow_table, OFTABLE_PHY_TO_LOG, 100, &match, &ofpacts);
+        ofctrl_add_flow(flow_table, OFTABLE_PHY_TO_LOG, 100, &match, 0,
+                        &ofpacts);
     }
 
     /* Add flows for VXLAN encapsulations.  Due to the limited amount of
@@ -914,7 +916,7 @@  physical_run(struct controller_ctx *ctx, enum mf_field_id mff_ovn_geneve,
             put_load(1, MFF_LOG_FLAGS, MLF_RCV_FROM_VXLAN_BIT, 1, &ofpacts);
             put_resubmit(OFTABLE_LOG_INGRESS_PIPELINE, &ofpacts);
 
-            ofctrl_add_flow(flow_table, OFTABLE_PHY_TO_LOG, 100, &match,
+            ofctrl_add_flow(flow_table, OFTABLE_PHY_TO_LOG, 100, &match, 0,
                             &ofpacts);
         }
     }
@@ -935,7 +937,8 @@  physical_run(struct controller_ctx *ctx, enum mf_field_id mff_ovn_geneve,
 
     /* Resubmit to table 33. */
     put_resubmit(OFTABLE_LOCAL_OUTPUT, &ofpacts);
-    ofctrl_add_flow(flow_table, OFTABLE_REMOTE_OUTPUT, 150, &match, &ofpacts);
+    ofctrl_add_flow(flow_table, OFTABLE_REMOTE_OUTPUT, 150,
+                    &match, 0, &ofpacts);
 
     /* Table 32, Priority 0.
      * =======================
@@ -945,7 +948,7 @@  physical_run(struct controller_ctx *ctx, enum mf_field_id mff_ovn_geneve,
     match_init_catchall(&match);
     ofpbuf_clear(&ofpacts);
     put_resubmit(OFTABLE_LOCAL_OUTPUT, &ofpacts);
-    ofctrl_add_flow(flow_table, OFTABLE_REMOTE_OUTPUT, 0, &match, &ofpacts);
+    ofctrl_add_flow(flow_table, OFTABLE_REMOTE_OUTPUT, 0, &match, 0, &ofpacts);
 
     /* Table 34, Priority 0.
      * =======================
@@ -959,7 +962,8 @@  physical_run(struct controller_ctx *ctx, enum mf_field_id mff_ovn_geneve,
         put_load(0, MFF_REG0 + i, 0, 32, &ofpacts);
     }
     put_resubmit(OFTABLE_LOG_EGRESS_PIPELINE, &ofpacts);
-    ofctrl_add_flow(flow_table, OFTABLE_CHECK_LOOPBACK, 0, &match, &ofpacts);
+    ofctrl_add_flow(flow_table, OFTABLE_CHECK_LOOPBACK, 0, &match, 0,
+                    &ofpacts);
 
     /* Table 64, Priority 0.
      * =======================
@@ -969,7 +973,7 @@  physical_run(struct controller_ctx *ctx, enum mf_field_id mff_ovn_geneve,
     match_init_catchall(&match);
     ofpbuf_clear(&ofpacts);
     put_resubmit(OFTABLE_LOG_TO_PHY, &ofpacts);
-    ofctrl_add_flow(flow_table, OFTABLE_SAVE_INPORT, 0, &match, &ofpacts);
+    ofctrl_add_flow(flow_table, OFTABLE_SAVE_INPORT, 0, &match, 0, &ofpacts);
 
     ofpbuf_uninit(&ofpacts);
 
diff --git a/ovn/utilities/ovn-sbctl.8.in b/ovn/utilities/ovn-sbctl.8.in
index 783afc7..3e547f8 100644
--- a/ovn/utilities/ovn-sbctl.8.in
+++ b/ovn/utilities/ovn-sbctl.8.in
@@ -160,9 +160,18 @@  to unbind logical port that is not bound has no effect.
 .
 .SS "Logical Flow Commands"
 .
-.IP "\fBlflow\-list\fR [\fIlogical-datapath\fR]"
+.IP "\fBlflow\-list\fR [\fIlogical-datapath\fR] [\fIlflow\fR...]"
 List logical flows.  If \fIlogical-datapath\fR is specified, only list
-flows for that logical datapath.
+flows for that logical datapath.  The \fIlogical-datapath\fR may be
+given as a UUID or as a datapath name.
+.IP
+If at least one \fIlflow\fR is given, only matching logical flows, if
+any, are listed.  Each \fIlflow\fR may be specified as a UUID or the
+first few characters of a UUID, optionally prefixed by \fB0x\fR.
+(Because \fBovn\-controller\fR sets OpenFlow flow cookies to the first
+32 bits of the corresponding logical flow's UUID, this makes it easy
+to look up the logical flow that generated a particular OpenFlow
+flow.)
 .
 .IP "\fBdump\-flows\fR [\fIlogical-datapath\fR]"
 Alias for \fBlflow\-list\fB.
diff --git a/ovn/utilities/ovn-sbctl.c b/ovn/utilities/ovn-sbctl.c
index 92ae3e5..2c9c4a1 100644
--- a/ovn/utilities/ovn-sbctl.c
+++ b/ovn/utilities/ovn-sbctl.c
@@ -305,8 +305,8 @@  Port binding commands:\n\
   lsp-unbind PORT             reset the port binding of logical port PORT\n\
 \n\
 Logical flow commands:\n\
-  lflow-list [DATAPATH]       List logical flows for all or a single datapath\n\
-  dump-flows [DATAPATH]       Alias for lflow-list\n\
+  lflow-list [DATAPATH] [LFLOW...] List logical flows for DATAPATH\n\
+  dump-flows [DATAPATH] [LFLOW...] Alias for lflow-list\n\
 \n\
 Connection commands:\n\
   get-connection             print the connections\n\
@@ -696,53 +696,138 @@  lflow_cmp(const void *lf1_, const void *lf2_)
     return 0;
 }
 
+static char *
+parse_partial_uuid(char *s)
+{
+    /* Accept a full or partial UUID. */
+    if (uuid_is_partial_string(s) == strlen(s)) {
+        return s;
+    }
+
+    /* Accept a full or partial UUID prefixed by 0x, since "ovs-ofctl
+     * dump-flows" prints cookies prefixed by 0x. */
+    if (s[0] == '0' && (s[1] == 'x' || s[1] == 'X')
+        && uuid_is_partial_string(s + 2) == strlen(s + 2)) {
+        return s + 2;
+    }
+
+    /* Not a (partial) UUID. */
+    return NULL;
+}
+
+static bool
+is_partial_uuid_match(const struct uuid *uuid, const char *match)
+{
+    char uuid_s[UUID_LEN + 1];
+    snprintf(uuid_s, sizeof uuid_s, UUID_FMT, UUID_ARGS(uuid));
+
+    return !strncmp(uuid_s, match, strlen(match));
+}
+
+static const struct sbrec_datapath_binding *
+lookup_datapath(struct ovsdb_idl *idl, const char *s)
+{
+    const struct sbrec_datapath_binding *datapath;
+
+    struct uuid uuid;
+    if (uuid_from_string(&uuid, s)) {
+        datapath = sbrec_datapath_binding_get_for_uuid(idl, &uuid);
+        if (datapath) {
+            return datapath;
+        }
+    }
+
+    SBREC_DATAPATH_BINDING_FOR_EACH (datapath, idl) {
+        const char *name = smap_get(&datapath->external_ids, "name");
+        if (name && !strcmp(name, s)) {
+            return datapath;
+        }
+    }
+
+    return NULL;
+}
+
 static void
 cmd_lflow_list(struct ctl_context *ctx)
 {
-    const char *datapath = ctx->argc == 2 ? ctx->argv[1] : NULL;
-    struct uuid datapath_uuid = { .parts = { 0, }};
-    const struct sbrec_logical_flow **lflows;
-    const struct sbrec_logical_flow *lflow;
-    size_t n_flows = 0, n_capacity = 64;
+    const struct sbrec_datapath_binding *datapath = NULL;
+    if (ctx->argc > 1) {
+        datapath = lookup_datapath(ctx->idl, ctx->argv[1]);
+        if (datapath) {
+            ctx->argc--;
+            ctx->argv++;
+        }
+    }
 
-    if (datapath && !uuid_from_string(&datapath_uuid, datapath)) {
-        VLOG_ERR("Invalid format of datapath UUID");
-        return;
+    for (size_t i = 1; i < ctx->argc; i++) {
+        char *s = parse_partial_uuid(ctx->argv[i]);
+        if (!s) {
+            ctl_fatal("%s is not a UUID or the beginning of a UUID",
+                      ctx->argv[i]);
+        }
+        ctx->argv[i] = s;
     }
 
-    lflows = xmalloc(sizeof *lflows * n_capacity);
+    const struct sbrec_logical_flow **lflows = NULL;
+    size_t n_flows = 0;
+    size_t n_capacity = 0;
+    const struct sbrec_logical_flow *lflow;
     SBREC_LOGICAL_FLOW_FOR_EACH (lflow, ctx->idl) {
+        if (datapath && lflow->logical_datapath != datapath) {
+            continue;
+        }
+
         if (n_flows == n_capacity) {
             lflows = x2nrealloc(lflows, &n_capacity, sizeof *lflows);
         }
         lflows[n_flows] = lflow;
         n_flows++;
     }
-
     qsort(lflows, n_flows, sizeof *lflows, lflow_cmp);
 
-    const char *cur_pipeline = "";
-    size_t i;
-    for (i = 0; i < n_flows; i++) {
+    const struct sbrec_logical_flow *prev = NULL;
+    for (size_t i = 0; i < n_flows; i++) {
         lflow = lflows[i];
-        if (datapath && !uuid_equals(&datapath_uuid,
-                                     &lflow->logical_datapath->header_.uuid)) {
+
+        /* Figure out whether to print this particular flow.  By default, we
+         * print all flows, but if any UUIDs were listed on the command line
+         * then we only print the matching ones. */
+        bool include;
+        if (ctx->argc > 1) {
+            include = false;
+            for (size_t j = 1; j < ctx->argc; j++) {
+                if (is_partial_uuid_match(&lflow->header_.uuid,
+                                          ctx->argv[j])) {
+                    include = true;
+                    break;
+                }
+            }
+        } else {
+            include = true;
+        }
+        if (!include) {
             continue;
         }
-        if (strcmp(cur_pipeline, lflow->pipeline)) {
+
+        /* Print a header line for this datapath or pipeline, if we haven't
+         * already done so. */
+        if (!prev
+            || prev->logical_datapath != lflow->logical_datapath
+            || strcmp(prev->pipeline, lflow->pipeline)) {
             printf("Datapath: \"%s\" ("UUID_FMT")  Pipeline: %s\n",
                    smap_get_def(&lflow->logical_datapath->external_ids,
                                 "name", ""),
                    UUID_ARGS(&lflow->logical_datapath->header_.uuid),
                    lflow->pipeline);
-            cur_pipeline = lflow->pipeline;
         }
 
+        /* Print the flow. */
         printf("  table=%-2" PRId64 "(%-19s), priority=%-5" PRId64
                ", match=(%s), action=(%s)\n",
                lflow->table_id,
                smap_get_def(&lflow->external_ids, "stage-name", ""),
                lflow->priority, lflow->match, lflow->actions);
+        prev = lflow;
     }
 
     free(lflows);
@@ -1244,9 +1329,11 @@  static const struct ctl_command_syntax sbctl_commands[] = {
      "--if-exists", RW},
 
     /* Logical flow commands */
-    {"lflow-list", 0, 1, "[DATAPATH]", pre_get_info, cmd_lflow_list, NULL,
+    {"lflow-list", 0, INT_MAX, "[DATAPATH] [LFLOW...]",
+     pre_get_info, cmd_lflow_list, NULL,
      "", RO},
-    {"dump-flows", 0, 1, "[DATAPATH]", pre_get_info, cmd_lflow_list, NULL,
+    {"dump-flows", 0, INT_MAX, "[DATAPATH] [LFLOW...]",
+     pre_get_info, cmd_lflow_list, NULL,
      "", RO}, /* Friendly alias for lflow-list */
 
     /* Connection commands. */