On Tue, Oct 07, 2014 at 12:23:38AM +1300, Joe Stringer wrote:
> One of the limiting factors on the number of flows that can be supported
> in the datapath is the overhead of assembling flow dump messages in the
> datapath. This patch modifies the flow_dump interface to allow
> revalidators to skip dumping the key, mask and actions from the
> datapath, by making use of the unique identifiers introduced in earlier
> patches.
>
> For each flow dump, the dpif user specifies whether to skip these
> attributes, allowing the common case to only dump a pair of 128-bit ID
> and flow stats. This increases the number of flows that a revalidator
> can handle per second by 50% or more.
>
> Signed-off-by: Joe Stringer <[email protected]>
Thanks! I think this series is close to being ready to go.
I see an apparent inconsistency in push_ukey_ops__(): the initial loop
asserts that every operation has a ukey:
for (i = 0; i < n_ops; i++) {
ovs_assert(ops[i].ukey);
opsp[i] = &ops[i].dop;
}
but later code has different cases based on whether a ukey is present.
Unless I misunderstand, this needs to be resolved one way or the other.
The change to dp_netdev_flow_to_dpif_flow() appears fairly large at
first glance, but ignoring changes in indentation it shrinks a great
deal and I see the following, which makes me wonder whether the
memcpy() to struct assignment change should have been done in an
earlier commit:
@@ -1461,12 +1470,13 @@ dp_netdev_flow_to_dpif_flow(const struct dp_netdev_flow
*netdev_flow,
/* Actions */
actions = dp_netdev_flow_get_actions(netdev_flow);
flow->actions = actions->actions;
flow->actions_len = actions->size;
+ }
- memcpy(&flow->uid, &netdev_flow->uid, sizeof flow->uid);
+ flow->uid = netdev_flow->uid;
get_dpif_flow_stats(netdev_flow, &flow->stats);
}
static int
dpif_netdev_mask_from_nlattrs(const struct nlattr *key, uint32_t key_len,
In dpif_netlink_flow_dump_create(), it took me a moment to realize
that the dump_flags were only for the terse mode. Maybe they could be
called the terse_flags, or just written inline in the call to
odp_uid_to_nlattrs()?
In dpif_netlink_flow_dump_next(), this comment deserves an update:
if (dump->up.terse || datapath_flow.actions) {
/* Common case: the flow includes actions. */
dpif_netlink_flow_to_dpif_flow(&dpif->dpif, &flows[n_flows++],
&datapath_flow);
maybe to "Common case: the flow includes actions (or we don't want
actions).".
In the ukey_acquire() comment:
* Returns 0 on success, setting *result to the matching ukey and returning
it
* in a locked state. Otherwise, returns an errno and clears *result. EBUSY
* indicates that another thread is handling this flow. Other errors an
"Other errors" indicate "an".
* unexpected condition creating a new ukey.
There might be an optimization in push_ukey_ops__() by comparing
stats->packet to op->ukey->stats.n_packets without taking the lock; if
they're the same, then (in the absence of NetFlow) I think we could
skip taking the lock entirely. I don't know whether this would yield
any measurable improvement, and if not it is probably not worth it.
As long as the apparent inconsistency in push_ukey_ops__() that I
reported above gets cleared up one way or another, you can have my:
Acked-by: Ben Pfaff <[email protected]>
Thanks,
Ben.
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
