On Fri, Jul 31, 2015 at 01:28:43PM -0700, Jarno Rajahalme wrote:
>
> > On Jul 31, 2015, at 1:06 PM, Ben Pfaff <b...@nicira.com> wrote:
> >
> > On Fri, Jul 31, 2015 at 11:39:07AM -0700, Jarno Rajahalme wrote:
> >>> On Jul 29, 2015, at 11:42 PM, Ben Pfaff <b...@nicira.com> wrote:
> >>>
> >>> Until now, struct xlate_out has embedded a struct flow_wildcards, which
> >>> xlate_actions() filled in during the flow translation process (unless this
> >>> was disabled with xin->skip_wildcards to in theory save time). This
> >>> commit
> >>
> >> I have recently noticed with an upcoming "test-classifier benchmark”
> >> that computing the wildcards roughly doubles the classifier lookup
> >> time.
> >
> > Thanks, I'll add that info to the commit message.
> >
> >>> removes the embedded flow_wildcards and 'skip_wildcards', instead putting
> >>> a pointer to a flow_wildcards into struct xlate_in, for a caller to fill
> >>> in with a pointer to its own structure if desired.
> >>
> >> I was concerned about the scratch wc at first, but then I noticed that
> >> no behavior should be changed, as the xlate_out wc was previously used
> >> similarly.
> >
> > That's correct, I'll clarify that in the commit message too.
> >
> >>> One reason for this change is performance. Until now, the userspace slow
> >>> path has done a full copy of a struct flow_wildcards for each upcall in
> >>> upcall_cb(). This commit eliminates that copy. I don't know whether this
> >>> has a measurable performance impact; it may not, especially since this is
> >>> on the slow path.
> >>
> >> Struct flow copies used to be noticeable in slow-path stress test
> >> traces even when it was half the current size, so IMO it is good to
> >> get rid of unnecessary init and copy operations.
> >
> > Great.
> >
> >>> This commit also eliminates a large data structure from struct xlate_out,
> >>> reducing the cost of the initialization of that structure at the beginning
> >>> of xlate_actions(). However, there is more size reduction to come in
> >>> later commits.
> >>>
> >>
> >> I was a bit baffled by the test case change. Do you know why the
> >> change was necessary, or why the explicit mask was not needed before
> >> this patch?
> >>
> >> Looking at the end of xlate_actions(), we should have cleared the
> >> upper bits of imp code and type already before?
> >
> > The test case change is due to the following. It was necessary because
> > xout.wc went away. Also, I couldn't figure out what the motivation for
> > it was, either by thinking about it or by examining the history.
> >
> > I could break that change out as a separate commit, if you like. It
> > would make it more obvious why the test case changed.
> >
> >>> @@ -4548,7 +4548,6 @@ trace_format_megaflow(struct ds *result, int level,
> >>> const char *title,
> >>>
> >>> ds_put_char_multiple(result, '\t', level);
> >>> ds_put_format(result, "%s: ", title);
> >>> - flow_wildcards_or(&trace->wc, &trace->xout.wc, &trace->wc);
> >>> match_init(&match, trace->key, &trace->wc);
> >>> match_format(&match, result, OFP_DEFAULT_PRIORITY);
> >>> ds_put_char(result, '\n');
> >
>
> This seems like an attempt to accumulate the masks across resubmits, but the
> xout.wc was already properly accumulated, so this is not necessary, and
> actually turned out to be “harmful” after we added the clearing of the upper
> bits for ICMP, which this added back.
>
> No need to split this, but could mention this in commit message. This is a
> bug fix in trace output, IMO.
Too late ;-) I broke it out as:
--8<--------------------------cut here-------------------------->8--
From: Ben Pfaff <b...@nicira.com>
Date: Fri, 31 Jul 2015 13:33:16 -0700
Subject: [PATCH] ofproto-dpif: Fix inaccurate wildcard output in
ofproto/trace.
Until now, the ofproto/trace command has tried to accumulate wildcards
independently of flow translation. This was unnecessary, and in a few
cases where flow translation drops wildcards, it meant that ofproto/trace
printed inaccurate wildcards (because it keep the wildcards that flow
translation dropped).
This updates a test case whose output is now more accurate.
Signed-off-by: Ben Pfaff <b...@nicira.com>
---
ofproto/ofproto-dpif.c | 6 +-----
tests/ofproto-dpif.at | 2 +-
2 files changed, 2 insertions(+), 6 deletions(-)
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index d06d765..5826c9f 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -4464,7 +4464,6 @@ struct trace_ctx {
struct xlate_in xin;
const struct flow *key;
struct flow flow;
- struct flow_wildcards wc;
struct ds *result;
};
@@ -4548,8 +4547,7 @@ trace_format_megaflow(struct ds *result, int level, const
char *title,
ds_put_char_multiple(result, '\t', level);
ds_put_format(result, "%s: ", title);
- flow_wildcards_or(&trace->wc, &trace->xout.wc, &trace->wc);
- match_init(&match, trace->key, &trace->wc);
+ match_init(&match, trace->key, &trace->xout.wc);
match_format(&match, result, OFP_DEFAULT_PRIORITY);
ds_put_char(result, '\n');
}
@@ -4891,8 +4889,6 @@ ofproto_trace(struct ofproto_dpif *ofproto, struct flow
*flow,
flow_format(ds, flow);
ds_put_char(ds, '\n');
- flow_wildcards_init_catchall(&trace.wc);
-
trace.result = ds;
trace.key = flow; /* Original flow key, used for megaflow. */
trace.flow = *flow; /* May be modified by actions. */
diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
index 7000e4f..2656ca7 100644
--- a/tests/ofproto-dpif.at
+++ b/tests/ofproto-dpif.at
@@ -270,7 +270,7 @@ table=2 in_port=1,icmp6,icmpv6_type=135
actions=set_field:fe80::4->nd_target,set
AT_CHECK([ovs-ofctl -O OpenFlow12 add-flows br0 flows.txt])
AT_CHECK([ovs-appctl ofproto/trace br0
'in_port=1,dl_src=50:54:00:00:00:05,dl_dst=50:54:00:00:00:07,icmp6,ipv6_src=fe80::1,ipv6_dst=fe80::2,nw_tos=0,nw_ttl=128,icmpv6_type=135,nd_target=fe80::2020,nd_sll=66:55:44:33:22:11'],
[0], [stdout])
AT_CHECK([tail -4 stdout], [0],
- [Megaflow:
recirc_id=0,icmp6,in_port=1,nw_frag=no,icmp_type=135,icmp_code=0x0/0xff,nd_target=fe80::2020,nd_sll=66:55:44:33:22:11
+ [Megaflow:
recirc_id=0,icmp6,in_port=1,nw_frag=no,icmp_type=0x87/0xff,icmp_code=0x0/0xff,nd_target=fe80::2020,nd_sll=66:55:44:33:22:11
Datapath actions:
10,set(nd(target=fe80::4,sll=cc:cc:cc:cc:cc:cc)),11,set(nd(target=fe80::3,sll=aa:aa:aa:aa:aa:aa)),13
This flow is handled by the userspace slow path because it:
- Uses action(s) not supported by datapath.
--
2.1.3
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
