[ovs-dev] [PATCHv2] dpif: Support flow_get in dpif_operate().

Joe Stringer joestringer at nicira.com
Wed Aug 13 00:53:31 UTC 2014


This cleans up the dpif interface to make it more consistent with the
other dpif operations, and allows flows to be fetched in batches.

Signed-off-by: Joe Stringer <joestringer at nicira.com>
---
v2: Require callers to pass down an initialized buffer.
    Remove NLM_F_ECHO flag from get request.
    Rebase against master.
RFC: Initial post under "dpif: Allow batching flow_get."
---
 lib/dpif-linux.c              |   85 +++++++++++++++++++++-------------------
 lib/dpif-netdev.c             |   60 ++++++++++++++--------------
 lib/dpif-provider.h           |   31 ---------------
 lib/dpif.c                    |   87 ++++++++++++++++++-----------------------
 lib/dpif.h                    |   33 +++++++++++++++-
 ofproto/ofproto-dpif-upcall.c |    6 ++-
 6 files changed, 148 insertions(+), 154 deletions(-)

diff --git a/lib/dpif-linux.c b/lib/dpif-linux.c
index 6d461b2..e767d9f 100644
--- a/lib/dpif-linux.c
+++ b/lib/dpif-linux.c
@@ -126,6 +126,8 @@ static int dpif_linux_flow_transact(struct dpif_linux_flow *request,
                                     struct ofpbuf **bufp);
 static void dpif_linux_flow_get_stats(const struct dpif_linux_flow *,
                                       struct dpif_flow_stats *);
+static void dpif_linux_flow_to_dpif_flow(struct dpif_flow *,
+                                         const struct dpif_linux_flow *);
 
 /* One of the dpif channels between the kernel and userspace. */
 struct dpif_channel {
@@ -1046,48 +1048,27 @@ dpif_linux_port_poll_wait(const struct dpif *dpif_)
     }
 }
 
-static int
-dpif_linux_flow_get__(const struct dpif_linux *dpif,
-                      const struct nlattr *key, size_t key_len,
-                      struct dpif_linux_flow *reply, struct ofpbuf **bufp)
+static void
+dpif_linux_init_flow_get(const struct dpif_linux *dpif,
+                         const struct nlattr *key, size_t key_len,
+                         struct dpif_linux_flow *request)
 {
-    struct dpif_linux_flow request;
-
-    dpif_linux_flow_init(&request);
-    request.cmd = OVS_FLOW_CMD_GET;
-    request.dp_ifindex = dpif->dp_ifindex;
-    request.key = key;
-    request.key_len = key_len;
-    return dpif_linux_flow_transact(&request, reply, bufp);
+    dpif_linux_flow_init(request);
+    request->cmd = OVS_FLOW_CMD_GET;
+    request->dp_ifindex = dpif->dp_ifindex;
+    request->key = key;
+    request->key_len = key_len;
 }
 
 static int
-dpif_linux_flow_get(const struct dpif *dpif_,
+dpif_linux_flow_get(const struct dpif_linux *dpif,
                     const struct nlattr *key, size_t key_len,
-                    struct ofpbuf **bufp,
-                    struct nlattr **maskp, size_t *mask_len,
-                    struct nlattr **actionsp, size_t *actions_len,
-                    struct dpif_flow_stats *stats)
+                    struct dpif_linux_flow *reply, struct ofpbuf **bufp)
 {
-    const struct dpif_linux *dpif = dpif_linux_cast(dpif_);
-    struct dpif_linux_flow reply;
-    int error;
+    struct dpif_linux_flow request;
 
-    error = dpif_linux_flow_get__(dpif, key, key_len, &reply, bufp);
-    if (!error) {
-        if (maskp) {
-            *maskp = CONST_CAST(struct nlattr *, reply.mask);
-            *mask_len = reply.mask_len;
-        }
-        if (actionsp) {
-            *actionsp = CONST_CAST(struct nlattr *, reply.actions);
-            *actions_len = reply.actions_len;
-        }
-        if (stats) {
-            dpif_linux_flow_get_stats(&reply, stats);
-        }
-    }
-    return error;
+    dpif_linux_init_flow_get(dpif, key, key_len, &request);
+    return dpif_linux_flow_transact(&request, reply, bufp);
 }
 
 static void
@@ -1217,7 +1198,7 @@ dpif_linux_flow_dump_thread_destroy(struct dpif_flow_dump_thread *thread_)
 
 static void
 dpif_linux_flow_to_dpif_flow(struct dpif_flow *dpif_flow,
-                             struct dpif_linux_flow *linux_flow)
+                             const struct dpif_linux_flow *linux_flow)
 {
     dpif_flow->key = linux_flow->key;
     dpif_flow->key_len = linux_flow->key_len;
@@ -1266,9 +1247,9 @@ dpif_linux_flow_dump_next(struct dpif_flow_dump_thread *thread_,
         } else {
             /* Rare case: the flow does not include actions.  Retrieve this
              * individual flow again to get the actions. */
-            error = dpif_linux_flow_get__(dpif, linux_flow.key,
-                                          linux_flow.key_len, &linux_flow,
-                                          &thread->nl_actions);
+            error = dpif_linux_flow_get(dpif, linux_flow.key,
+                                        linux_flow.key_len, &linux_flow,
+                                        &thread->nl_actions);
             if (error == ENOENT) {
                 VLOG_DBG("dumped flow disappeared on get");
                 continue;
@@ -1344,6 +1325,7 @@ dpif_linux_operate__(struct dpif_linux *dpif,
         struct dpif_flow_put *put;
         struct dpif_flow_del *del;
         struct dpif_execute *execute;
+        struct dpif_flow_get *get;
         struct dpif_linux_flow flow;
 
         ofpbuf_use_stub(&aux->request,
@@ -1380,6 +1362,13 @@ dpif_linux_operate__(struct dpif_linux *dpif,
                                       &aux->request);
             break;
 
+        case DPIF_OP_FLOW_GET:
+            get = &op->u.flow_get;
+            dpif_linux_init_flow_get(dpif, get->key, get->key_len, &flow);
+            aux->txn.reply = get->buffer;
+            dpif_linux_flow_to_ofpbuf(&flow, &aux->request);
+            break;
+
         default:
             OVS_NOT_REACHED();
         }
@@ -1396,6 +1385,7 @@ dpif_linux_operate__(struct dpif_linux *dpif,
         struct dpif_op *op = ops[i];
         struct dpif_flow_put *put;
         struct dpif_flow_del *del;
+        struct dpif_flow_get *get;
 
         op->error = txn->error;
 
@@ -1441,6 +1431,22 @@ dpif_linux_operate__(struct dpif_linux *dpif,
         case DPIF_OP_EXECUTE:
             break;
 
+        case DPIF_OP_FLOW_GET:
+            get = &op->u.flow_get;
+            if (!op->error) {
+                struct dpif_linux_flow reply;
+
+                op->error = dpif_linux_flow_from_ofpbuf(&reply, txn->reply);
+                if (!op->error) {
+                    dpif_linux_flow_to_dpif_flow(get->flow, &reply);
+                }
+            }
+
+            if (op->error) {
+                memset(get->flow, 0, sizeof *get->flow);
+            }
+            break;
+
         default:
             OVS_NOT_REACHED();
         }
@@ -1867,7 +1873,6 @@ const struct dpif_class dpif_linux_class = {
     dpif_linux_port_dump_done,
     dpif_linux_port_poll,
     dpif_linux_port_poll_wait,
-    dpif_linux_flow_get,
     dpif_linux_flow_flush,
     dpif_linux_flow_dump_create,
     dpif_linux_flow_dump_destroy,
diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index 0eea3ab..ba54e3b 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -1050,7 +1050,7 @@ dp_netdev_find_flow(const struct dp_netdev *dp, const struct flow *flow)
 }
 
 static void
-get_dpif_flow_stats(struct dp_netdev_flow *netdev_flow,
+get_dpif_flow_stats(const struct dp_netdev_flow *netdev_flow,
                     struct dpif_flow_stats *stats)
 {
     struct dp_netdev_flow_stats *bucket;
@@ -1067,6 +1067,27 @@ get_dpif_flow_stats(struct dp_netdev_flow *netdev_flow,
     }
 }
 
+static void
+dp_netdev_flow_to_dpif_flow(const struct dp_netdev_flow *netdev_flow,
+                            struct ofpbuf *buffer, struct dpif_flow *flow)
+{
+    struct flow_wildcards wc;
+    struct dp_netdev_actions *actions;
+
+    minimask_expand(&netdev_flow->cr.match.mask, &wc);
+    odp_flow_key_from_mask(buffer, &wc.masks, &netdev_flow->flow,
+                           odp_to_u32(wc.masks.in_port.odp_port),
+                           SIZE_MAX, true);
+    flow->mask = ofpbuf_data(buffer);
+    flow->mask_len = ofpbuf_size(buffer);
+
+    actions = dp_netdev_flow_get_actions(netdev_flow);
+    flow->actions = actions->actions;
+    flow->actions_len = actions->size;
+
+    get_dpif_flow_stats(netdev_flow, &flow->stats);
+}
+
 static int
 dpif_netdev_mask_from_nlattrs(const struct nlattr *key, uint32_t key_len,
                               const struct nlattr *mask_key,
@@ -1160,19 +1181,14 @@ dpif_netdev_flow_from_nlattrs(const struct nlattr *key, uint32_t key_len,
 }
 
 static int
-dpif_netdev_flow_get(const struct dpif *dpif,
-                     const struct nlattr *nl_key, size_t nl_key_len,
-                     struct ofpbuf **bufp,
-                     struct nlattr **maskp, size_t *mask_len,
-                     struct nlattr **actionsp, size_t *actions_len,
-                     struct dpif_flow_stats *stats)
+dpif_netdev_flow_get(const struct dpif *dpif, const struct dpif_flow_get *get)
 {
     struct dp_netdev *dp = get_dp_netdev(dpif);
     struct dp_netdev_flow *netdev_flow;
     struct flow key;
     int error;
 
-    error = dpif_netdev_flow_from_nlattrs(nl_key, nl_key_len, &key);
+    error = dpif_netdev_flow_from_nlattrs(get->key, get->key_len, &key);
     if (error) {
         return error;
     }
@@ -1180,28 +1196,7 @@ dpif_netdev_flow_get(const struct dpif *dpif,
     netdev_flow = dp_netdev_find_flow(dp, &key);
 
     if (netdev_flow) {
-        if (stats) {
-            get_dpif_flow_stats(netdev_flow, stats);
-        }
-
-        if (maskp) {
-            struct flow_wildcards wc;
-
-            *bufp = ofpbuf_new(sizeof(struct odputil_keybuf));
-            minimask_expand(&netdev_flow->cr.match.mask, &wc);
-            odp_flow_key_from_mask(*bufp, &wc.masks, &netdev_flow->flow,
-                                   odp_to_u32(wc.masks.in_port.odp_port),
-                                   SIZE_MAX, true);
-            *maskp = ofpbuf_data(*bufp);
-            *mask_len = ofpbuf_size(*bufp);
-        }
-        if (actionsp) {
-            struct dp_netdev_actions *actions;
-
-            actions = dp_netdev_flow_get_actions(netdev_flow);
-            *actionsp = actions->actions;
-            *actions_len = actions->size;
-        }
+        dp_netdev_flow_to_dpif_flow(netdev_flow, get->buffer, get->flow);
      } else {
         error = ENOENT;
     }
@@ -1534,6 +1529,10 @@ dpif_netdev_operate(struct dpif *dpif, struct dpif_op **ops, size_t n_ops)
         case DPIF_OP_EXECUTE:
             op->error = dpif_netdev_execute(dpif, &op->u.execute);
             break;
+
+        case DPIF_OP_FLOW_GET:
+            op->error = dpif_netdev_flow_get(dpif, &op->u.flow_get);
+            break;
         }
     }
 }
@@ -2231,7 +2230,6 @@ const struct dpif_class dpif_netdev_class = {
     dpif_netdev_port_dump_done,
     dpif_netdev_port_poll,
     dpif_netdev_port_poll_wait,
-    dpif_netdev_flow_get,
     dpif_netdev_flow_flush,
     dpif_netdev_flow_dump_create,
     dpif_netdev_flow_dump_destroy,
diff --git a/lib/dpif-provider.h b/lib/dpif-provider.h
index 9f6ef04..7966b83 100644
--- a/lib/dpif-provider.h
+++ b/lib/dpif-provider.h
@@ -239,37 +239,6 @@ struct dpif_class {
      * value other than EAGAIN. */
     void (*port_poll_wait)(const struct dpif *dpif);
 
-    /* Queries 'dpif' for a flow entry.  The flow is specified by the Netlink
-     * attributes with types OVS_KEY_ATTR_* in the 'key_len' bytes starting at
-     * 'key'.
-     *
-     * Returns 0 if successful.  If no flow matches, returns ENOENT.  On other
-     * failure, returns a positive errno value.
-     *
-     * On success, '*bufp' will be set to an ofpbuf owned by the caller that
-     * contains the response for 'maskp' and/or 'actionsp'. The caller must
-     * supply a valid pointer, and must free the ofpbuf (with ofpbuf_delete())
-     * when it is no longer needed.
-     *
-     * If 'maskp' is nonnull, then on success '*maskp' will point to the
-     * Netlink attributes for the flow's mask. '*mask_len' will be set to the
-     * length of the mask attributes. Implementations may opt to point 'maskp'
-     * at RCU-protected data rather than making a copy in '*bufp'.
-     *
-     * If 'actionsp' is nonnull, then on success '*actionsp' will point to the
-     * Netlink attributes for the flow's actions. '*actions_len' will be set to
-     * the length of the actions attributes. Implementations may opt to point
-     * 'actionsp' at RCU-protected data rather than making a copy in '*bufp'.
-     *
-     * If 'stats' is nonnull, then on success it must be updated with the
-     * flow's statistics. */
-    int (*flow_get)(const struct dpif *dpif,
-                    const struct nlattr *key, size_t key_len,
-                    struct ofpbuf **bufp,
-                    struct nlattr **maskp, size_t *mask_len,
-                    struct nlattr **actionsp, size_t *acts_len,
-                    struct dpif_flow_stats *stats);
-
     /* Deletes all flows from 'dpif' and clears all of its queues of received
      * packets. */
     int (*flow_flush)(struct dpif *dpif);
diff --git a/lib/dpif.c b/lib/dpif.c
index 1f15840..8ba889f 100644
--- a/lib/dpif.c
+++ b/lib/dpif.c
@@ -98,6 +98,8 @@ static void log_flow_del_message(struct dpif *, const struct dpif_flow_del *,
                                  int error);
 static void log_execute_message(struct dpif *, const struct dpif_execute *,
                                 bool subexecute, int error);
+static void log_flow_get_message(const struct dpif *,
+                                 const struct dpif_flow_get *, int error);
 
 static void
 dp_initialize(void)
@@ -829,59 +831,27 @@ dpif_flow_flush(struct dpif *dpif)
     return error;
 }
 
-/* Queries 'dpif' for a flow entry.  The flow is specified by the Netlink
- * attributes with types OVS_KEY_ATTR_* in the 'key_len' bytes starting at
- * 'key'.
- *
- * Returns 0 if successful.  If no flow matches, returns ENOENT.  On other
- * failure, returns a positive errno value.
- *
- * On success, '*bufp' will be set to an ofpbuf owned by the caller that
- * contains the response for 'flow->mask' and 'flow->actions'. The caller must
- * supply a valid pointer, and must free the ofpbuf (with ofpbuf_delete()) when
- * it is no longer needed.
- *
- * On success, 'flow' will be populated with the mask, actions and stats for
- * the datapath flow corresponding to 'key'. The mask and actions will point
- * within '*bufp'.
- *
- * Implementations may opt to point 'flow->mask' and/or 'flow->actions' at
- * RCU-protected data rather than making a copy of them. Therefore, callers
- * that wish to hold these over quiescent periods must make a copy of these
- * fields before quiescing. */
+/* A dpif_operate() wrapper for performing a single DPIF_OP_FLOW_GET. */
 int
-dpif_flow_get(const struct dpif *dpif,
+dpif_flow_get(struct dpif *dpif,
               const struct nlattr *key, size_t key_len,
-              struct ofpbuf **bufp, struct dpif_flow *flow)
+              struct ofpbuf *buf, struct dpif_flow *flow)
 {
-    int error;
-    struct nlattr *mask, *actions;
-    size_t mask_len, actions_len;
-    struct dpif_flow_stats stats;
+    struct dpif_op *opp;
+    struct dpif_op op;
 
-    COVERAGE_INC(dpif_flow_get);
+    op.type = DPIF_OP_FLOW_GET;
+    op.u.flow_get.key = key;
+    op.u.flow_get.key_len = key_len;
+    op.u.flow_get.buffer = buf;
+    op.u.flow_get.flow = flow;
+    op.u.flow_get.flow->key = key;
+    op.u.flow_get.flow->key_len = key_len;
 
-    *bufp = NULL;
-    error = dpif->dpif_class->flow_get(dpif, key, key_len, bufp,
-                                       &mask, &mask_len,
-                                       &actions, &actions_len, &stats);
-    if (error) {
-        memset(flow, 0, sizeof *flow);
-        ofpbuf_delete(*bufp);
-        *bufp = NULL;
-    } else {
-        flow->mask = mask;
-        flow->mask_len = mask_len;
-        flow->actions = actions;
-        flow->actions_len = actions_len;
-        flow->stats = stats;
-    }
-    if (should_log_flow_message(error)) {
-        log_flow_message(dpif, error, "flow_get", key, key_len,
-                         NULL, 0, &flow->stats,
-                         flow->actions, flow->actions_len);
-    }
-    return error;
+    opp = &op;
+    dpif_operate(dpif, &opp, 1);
+
+    return op.error;
 }
 
 /* A dpif_operate() wrapper for performing a single DPIF_OP_FLOW_PUT. */
@@ -1177,6 +1147,14 @@ dpif_operate(struct dpif *dpif, struct dpif_op **ops, size_t n_ops)
                     break;
                 }
 
+                case DPIF_OP_FLOW_GET: {
+                    struct dpif_flow_get *get = &op->u.flow_get;
+
+                    COVERAGE_INC(dpif_flow_get);
+                    log_flow_get_message(dpif, get, error);
+                    break;
+                }
+
                 case DPIF_OP_FLOW_DEL: {
                     struct dpif_flow_del *del = &op->u.flow_del;
 
@@ -1574,3 +1552,16 @@ log_execute_message(struct dpif *dpif, const struct dpif_execute *execute,
         free(packet);
     }
 }
+
+static void
+log_flow_get_message(const struct dpif *dpif, const struct dpif_flow_get *get,
+                     int error)
+{
+    if (should_log_flow_message(error)) {
+        log_flow_message(dpif, error, "flow_get",
+                         get->key, get->key_len,
+                         get->flow->mask, get->flow->mask_len,
+                         &get->flow->stats,
+                         get->flow->actions, get->flow->actions_len);
+    }
+}
diff --git a/lib/dpif.h b/lib/dpif.h
index 64fe110..4a21a59 100644
--- a/lib/dpif.h
+++ b/lib/dpif.h
@@ -522,9 +522,9 @@ int dpif_flow_put(struct dpif *, enum dpif_flow_put_flags,
 int dpif_flow_del(struct dpif *,
                   const struct nlattr *key, size_t key_len,
                   struct dpif_flow_stats *);
-int dpif_flow_get(const struct dpif *,
+int dpif_flow_get(struct dpif *,
                   const struct nlattr *key, size_t key_len,
-                  struct ofpbuf **, struct dpif_flow *);
+                  struct ofpbuf *, struct dpif_flow *);
 
 /* Flow dumping interface
  * ======================
@@ -573,6 +573,8 @@ struct dpif_flow {
 };
 int dpif_flow_dump_next(struct dpif_flow_dump_thread *,
                         struct dpif_flow *flows, int max_flows);
+
+#define DPIF_FLOW_BUFSIZE 2048
 
 /* Operation batching interface.
  *
@@ -584,6 +586,7 @@ enum dpif_op_type {
     DPIF_OP_FLOW_PUT = 1,
     DPIF_OP_FLOW_DEL,
     DPIF_OP_EXECUTE,
+    DPIF_OP_FLOW_GET,
 };
 
 /* Add or modify a flow.
@@ -664,6 +667,31 @@ struct dpif_execute {
     struct pkt_metadata md;         /* Packet metadata. */
 };
 
+/* Queries the dpif for a flow entry.
+ *
+ * The flow is specified by the Netlink attributes with types OVS_KEY_ATTR_* in
+ * the 'key_len' bytes starting at 'key'. 'buffer' must point to an initialized
+ * buffer of size DPIF_FLOW_BUFSIZE bytes.
+ *
+ * On success, 'flow' will be populated with the mask, actions and stats for
+ * the datapath flow corresponding to 'key'. The mask and actions may point
+ * within '*buffer', or may point at RCU-protected data. Therefore, callers
+ * that wish to hold these over quiescent periods must make a copy of these
+ * fields before quiescing.
+ *
+ * Succeeds with status 0 if the flow is fetched, or fails with ENOENT if no
+ * such flow exists. Other failures are indicated with a positive errno value.
+ */
+struct dpif_flow_get {
+    /* Input. */
+    const struct nlattr *key;       /* Flow to get. */
+    size_t key_len;                 /* Length of 'key' in bytes. */
+    struct ofpbuf *buffer;          /* Storage for output parameters. */
+
+    /* Output. */
+    struct dpif_flow *flow;         /* Resulting flow from datapath. */
+};
+
 int dpif_execute(struct dpif *, struct dpif_execute *);
 
 struct dpif_op {
@@ -673,6 +701,7 @@ struct dpif_op {
         struct dpif_flow_put flow_put;
         struct dpif_flow_del flow_del;
         struct dpif_execute execute;
+        struct dpif_flow_get flow_get;
     } u;
 };
 
diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
index 66728e6..851aed7 100644
--- a/ofproto/ofproto-dpif-upcall.c
+++ b/ofproto/ofproto-dpif-upcall.c
@@ -1523,15 +1523,17 @@ handle_missed_revalidation(struct revalidator *revalidator,
 {
     struct udpif *udpif = revalidator->udpif;
     struct dpif_flow flow;
-    struct ofpbuf *buf;
+    struct ofpbuf buf;
+    uint64_t stub[DPIF_FLOW_BUFSIZE / 8];
     bool keep = false;
 
     COVERAGE_INC(revalidate_missed_dp_flow);
 
+    ofpbuf_use_stub(&buf, &stub, sizeof stub);
     if (!dpif_flow_get(udpif->dpif, ukey->key, ukey->key_len, &buf, &flow)) {
         keep = revalidate_ukey(udpif, ukey, &flow);
-        ofpbuf_delete(buf);
     }
+    ofpbuf_uninit(&buf);
 
     return keep;
 }
-- 
1.7.10.4




More information about the dev mailing list