On 25 December 2016 at 03:39, Paul Blakey <[email protected]> wrote:
While dumping flows, dump flows that were offloaded to
netdev and parse them back to dpif flow.
Signed-off-by: Paul Blakey <[email protected]>
Reviewed-by: Roi Dayan <[email protected]>
---
lib/dpif-netlink.c | 179 +++++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 179 insertions(+)
diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
index 36f2888..3d8940e 100644
--- a/lib/dpif-netlink.c
+++ b/lib/dpif-netlink.c
@@ -38,6 +38,7 @@
#include "flow.h"
#include "fat-rwlock.h"
#include "netdev.h"
+#include "netdev-provider.h"
#include "netdev-linux.h"
#include "netdev-vport.h"
#include "netlink-conntrack.h"
@@ -55,6 +56,7 @@
#include "unaligned.h"
#include "util.h"
#include "openvswitch/vlog.h"
+#include "openvswitch/match.h"
VLOG_DEFINE_THIS_MODULE(dpif_netlink);
#ifdef _WIN32
@@ -68,6 +70,8 @@ enum { MAX_PORTS = USHRT_MAX };
* missing if we have old headers. */
#define ETH_FLAG_LRO (1 << 15) /* LRO is enabled */
+#define FLOW_DUMP_MAX_BATCH 50
+
struct dpif_netlink_dp {
/* Generic Netlink header. */
uint8_t cmd;
@@ -1355,6 +1359,10 @@ struct dpif_netlink_flow_dump {
struct dpif_flow_dump up;
struct nl_dump nl_dump;
atomic_int status;
+ struct netdev_flow_dump **netdev_dumps;
+ int netdev_num;
+ int netdev_given;
+ struct ovs_mutex netdev_lock;
Could you add a brief comment above these variables that describes
their use? (It's also common in OVS code to mention that, eg,
netdev_lock protects the following elements. )
If there's a more descriptive name than "netdev_num", like
netdev_max_dumps or something then please use that instead. At a
glance, "given" and "num" don't provide particularly much context
about how they relate to each other or to the dump.
};
static struct dpif_netlink_flow_dump *
@@ -1363,6 +1371,34 @@ dpif_netlink_flow_dump_cast(struct dpif_flow_dump *dump)
return CONTAINER_OF(dump, struct dpif_netlink_flow_dump, up);
}
+static void start_netdev_dump(const struct dpif *dpif_,
+ struct dpif_netlink_flow_dump *dump) {
+
+ if (!netdev_flow_api_enabled) {
+ dump->netdev_num = 0;
+ return;
+ }
Typically for style we still place all variable declarations at the
top of a function, in a christmas tree long lines to short lines,
before functional code like this.
+
+ struct netdev_list_element *element;
+ struct ovs_list port_list;
+ int ports = netdev_hmap_port_get_list(dpif_->dpif_class, &port_list);
+ int i = 0;
+
+ dump->netdev_dumps =
+ ports ? xzalloc(sizeof(struct netdev_flow_dump *) * ports) : 0;
Can this be sizeof(dump->netdev_dumps)?
+ dump->netdev_num = ports;
+ dump->netdev_given = 0;
+
+ LIST_FOR_EACH(element, node, &port_list) {
+ dump->netdev_dumps[i] = netdev_flow_dump_create(element->netdev);
+ dump->netdev_dumps[i]->port = element->port_no;
+ i++;
+ }
As a matter of style, it's easier to see that this loop is bounded by
'ports' (and that number is correct) if it's structured as
for (i = 0; i < ports; i++) {
element = get_next_node;
...
}
Also, it seems that even if the netdev doesn't support flow_dump, we
allocate a netdev_flow_dump and add it to the netdev_dumps here..
perhaps we could/should skip it for these netdevs instead?
+ netdev_port_list_del(&port_list);
+
+ ovs_mutex_init(&dump->netdev_lock);
I don't see a corresponding ovs_mutex_destroy() call for this.
+}
+
static struct dpif_flow_dump *
dpif_netlink_flow_dump_create(const struct dpif *dpif_, bool terse)
{
@@ -1387,6 +1423,8 @@ dpif_netlink_flow_dump_create(const struct dpif *dpif_,
bool terse)
atomic_init(&dump->status, 0);
dump->up.terse = terse;
+ start_netdev_dump(dpif_, dump);
+
return &dump->up;
}
@@ -1397,6 +1435,16 @@ dpif_netlink_flow_dump_destroy(struct dpif_flow_dump
*dump_)
unsigned int nl_status = nl_dump_done(&dump->nl_dump);
int dump_status;
+ if (netdev_flow_api_enabled) {
+ for (int i = 0; i < dump->netdev_num; i++) {
+ int err = netdev_flow_dump_destroy(dump->netdev_dumps[i]);
+ if (err != 0 && err != EOPNOTSUPP) {
+ VLOG_ERR("failed dumping netdev: %s", ovs_strerror(err));
+ }
+ }
+ free(dump->netdev_dumps);
+ }
You don't really need to check for netdev_flow_api_enabled here;
netdev_num will be 0 if it is disabled, so that for loop turns into a
no-op; then you could initialize dump->netdev_dumps to NULL in that
case and unconditionally free it. It's a bit simpler to read the code
if you don't have to think about whether or not hardware offloads are
enabled.
+
/* No other thread has access to 'dump' at this point. */
atomic_read_relaxed(&dump->status, &dump_status);
free(dump);
@@ -1410,6 +1458,11 @@ struct dpif_netlink_flow_dump_thread {
struct dpif_flow_stats stats;
struct ofpbuf nl_flows; /* Always used to store flows. */
struct ofpbuf *nl_actions; /* Used if kernel does not supply actions. */
+ struct odputil_keybuf keybuf[FLOW_DUMP_MAX_BATCH];
+ struct odputil_keybuf maskbuf[FLOW_DUMP_MAX_BATCH];
+ struct odputil_keybuf actbuf[FLOW_DUMP_MAX_BATCH];
+ int netdev_cur_dump;
+ bool netdev_done;
I wonder if it's worthwhile to reuse 'nl_flows' to store all of these
netlink-formatted key/mask/acts instead of having these keybufs? It
seems that it is currently unused for the first half of the
dpif_netlink_flow_dump while the flows are being dumped from the
netdev.
Regardless of the above question, I also question whether
FLOW_DUMP_MAX_BATCH is too big for dumping from the kernel. How many
tc flows will we really get from the kernel at once?
