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