> On Aug 21, 2015, at 8:30 AM, Ben Pfaff <[email protected]> wrote:
>
> On Fri, Aug 07, 2015 at 04:57:37PM -0700, Jarno Rajahalme wrote:
>> This makes stage mask computation happen only when a subtable is
>> inserted and allows simplification of the main lookup function.
>>
>> Classifier benchmark shows that this speeds up the classification
>> (with wildcards) about 5%.
>>
>> Signed-off-by: Jarno Rajahalme <[email protected]>
>
> I don't think that flow_hash_in_minimask_range() actually does what its
> name implies anymore (where's the range?). It at least needs a comment
> update since the comment still talks about parameters it doesn't have.
> Ditto for minimatch_hash_range().
>
The ‘maps’ argument really needs to be a successive/continuous subset of the
bits in the masks map. I renamed it as “range” and fixed and elaborated the
comments.
> In a construction like this:
> if (map->tnl_map) {
> MAP_FOR_EACH_INDEX(idx, map->tnl_map) {
> dst_u64[idx] |= *p++;
> }
> }
> it appears to me that the 'if' is redundant. In one case I see there's
> an OVS_UNLIKELY on the 'if' but I wonder whether that's sufficient
> benefit.
>
> If we believe that tnl_map is usually 0, then it would be profitable to
> test pkt_map first below to allow short-circuiting to bail out earlier:
>
> static inline bool
> miniflow_equal_maps(const struct miniflow *a, const struct miniflow *b)
> {
> return a->tnl_map == b->tnl_map && a->pkt_map == b->pkt_map;
> }
>
Right, it has likely only very marginal benefit, so I removed the extra if’s.
>
> While reading code, I came up with some additional comments that are not
> directly related to the changes here.
>
> First, classifier.c has a lot of "static inline" functions. Does the
> "inline" actually produce measurable performance improvement? If not,
> then it's better to avoid "inline" in .c files since it suppresses
> otherwise useful "unused function" warnings.
>
Currently, the compiler inlines all these functions as intended. Earlier I
noticed from the disassembly that inlining was not happening without the
explicit “inline” keywords. Now that the classifier benchmark is in, we can
test if it makes a difference in performance. I’ll do this sometime later.
> Second, the amount of duplicated code because of tnl_map versus pkt_map
> is starting to bug me. If these were just a 2-element array, for
> example, then miniflow_and_mask_matches_flow_wc() could be in my opinion
> tons cleaner as something like this:
>
A later patch of the series does this in a general way (sizing the array
automatically).
> {
> const uint64_t *flowp = miniflow_get_values(flow);
> const uint64_t *maskp = miniflow_get_values(&mask->masks);
> const uint64_t *target_u64 = (const uint64_t *)target;
> uint64_t *wc_u64 = (uint64_t *)&wc->masks;
>
> for (int i = 0; i < 2; i++) {
> size_t idx;
>
> MAP_FOR_EACH_INDEX(idx, mask->masks.maps[i]) {
> uint64_t msk = *maskp++;
>
> uint64_t diff = (*flowp++ ^ target_u64[idx]) & msk;
> if (diff) {
> /* Only unwildcard if none of the differing bits is already
> * exact-matched. */
> if (!(wc_u64[idx] & diff)) {
> /* Keep one bit of the difference. The selected bit may be
> * different in big-endian v.s. little-endian systems. */
> wc_u64[idx] |= rightmost_1bit(diff);
> }
> return false;
> }
>
> /* Fill in the bits that were looked at. */
> wc_u64[idx] |= msk;
> }
> target_u64 += FLOW_TNL_U64S;
> wc_u64 += FLOW_TNL_U64S;
> }
>
> return true;
> }
>
> I noticed that MINIFLOW_IN_MAP could be a function, so I send out a
> patch (which applies following this one):
> http://openvswitch.org/pipermail/dev/2015-August/059029.html
> <http://openvswitch.org/pipermail/dev/2015-August/059029.html>
There might be other macros that could be functions as well in the series, I’ll
check for that.
Jarno
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
