Your call either way. I don't feel strongly.
Ethan
On Thu, Aug 6, 2015 at 1:07 PM, Justin Pettit <jpet...@nicira.com> wrote:
> 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
