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:
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
