[ovs-dev] [PATCH v4 4/9] classifier: Pre-compute stage masks.

Jarno Rajahalme jrajahalme at nicira.com
Fri Aug 21 21:36:40 UTC 2015


> On Aug 21, 2015, at 8:30 AM, Ben Pfaff <blp at nicira.com> 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 <jrajahalme at nicira.com>
> 
> 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





More information about the dev mailing list