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'); > Acked-by: Jarno Rajahalme <jrajaha...@nicira.com> Thanks! _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
