Just sent out the v2,
Jarno
> On Feb 4, 2016, at 3:25 PM, Jarno Rajahalme <ja...@ovn.org> wrote:
>
>
>> On Feb 4, 2016, at 3:22 PM, Ben Pfaff <b...@ovn.org> wrote:
>>
>> On Thu, Feb 04, 2016 at 03:08:48PM -0800, Jarno Rajahalme wrote:
>>>
>>>> On Feb 4, 2016, at 3:00 PM, Ben Pfaff <b...@ovn.org> wrote:
>>>>
>>>> On Wed, Feb 03, 2016 at 12:33:01PM -0800, Jarno Rajahalme wrote:
>>>>> From: Ethan Jackson <et...@nicira.com>
>>>>>
>>>>> 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.
>>>>>
>>>>> This patch is a backport of commit 43b2f13 to branch-2.3.
>>>>>
>>>>> Signed-off-by: Ethan J. Jackson <et...@nicira.com>
>>>>> Signed-off-by: Jarno Rajahalme <ja...@ovn.org>
>>>>
>>>> The buffer management in modify_op_init() really bothers me. It is
>>>> brittle and confusing. It has an unstated assumption that, if 'buf' is
>>>> nonnull, then it contains 'key' and 'mask' and that, if 'buf' is null,
>>>> then 'key' and 'mask' are elsewhere. I am not sure that this assumption
>>>> is correct, because in revalidator_sweep__() the 'key' argument to
>>>> modify_op_init() seems to come from a ukey whereas the 'mask' argument
>>>> seems to come from an ofpbuf allocated by dpif_flow_get().
>>>>
>>>> Either way, it's really weird and hard to understand and maintain. Can
>>>> we do it another way?
>>>
>>> This is why I asked you (offline) to pay attention to the buffer
>>> management.. Adjusting the ‘key’ is just wrong, and even if fixed (i.e.,
>>> left unadjusted) it is likely better to solve the buffering here in some
>>> other way.
>>
>> How about making the caller responsible for buffer management, like
>> this:
>>
>
> This is cleaner, I’ll apply this for the v2, thanks!
>
> Jarno
>
>> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
>> index 03b4afa..41a51cc 100644
>> --- a/ofproto/ofproto-dpif-upcall.c
>> +++ b/ofproto/ofproto-dpif-upcall.c
>> @@ -1,4 +1,4 @@
>> -/* Copyright (c) 2009, 2010, 2011, 2012, 2013, 2014 Nicira, Inc.
>> +/* Copyright (c) 2009, 2010, 2011, 2012, 2013, 2014, 2016 Nicira, Inc.
>> *
>> * Licensed under the Apache License, Version 2.0 (the "License");
>> * you may not use this file except in compliance with the License.
>> @@ -1384,23 +1384,17 @@ modify_op_init(struct dump_op *op, const struct
>> nlattr *key, size_t key_len,
>> const struct nlattr *actions, size_t actions_len,
>> struct udpif_key *ukey, struct ofpbuf *buf)
>> {
>> - struct nlattr *old_data;
>> - intptr_t nlattr_delta;
>> -
>> op->ukey = ukey;
>> - op->buf = buf ? buf : ofpbuf_new(actions_len);
>> + op->buf = buf;
>> op->op.type = DPIF_OP_FLOW_PUT;
>> op->op.u.flow_put.flags = DPIF_FP_MODIFY;
>>
>> - old_data = ofpbuf_data(op->buf);
>> - op->op.u.flow_put.actions = ofpbuf_put(op->buf, actions, actions_len);
>> + op->op.u.flow_put.actions = actions;
>> op->op.u.flow_put.actions_len = actions_len;
>> - /* 'op->buf's data may have been reallocated by the ofpbuf_put() above.
>> */
>> - nlattr_delta = (struct nlattr *)ofpbuf_data(op->buf) - old_data;
>>
>> - op->op.u.flow_put.key = key + nlattr_delta;
>> + op->op.u.flow_put.key = key;
>> op->op.u.flow_put.key_len = key_len;
>> - op->op.u.flow_put.mask = mask + nlattr_delta;
>> + op->op.u.flow_put.mask = mask;
>> op->op.u.flow_put.mask_len = mask_len;
>> op->op.u.flow_put.stats = NULL;
>> }
>> @@ -1580,9 +1574,10 @@ revalidate(struct revalidator *revalidator)
>> if (result == UKEY_DELETE) {
>> delete_op_init(&ops[n_ops++], key, key_len, ukey);
>> } else if (result == UKEY_MODIFY) {
>> + struct ofpbuf *actions_copy = ofpbuf_clone(&odp_actions);
>> modify_op_init(&ops[n_ops++], key, key_len, mask, mask_len,
>> - ofpbuf_data(&odp_actions),
>> - ofpbuf_size(&odp_actions), ukey, NULL);
>> + ofpbuf_data(actions_copy),
>> + ofpbuf_size(actions_copy), ukey, actions_copy);
>> }
>> ovs_mutex_unlock(&ukey->mutex);
>>
>> @@ -1661,10 +1656,18 @@ revalidator_sweep__(struct revalidator *revalidator,
>> bool purge)
>> if (result == UKEY_DELETE) {
>> delete_op_init(&ops[n_ops++], ukey->key, ukey->key_len, ukey);
>> } else if (result == UKEY_MODIFY) {
>> + /* Append 'actions' to 'buf', properly adjusting 'mask' in
>> + * case this expands 'buf'. */
>> + struct nlattr *actions;
>> + buf->frame = mask;
>> + actions = ofpbuf_put(buf, ofpbuf_data(&odp_actions),
>> + ofpbuf_size(&odp_actions));
>> + mask = buf->frame;
>> +
>> /* Takes ownership of 'buf'. */
>> modify_op_init(&ops[n_ops++], ukey->key, ukey->key_len, mask,
>> - mask_len, ofpbuf_data(&odp_actions),
>> - ofpbuf_size(&odp_actions), ukey, buf);
>> + mask_len, actions, ofpbuf_size(&odp_actions),
>> + ukey, buf);
>> buf = NULL;
>> }
>> ofpbuf_delete(buf);
>>
>>
>>>
>>> Jarno
>>>
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
