From bc4a05c639ee3789d009bb6143345cf121b2d4d4 Mon Sep 17 00:00:00 2001 From: Ben Pfaff Date: Mon, 17 Jan 2011 14:40:58 -0800 Subject: [PATCH] datapath: Change ODP_FLOW_GET to retrieve only a single flow at a time. This brings the code closer to what the Netlink interface will need to implement. Signed-off-by: Ben Pfaff Acked-by: Jesse Gross --- datapath/datapath.c | 130 ++++++------------------ datapath/odp-compat.h | 9 +- include/openvswitch/datapath-protocol.h | 8 +- lib/dpif-linux.c | 7 +- lib/dpif-netdev.c | 60 +++++------ lib/dpif-provider.h | 39 +++---- lib/dpif.c | 50 +-------- lib/dpif.h | 1 - ofproto/ofproto.c | 4 +- 9 files changed, 76 insertions(+), 232 deletions(-) diff --git a/datapath/datapath.c b/datapath/datapath.c index da96e3eea..0f59e9ae5 100644 --- a/datapath/datapath.c +++ b/datapath/datapath.c @@ -779,7 +779,6 @@ static void get_stats(struct sw_flow *flow, struct odp_flow_stats *stats) stats->n_bytes = flow->byte_count; stats->reserved = 0; stats->tcp_flags = flow->tcp_flags; - stats->error = 0; } static void clear_stats(struct sw_flow *flow) @@ -1017,55 +1016,25 @@ static int del_flow(struct datapath *dp, struct odp_flow __user *ufp) return error; } -static int do_query_flows(struct datapath *dp, const struct odp_flowvec *flowvec) +static int query_flow(struct datapath *dp, struct odp_flow __user *uflow) { struct tbl *table = get_table_protected(dp); - u32 i; - - for (i = 0; i < flowvec->n_flows; i++) { - struct odp_flow __user *ufp = (struct odp_flow __user __force *)&flowvec->flows[i]; - struct sw_flow_key key; - struct odp_flow uf; - struct tbl_node *flow_node; - int error; - - if (copy_from_user(&uf, ufp, sizeof(uf))) - return -EFAULT; - - error = flow_copy_from_user(&key, (const struct nlattr __force __user *)uf.key, uf.key_len); - if (error) - return error; - - flow_node = tbl_lookup(table, &uf.key, flow_hash(&key), flow_cmp); - if (!flow_node) - error = put_user(ENOENT, &ufp->stats.error); - else - error = answer_query(dp, flow_cast(flow_node), uf.flags, ufp); - if (error) - return -EFAULT; - } - return flowvec->n_flows; -} - -static int do_flowvec_ioctl(struct datapath *dp, unsigned long argp, - int (*function)(struct datapath *, - const struct odp_flowvec *)) -{ - struct odp_flowvec __user *uflowvec; - struct odp_flowvec flowvec; - int retval; + struct tbl_node *flow_node; + struct sw_flow_key key; + struct odp_flow flow; + int error; - uflowvec = (struct odp_flowvec __user *)argp; - if (copy_from_user(&flowvec, uflowvec, sizeof(flowvec))) + if (copy_from_user(&flow, uflow, sizeof(flow))) return -EFAULT; - if (flowvec.n_flows > INT_MAX / sizeof(struct odp_flow)) - return -EINVAL; + error = flow_copy_from_user(&key, (const struct nlattr __force __user *)flow.key, flow.key_len); + if (error) + return error; - retval = function(dp, &flowvec); - return (retval < 0 ? retval - : retval == flowvec.n_flows ? 0 - : put_user(retval, &uflowvec->n_flows)); + flow_node = tbl_lookup(table, &flow.key, flow_hash(&key), flow_cmp); + if (!flow_node) + return -ENOENT; + return answer_query(dp, flow_cast(flow_node), flow.flags, uflow); } static struct sw_flow *do_dump_flow(struct datapath *dp, u32 __user *state) @@ -1769,7 +1738,7 @@ static long openvswitch_ioctl(struct file *f, unsigned int cmd, break; case ODP_FLOW_GET: - err = do_flowvec_ioctl(dp, argp, do_query_flows); + err = query_flow(dp, (struct odp_flow __user *)argp); break; case ODP_FLOW_DUMP: @@ -1870,37 +1839,25 @@ static int compat_del_flow(struct datapath *dp, struct compat_odp_flow __user *u return error; } -static int compat_query_flows(struct datapath *dp, - struct compat_odp_flow __user *flows, - u32 n_flows) +static int compat_query_flow(struct datapath *dp, struct compat_odp_flow __user *uflow) { struct tbl *table = get_table_protected(dp); - u32 i; - - for (i = 0; i < n_flows; i++) { - struct compat_odp_flow __user *ufp = &flows[i]; - struct odp_flow uf; - struct tbl_node *flow_node; - struct sw_flow_key key; - int error; + struct tbl_node *flow_node; + struct sw_flow_key key; + struct odp_flow flow; + int error; - if (compat_get_flow(&uf, ufp)) - return -EFAULT; + if (compat_get_flow(&flow, uflow)) + return -EFAULT; - error = flow_copy_from_user(&key, (const struct nlattr __force __user *) uf.key, uf.key_len); - if (error) - return error; + error = flow_copy_from_user(&key, (const struct nlattr __force __user *)flow.key, flow.key_len); + if (error) + return error; - flow_node = tbl_lookup(table, &key, flow_hash(&key), flow_cmp); - if (!flow_node) - error = put_user(ENOENT, &ufp->stats.error); - else - error = compat_answer_query(dp, flow_cast(flow_node), - uf.flags, ufp); - if (error) - return -EFAULT; - } - return n_flows; + flow_node = tbl_lookup(table, &key, flow_hash(&key), flow_cmp); + if (!flow_node) + return -ENOENT; + return compat_answer_query(dp, flow_cast(flow_node), flow.flags, uflow); } static int compat_dump_flow(struct datapath *dp, struct compat_odp_flow_dump __user *udumpp) @@ -1936,35 +1893,6 @@ static int compat_dump_flow(struct datapath *dp, struct compat_odp_flow_dump __u return compat_answer_query(dp, flow, 0, uflowp); } -static int compat_flowvec_ioctl(struct datapath *dp, unsigned long argp, - int (*function)(struct datapath *, - struct compat_odp_flow __user *, - u32 n_flows)) -{ - struct compat_odp_flowvec __user *uflowvec; - struct compat_odp_flow __user *flows; - struct compat_odp_flowvec flowvec; - int retval; - - uflowvec = compat_ptr(argp); - if (!access_ok(VERIFY_WRITE, uflowvec, sizeof(*uflowvec)) || - copy_from_user(&flowvec, uflowvec, sizeof(flowvec))) - return -EFAULT; - - if (flowvec.n_flows > INT_MAX / sizeof(struct compat_odp_flow)) - return -EINVAL; - - flows = compat_ptr(flowvec.flows); - if (!access_ok(VERIFY_WRITE, flows, - flowvec.n_flows * sizeof(struct compat_odp_flow))) - return -EFAULT; - - retval = function(dp, flows, flowvec.n_flows); - return (retval < 0 ? retval - : retval == flowvec.n_flows ? 0 - : put_user(retval, &uflowvec->n_flows)); -} - static int compat_execute(struct datapath *dp, const struct compat_odp_execute __user *uexecute) { struct odp_execute execute; @@ -2028,7 +1956,7 @@ static long openvswitch_compat_ioctl(struct file *f, unsigned int cmd, unsigned break; case ODP_FLOW_GET32: - err = compat_flowvec_ioctl(dp, argp, compat_query_flows); + err = compat_query_flow(dp, compat_ptr(argp)); break; case ODP_FLOW_DUMP32: diff --git a/datapath/odp-compat.h b/datapath/odp-compat.h index 11089ddff..9e74a26d9 100644 --- a/datapath/odp-compat.h +++ b/datapath/odp-compat.h @@ -1,5 +1,5 @@ /* - * Copyright (c) 2010 Nicira Networks. + * Copyright (c) 2010, 2011 Nicira Networks. * Distributed under the terms of the GNU GPL version 2. * * Significant portions of this file may be copied from parts of the Linux @@ -15,7 +15,7 @@ #include "openvswitch/datapath-protocol.h" #include -#define ODP_FLOW_GET32 _IOWR('O', 13, struct compat_odp_flowvec) +#define ODP_FLOW_GET32 _IOWR('O', 13, struct compat_odp_flow) #define ODP_FLOW_PUT32 _IOWR('O', 14, struct compat_odp_flow) #define ODP_FLOW_DUMP32 _IOWR('O', 15, struct compat_odp_flow_dump) #define ODP_FLOW_DEL32 _IOWR('O', 17, struct compat_odp_flow) @@ -41,11 +41,6 @@ struct compat_odp_flow_dump { uint32_t state[2]; }; -struct compat_odp_flowvec { - compat_uptr_t flows; - u32 n_flows; -}; - struct compat_odp_execute { compat_uptr_t actions; u32 actions_len; diff --git a/include/openvswitch/datapath-protocol.h b/include/openvswitch/datapath-protocol.h index ec089bfe2..54c39d1ff 100644 --- a/include/openvswitch/datapath-protocol.h +++ b/include/openvswitch/datapath-protocol.h @@ -88,7 +88,7 @@ #define ODP_VPORT_SET _IOR('O', 22, struct odp_vport) #define ODP_VPORT_DUMP _IOWR('O', 10, struct odp_vport) -#define ODP_FLOW_GET _IOWR('O', 13, struct odp_flowvec) +#define ODP_FLOW_GET _IOWR('O', 13, struct odp_flow) #define ODP_FLOW_PUT _IOWR('O', 14, struct odp_flow) #define ODP_FLOW_DUMP _IOWR('O', 15, struct odp_flow_dump) #define ODP_FLOW_FLUSH _IO('O', 16) @@ -214,7 +214,6 @@ struct odp_flow_stats { uint32_t used_nsec; uint8_t tcp_flags; uint8_t reserved; - uint16_t error; /* Used by ODP_FLOW_GET. */ }; enum odp_key_type { @@ -296,11 +295,6 @@ struct odp_flow_put { uint32_t flags; }; -struct odp_flowvec { - struct odp_flow *flows; - uint32_t n_flows; -}; - /* ODP_FLOW_DUMP argument. * * This is used to iterate through the flow table flow-by-flow. Each diff --git a/lib/dpif-linux.c b/lib/dpif-linux.c index 6e85d61a0..e0619b584 100644 --- a/lib/dpif-linux.c +++ b/lib/dpif-linux.c @@ -459,12 +459,9 @@ dpif_linux_port_poll_wait(const struct dpif *dpif_) } static int -dpif_linux_flow_get(const struct dpif *dpif_, struct odp_flow flows[], int n) +dpif_linux_flow_get(const struct dpif *dpif_, struct odp_flow *flow) { - struct odp_flowvec fv; - fv.flows = flows; - fv.n_flows = n; - return do_ioctl(dpif_, ODP_FLOW_GET, &fv); + return do_ioctl(dpif_, ODP_FLOW_GET, flow); } static int diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c index 8852e9d54..8bb0ea4a2 100644 --- a/lib/dpif-netdev.c +++ b/lib/dpif-netdev.c @@ -625,26 +625,20 @@ static void answer_flow_query(struct dp_netdev_flow *flow, uint32_t query_flags, struct odp_flow *odp_flow) { - if (flow) { - odp_flow->stats.n_packets = flow->packet_count; - odp_flow->stats.n_bytes = flow->byte_count; - odp_flow->stats.used_sec = flow->used.tv_sec; - odp_flow->stats.used_nsec = flow->used.tv_nsec; - odp_flow->stats.tcp_flags = TCP_FLAGS(flow->tcp_ctl); - odp_flow->stats.reserved = 0; - odp_flow->stats.error = 0; - if (odp_flow->actions_len > 0) { - memcpy(odp_flow->actions, flow->actions, - MIN(odp_flow->actions_len, flow->actions_len)); - odp_flow->actions_len = flow->actions_len; - } - - if (query_flags & ODPFF_ZERO_TCP_FLAGS) { - flow->tcp_ctl = 0; - } + odp_flow->stats.n_packets = flow->packet_count; + odp_flow->stats.n_bytes = flow->byte_count; + odp_flow->stats.used_sec = flow->used.tv_sec; + odp_flow->stats.used_nsec = flow->used.tv_nsec; + odp_flow->stats.tcp_flags = TCP_FLAGS(flow->tcp_ctl); + odp_flow->stats.reserved = 0; + if (odp_flow->actions_len > 0) { + memcpy(odp_flow->actions, flow->actions, + MIN(odp_flow->actions_len, flow->actions_len)); + odp_flow->actions_len = flow->actions_len; + } - } else { - odp_flow->stats.error = ENOENT; + if (query_flags & ODPFF_ZERO_TCP_FLAGS) { + flow->tcp_ctl = 0; } } @@ -675,25 +669,25 @@ dpif_netdev_flow_from_nlattrs(const struct nlattr *key, uint32_t key_len, } static int -dpif_netdev_flow_get(const struct dpif *dpif, struct odp_flow flows[], int n) +dpif_netdev_flow_get(const struct dpif *dpif, struct odp_flow *odp_flow) { struct dp_netdev *dp = get_dp_netdev(dpif); - int i; - - for (i = 0; i < n; i++) { - struct odp_flow *odp_flow = &flows[i]; - struct flow key; - int error; + struct dp_netdev_flow *flow; + struct flow key; + int error; - error = dpif_netdev_flow_from_nlattrs(odp_flow->key, odp_flow->key_len, - &key); - if (error) { - return error; - } + error = dpif_netdev_flow_from_nlattrs(odp_flow->key, odp_flow->key_len, + &key); + if (error) { + return error; + } - answer_flow_query(dp_netdev_lookup_flow(dp, &key), - odp_flow->flags, odp_flow); + flow = dp_netdev_lookup_flow(dp, &key); + if (!flow) { + return ENOENT; } + + answer_flow_query(flow, odp_flow->flags, odp_flow); return 0; } diff --git a/lib/dpif-provider.h b/lib/dpif-provider.h index f6548b39c..2218c1127 100644 --- a/lib/dpif-provider.h +++ b/lib/dpif-provider.h @@ -207,34 +207,21 @@ struct dpif_class { * value other than EAGAIN. */ void (*port_poll_wait)(const struct dpif *dpif); - /* For each flow 'flow' in the 'n' flows in 'flows': + /* Queries 'dpif' for a flow entry matching 'flow->key'. * - * - If a flow matching 'flow->key' exists in 'dpif': + * If a flow matching 'flow->key' exists in 'dpif', stores statistics for + * the flow into 'flow->stats'. If 'flow->actions_len' is zero, then + * 'flow->actions' is ignored. If 'flow->actions_len' is nonzero, then + * 'flow->actions' should point to an array of the specified number of + * bytes. At most that many bytes of the flow's actions will be copied + * into that array. 'flow->actions_len' will be updated to the number of + * bytes of actions actually present in the flow, which may be greater than + * the amount stored if the flow has more actions than space available in + * the array. * - * Stores 0 into 'flow->stats.error' and stores statistics for the flow - * into 'flow->stats'. - * - * If 'flow->n_actions' is zero, then 'flow->actions' is ignored. If - * 'flow->n_actions' is nonzero, then 'flow->actions' should point to - * an array of the specified number of actions. At most that many of - * the flow's actions will be copied into that array. - * 'flow->n_actions' will be updated to the number of actions actually - * present in the flow, which may be greater than the number stored if - * the flow has more actions than space available in the array. - * - * - Flow-specific errors are indicated by a positive errno value in - * 'flow->stats.error'. In particular, ENOENT indicates that no flow - * matching 'flow->key' exists in 'dpif'. When an error value is stored, - * the contents of 'flow->key' are preserved but other members of 'flow' - * should be treated as indeterminate. - * - * Returns 0 if all 'n' flows in 'flows' were updated (whether they were - * individually successful or not is indicated by 'flow->stats.error', - * however). Returns a positive errno value if an error that prevented - * this update occurred, in which the caller must not depend on any - * elements in 'flows' being updated or not updated. - */ - int (*flow_get)(const struct dpif *dpif, struct odp_flow flows[], int n); + * If no flow matching 'flow->key' exists in 'dpif', returns ENOENT. On + * other failure, returns a positive errno value. */ + int (*flow_get)(const struct dpif *dpif, struct odp_flow *flow); /* Adds or modifies a flow in 'dpif' as specified in 'put': * diff --git a/lib/dpif.c b/lib/dpif.c index d7441967f..463d4e7f2 100644 --- a/lib/dpif.c +++ b/lib/dpif.c @@ -723,10 +723,7 @@ dpif_flow_get(const struct dpif *dpif, struct odp_flow *flow) COVERAGE_INC(dpif_flow_get); check_rw_flow_actions(flow); - error = dpif->dpif_class->flow_get(dpif, flow, 1); - if (!error) { - error = flow->stats.error; - } + error = dpif->dpif_class->flow_get(dpif, flow); if (error) { /* Make the results predictable on error. */ memset(&flow->stats, 0, sizeof flow->stats); @@ -738,51 +735,6 @@ dpif_flow_get(const struct dpif *dpif, struct odp_flow *flow) return error; } -/* For each flow 'flow' in the 'n' flows in 'flows': - * - * - If a flow matching 'flow->key' exists in 'dpif': - * - * Stores 0 into 'flow->stats.error' and stores statistics for the flow - * into 'flow->stats'. - * - * If 'flow->actions_len' is zero, then 'flow->actions' is ignored. If - * 'flow->actions_len' is nonzero, then 'flow->actions' should point to an - * array of the specified number of bytes. At most that amount of flow's - * actions will be copied into that array. 'flow->actions_len' will be - * updated to the number of bytes of actions actually present in the flow, - * which may be greater than the amount stored if the flow's actions are - * longer than the available space. - * - * - Flow-specific errors are indicated by a positive errno value in - * 'flow->stats.error'. In particular, ENOENT indicates that no flow - * matching 'flow->key' exists in 'dpif'. When an error value is stored, the - * contents of 'flow->key' are preserved but other members of 'flow' should - * be treated as indeterminate. - * - * Returns 0 if all 'n' flows in 'flows' were updated (whether they were - * individually successful or not is indicated by 'flow->stats.error', - * however). Returns a positive errno value if an error that prevented this - * update occurred, in which the caller must not depend on any elements in - * 'flows' being updated or not updated. - */ -int -dpif_flow_get_multiple(const struct dpif *dpif, - struct odp_flow flows[], size_t n) -{ - int error; - size_t i; - - COVERAGE_ADD(dpif_flow_get, n); - - for (i = 0; i < n; i++) { - check_rw_flow_actions(&flows[i]); - } - - error = dpif->dpif_class->flow_get(dpif, flows, n); - log_operation(dpif, "flow_get_multiple", error); - return error; -} - /* Adds or modifies a flow in 'dpif' as specified in 'put': * * - If the flow specified in 'put->flow' does not exist in 'dpif', then diff --git a/lib/dpif.h b/lib/dpif.h index 4efabd0a2..3c376ec1f 100644 --- a/lib/dpif.h +++ b/lib/dpif.h @@ -111,7 +111,6 @@ int dpif_flow_flush(struct dpif *); int dpif_flow_put(struct dpif *, struct odp_flow_put *); int dpif_flow_del(struct dpif *, struct odp_flow *); int dpif_flow_get(const struct dpif *, struct odp_flow *); -int dpif_flow_get_multiple(const struct dpif *, struct odp_flow[], size_t n); struct dpif_flow_dump { const struct dpif *dpif; diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c index ea869f1ec..d928f686e 100644 --- a/ofproto/ofproto.c +++ b/ofproto/ofproto.c @@ -3487,9 +3487,7 @@ query_stats(struct ofproto *p, struct rule *rule, packet_count = rule->packet_count; byte_count = rule->byte_count; - /* Ask the datapath for statistics on all of the rule's facets. (We could - * batch up statistics requests using dpif_flow_get_multiple(), but that is - * not yet implemented.) + /* Ask the datapath for statistics on all of the rule's facets. * * Also, add any statistics that are not tracked by the datapath for each * facet. This includes, for example, statistics for packets that were -- 2.43.0