[ovs-dev] [PATCH] ofproto: Allow in-place modifications of datapath flows.
Justin Pettit
jpettit at nicira.com
Thu Aug 6 20:07:59 UTC 2015
This is cleaner than I was expecting. Do you think we want to backport it to 2.3?
--Justin
> On Aug 6, 2015, at 12:30 PM, Jarno Rajahalme <jrajahalme at nicira.com> wrote:
>
> Ethan,
>
> This ended up being a very clean patch, thank you!
>
> IMO this is good to go, but I’d like to see a test case that shows the modify operation in the logs to prevent regression in the future. Maybe work off some of the bond rebalance test cases to prove that modify is used in the case we currently know suffers from the lack of modify support?
>
> Also, there are some minor comments below you could address at the same time.
>
> Jarno
>
>> On Aug 5, 2015, at 4:07 PM, Ethan Jackson <ethan at nicira.com> wrote:
>>
>> There are certain use cases (such as bond rebalancing) where a
>> datapath flow's actions may change, while it's wildcard pattern
>> remains the same. Before this patch, revalidators would note the
>> change, delete the flow, and wait for the handlers to install an
>> updated version. This is inefficient, as many packets could get
>> punted to userspace before the new flow is finally installed.
>>
>> To improve the situation, this patch implements in place modification
>> of datapath flows. If the revalidators detect the only change to a
>> given ukey is its actions, instead of deleting it, it does a put with
>> the MODIFY flag set.
>>
>> Signed-off-by: Ethan Jackson <ethan at nicira.com>
>> ---
>> ofproto/ofproto-dpif-upcall.c | 118 +++++++++++++++++++++++++++++++-----------
>> 1 file changed, 87 insertions(+), 31 deletions(-)
>>
>> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
>> index 59010c2..7abc97d 100644
>> --- a/ofproto/ofproto-dpif-upcall.c
>> +++ b/ofproto/ofproto-dpif-upcall.c
>> @@ -150,6 +150,12 @@ enum upcall_type {
>> IPFIX_UPCALL /* Per-bridge sampling. */
>> };
>>
>> +enum reval_result {
>> + UKEY_KEEP,
>> + UKEY_DELETE,
>> + UKEY_MODIFY
>> +};
>> +
>> struct upcall {
>> struct ofproto_dpif *ofproto; /* Parent ofproto. */
>> const struct recirc_id_node *recirc; /* Recirculation context. */
>> @@ -1663,33 +1669,41 @@ should_revalidate(const struct udpif *udpif, uint64_t packets,
>> return false;
>> }
>>
>> -static bool
>> +/* Verifies that the datapath actions of 'ukey' are still correct, and pushes
>> + * 'stats' for it.
>> + *
>> + * Returns a recommended action for 'ukey', options include: UKEY_DELETE,
>> + * meaning the ukey should be deleted. UKEY_KEEP, meaning the ukey is fine as
>> + * is. Or UKEY_MODIFY, meaning the ukey's actions should be changed but is
>> + * otherwise fine. If UKEY_MODIFY, is returned, callers should change the
>> + * ukey's actions to those found in the caller supplied 'odp_actions' buffer.
>> + * */
>
> This would be easier to read if you reformat this with each option listed in it’s own line, and drop the repeating “meaning the”.
>
>> +static enum reval_result
>> revalidate_ukey(struct udpif *udpif, struct udpif_key *ukey,
>> - const struct dpif_flow_stats *stats, uint64_t reval_seq)
>> + const struct dpif_flow_stats *stats,
>> + struct ofpbuf *odp_actions, uint64_t reval_seq)
>> OVS_REQUIRES(ukey->mutex)
>> {
>> - uint64_t odp_actions_stub[1024 / 8];
>> - struct ofpbuf odp_actions = OFPBUF_STUB_INITIALIZER(odp_actions_stub);
>> -
>> struct xlate_out xout, *xoutp;
>> struct netflow *netflow;
>> struct ofproto_dpif *ofproto;
>> struct dpif_flow_stats push;
>> struct flow flow, dp_mask;
>> struct flow_wildcards wc;
>> + enum reval_result result;
>> uint64_t *dp64, *xout64;
>> ofp_port_t ofp_in_port;
>> struct xlate_in xin;
>> long long int last_used;
>> int error;
>> size_t i;
>> - bool ok;
>> bool need_revalidate;
>>
>> - ok = false;
>> + result = UKEY_DELETE;
>> xoutp = NULL;
>> netflow = NULL;
>>
>> + ofpbuf_clear(odp_actions);
>> need_revalidate = (ukey->reval_seq != reval_seq);
>> last_used = ukey->stats.used;
>> push.used = stats->used;
>> @@ -1703,20 +1717,19 @@ revalidate_ukey(struct udpif *udpif, struct udpif_key *ukey,
>>
>> if (need_revalidate && last_used
>> && !should_revalidate(udpif, push.n_packets, last_used)) {
>> - ok = false;
>> goto exit;
>> }
>>
>> /* We will push the stats, so update the ukey stats cache. */
>> ukey->stats = *stats;
>> if (!push.n_packets && !need_revalidate) {
>> - ok = true;
>> + result = UKEY_KEEP;
>> goto exit;
>> }
>>
>> if (ukey->xcache && !need_revalidate) {
>> xlate_push_stats(ukey->xcache, &push);
>> - ok = true;
>> + result = UKEY_KEEP;
>> goto exit;
>> }
>>
>> @@ -1739,7 +1752,7 @@ revalidate_ukey(struct udpif *udpif, struct udpif_key *ukey,
>> }
>>
>> xlate_in_init(&xin, ofproto, &flow, ofp_in_port, NULL, push.tcp_flags,
>> - NULL, need_revalidate ? &wc : NULL, &odp_actions);
>> + NULL, need_revalidate ? &wc : NULL, odp_actions);
>> if (push.n_packets) {
>> xin.resubmit_stats = &push;
>> xin.may_learn = true;
>> @@ -1749,18 +1762,14 @@ revalidate_ukey(struct udpif *udpif, struct udpif_key *ukey,
>> xoutp = &xout;
>>
>> if (!need_revalidate) {
>> - ok = true;
>> + result = UKEY_KEEP;
>> goto exit;
>> }
>>
>> if (xout.slow) {
>> - ofpbuf_clear(&odp_actions);
>> + ofpbuf_clear(odp_actions);
>> compose_slow_path(udpif, &xout, &flow, flow.in_port.odp_port,
>> - &odp_actions);
>> - }
>> -
>> - if (!ofpbuf_equal(&odp_actions, ukey->actions)) {
>> - goto exit;
>> + odp_actions);
>> }
>>
>> if (odp_flow_key_to_mask(ukey->mask, ukey->mask_len, ukey->key,
>> @@ -1781,18 +1790,24 @@ revalidate_ukey(struct udpif *udpif, struct udpif_key *ukey,
>> }
>> }
>>
>> - ok = true;
>> + if (!ofpbuf_equal(odp_actions, ukey->actions)) {
>> + /* The datpaath mask was OK, but the actions seem to have changed.
>
> Typo on “datpaath”
>
>> + * Let's modify it in place. */
>> + result = UKEY_MODIFY;
>> + goto exit;
>> + }
>> +
>> + result = UKEY_KEEP;
>>
>> exit:
>> - if (ok) {
>> + if (result != UKEY_DELETE) {
>> ukey->reval_seq = reval_seq;
>> }
>> - if (netflow && !ok) {
>> + if (netflow && result != UKEY_DELETE) {
>> netflow_flow_clear(netflow, &flow);
>> }
>> xlate_out_uninit(xoutp);
>> - ofpbuf_uninit(&odp_actions);
>> - return ok;
>> + return result;
>> }
>>
>> static void
>> @@ -1823,6 +1838,23 @@ delete_op_init(struct udpif *udpif, struct ukey_op *op, struct udpif_key *ukey)
>> }
>>
>> static void
>> +modify_op_init(struct ukey_op *op, struct udpif_key *ukey)
>> +{
>> + op->ukey = ukey;
>> + op->dop.type = DPIF_OP_FLOW_PUT;
>> + op->dop.u.flow_put.flags = DPIF_FP_MODIFY;
>> + op->dop.u.flow_put.key = ukey->key;
>> + op->dop.u.flow_put.key_len = ukey->key_len;
>> + op->dop.u.flow_put.mask = ukey->mask;
>> + op->dop.u.flow_put.mask_len = ukey->mask_len;
>> + op->dop.u.flow_put.ufid = &ukey->ufid;
>> + op->dop.u.flow_put.pmd_id = ukey->pmd_id;
>> + op->dop.u.flow_put.stats = NULL;
>> + op->dop.u.flow_put.actions = ukey->actions->data;
>> + op->dop.u.flow_put.actions_len = ukey->actions->size;
>> +}
>> +
>> +static void
>> push_ukey_ops__(struct udpif *udpif, struct ukey_op *ops, size_t n_ops)
>> {
>> struct dpif_op *opsp[REVALIDATE_MAX_BATCH];
>> @@ -1841,6 +1873,12 @@ push_ukey_ops__(struct udpif *udpif, struct ukey_op *ops, size_t n_ops)
>> stats = op->dop.u.flow_del.stats;
>> push = &push_buf;
>>
>> + if (op->dop.type == DPIF_OP_FLOW_PUT) {
>> + /* This was an in place modification. Don't bother pushing stats
>> + * for now. */
>
> As the stats were just pushed in revalidate_ukey()? If so, mention it here.
>
>> + continue;
>> + }
>> +
>> if (op->ukey) {
>> ovs_mutex_lock(&op->ukey->mutex);
>> push->used = MAX(stats->used, op->ukey->stats.used);
>> @@ -1926,6 +1964,9 @@ log_unexpected_flow(const struct dpif_flow *flow, int error)
>> static void
>> revalidate(struct revalidator *revalidator)
>> {
>> + uint64_t odp_actions_stub[1024 / 8];
>> + struct ofpbuf odp_actions = OFPBUF_STUB_INITIALIZER(odp_actions_stub);
>> +
>> struct udpif *udpif = revalidator->udpif;
>> struct dpif_flow_dump_thread *dump_thread;
>> uint64_t dump_seq, reval_seq;
>> @@ -1973,8 +2014,9 @@ revalidate(struct revalidator *revalidator)
>>
>> for (f = flows; f < &flows[n_dumped]; f++) {
>> long long int used = f->stats.used;
>> + enum reval_result result;
>> struct udpif_key *ukey;
>> - bool already_dumped, keep;
>> + bool already_dumped;
>> int error;
>>
>> if (ukey_acquire(udpif, f, &ukey, &error)) {
>> @@ -2008,15 +2050,20 @@ revalidate(struct revalidator *revalidator)
>> used = ukey->created;
>> }
>> if (kill_them_all || (used && used < now - max_idle)) {
>> - keep = false;
>> + result = UKEY_DELETE;
>> } else {
>> - keep = revalidate_ukey(udpif, ukey, &f->stats, reval_seq);
>> + result = revalidate_ukey(udpif, ukey, &f->stats, &odp_actions,
>> + reval_seq);
>> }
>> ukey->dump_seq = dump_seq;
>> - ukey->flow_exists = keep;
>> + ukey->flow_exists = result != UKEY_DELETE;
>>
>> - if (!keep) {
>> + if (result == UKEY_DELETE) {
>> delete_op_init(udpif, &ops[n_ops++], ukey);
>> + } else if (result == UKEY_MODIFY){
>
> Add space between “){“
>
>> + ofpbuf_delete(ukey->actions);
>> + ukey->actions = ofpbuf_clone(&odp_actions);
>> + modify_op_init(&ops[n_ops++], ukey);
>> }
>> ovs_mutex_unlock(&ukey->mutex);
>> }
>> @@ -2027,23 +2074,32 @@ revalidate(struct revalidator *revalidator)
>> ovsrcu_quiesce();
>> }
>> dpif_flow_dump_thread_destroy(dump_thread);
>> + ofpbuf_uninit(&odp_actions);
>> }
>>
>> static bool
>> handle_missed_revalidation(struct udpif *udpif, uint64_t reval_seq,
>> struct udpif_key *ukey)
>> {
>> + uint64_t odp_actions_stub[1024 / 8];
>> + struct ofpbuf odp_actions = OFPBUF_STUB_INITIALIZER(odp_actions_stub);
>> +
>> struct dpif_flow_stats stats;
>> - bool keep;
>> + enum reval_result result;
>>
>> COVERAGE_INC(revalidate_missed_dp_flow);
>>
>> memset(&stats, 0, sizeof stats);
>> ovs_mutex_lock(&ukey->mutex);
>> - keep = revalidate_ukey(udpif, ukey, &stats, reval_seq);
>> + result = revalidate_ukey(udpif, ukey, &stats, &odp_actions, reval_seq);
>> ovs_mutex_unlock(&ukey->mutex);
>>
>> - return keep;
>> + ofpbuf_uninit(&odp_actions);
>> +
>> + /* XXX: We somewhat conversvatively don't bother with in place
>> + * modifications in this case. Probably could be made to work, but this is
>> + * simpler, and the edge case is rare. */
>> + return result == UKEY_KEEP;
>
> There is only one caller of this function, and the call site seems simple enough to support modification, it would be nice if it did. If for nothing else than getting rid of one extra “XXX” (and the typo in the comment at the same time :-).
>
>> }
>>
>> static void
>> --
>> 2.1.0
>>
>> _______________________________________________
>> dev mailing list
>> dev at openvswitch.org
>> http://openvswitch.org/mailman/listinfo/dev
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
More information about the dev
mailing list