On 14 October 2014 10:43, Ben Pfaff <[email protected]> wrote:
> 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.
>
Sorry, this requirement has gone back and forth multiple times. The current
iteration should not require a ukey - this is linked to whether we attempt
to install ukeys from revalidator threads or not.
>
> 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,
>
Good spotting, I'll take a closer look at this when I respin the series.
> 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()?
>
Sure.
>
> 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).".
>
Agreed, I'll update this.
>
> 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.
>
I suppose this would provide a benefit if the typical case has no packets
to attribute in this function. However, when pushed to the limits of
datapath flow support, revalidate() and revalidate_ukey() will tend not to
push stats, instead deferring the push until push_ukey_ops__(). By that
logic, I wouldn't expect much improvement.
> 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 for the review!
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
