> On Feb 4, 2016, at 3:22 PM, Ben Pfaff <[email protected]> wrote:
>
> On Thu, Feb 04, 2016 at 03:08:48PM -0800, Jarno Rajahalme wrote:
>>
>>> On Feb 4, 2016, at 3:00 PM, Ben Pfaff <[email protected]> wrote:
>>>
>>> On Wed, Feb 03, 2016 at 12:33:01PM -0800, Jarno Rajahalme wrote:
>>>> From: Ethan Jackson <[email protected]>
>>>>
>>>> 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 <[email protected]>
>>>> Signed-off-by: Jarno Rajahalme <[email protected]>
>>>
>>> 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
[email protected]
http://openvswitch.org/mailman/listinfo/dev
