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

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


I sent a v5 of the rest of the series (with some new patches, too).

  Jarno

> On Aug 21, 2015, at 2:36 PM, Jarno Rajahalme <jrajahalme at nicira.com> wrote:
> 
> 
>> On Aug 21, 2015, at 8:30 AM, Ben Pfaff <blp at nicira.com <mailto: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 <mailto: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