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 <jrajaha...@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 <et...@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 <et...@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@openvswitch.org
>> http://openvswitch.org/mailman/listinfo/dev
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
