[ovs-dev] [PATCH RFC] dpcls: Avoid one 8-byte chunk in subtable mask.

[Jarno Rajahalme]

> On Dec 19, 2016, at 2:43 PM, antonio.fischetti at intel.com wrote:
> 
> This patch skips the chunk comprising of dp_hash and in_port in the
> subtable mask when the dpcls refers to a physical port.  This will
> slightly speed up the hash computation as one expensive function call
> to hash_add64() can be skipped.
> 

Do you have any performance test results to share?

> The idea behind is that the packets coming from a physical port
> shall have the same 8-bytes chunk in their dp_hash/in_port portion of
> the miniflow.  When hash is computed in the NIC, dp_hash is zero leaving
> only the in_port in the chunk.  This doesn't contribute effectively in
> spreading hash values and avoiding collisions.
> This approach could be extended to other port types.

At first this seems too hacky for me. However, since the dpcls is explicitly selected for the in_port, it makes sense to wildcard the in_port in the dpcls lookup itself. This has nothing to do with the port type. For dp_hash, it would typically not be matched, so there is no need to worry about it. Also, when it is matched (after recirculation), the in_port value would be the same as before recirculation, so it is not safe to assume anything about dp_hash based on the in_port value.

Proposals for improvement below.

> 
> Signed-off-by: Antonio Fischetti <antonio.fischetti at intel.com>
> Signed-off-by: Bhanuprakash Bodireddy <bhanuprakash.bodireddy at intel.com>
> Co-authored-by: Bhanuprakash Bodireddy bhanuprakash.bodireddy at intel.com
> ---
> lib/dpif-netdev.c | 28 ++++++++++++++++++++++++++--
> 1 file changed, 26 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index 1a47a45..cd8715f 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -1856,18 +1856,34 @@ netdev_flow_key_from_flow(struct netdev_flow_key *dst,
> /* Initialize a netdev_flow_key 'mask' from 'match'. */
> static inline void
> netdev_flow_mask_init(struct netdev_flow_key *mask,
> -                      const struct match *match)
> +                      const struct match *match,
> +                      char * port_type)

It’s better to handle all this in the caller and not modify this function at all.

> {
>     uint64_t *dst = miniflow_values(&mask->mf);
>     struct flowmap fmap;
>     uint32_t hash = 0;
>     size_t idx;
> +    bool phy_port = false;
> +
> +    if (port_type && !strcmp(port_type, "dpdk")) {
> +        phy_port = true;
> +    }
> 

>     /* Only check masks that make sense for the flow. */
>     flow_wc_map(&match->flow, &fmap);
>     flowmap_init(&mask->mf.map);
> +    /* Check that dp_hash and in_port must be into the same structure chunk. */
> +    BUILD_ASSERT_DECL(offsetof(struct flow, dp_hash)/sizeof(*dst) ==
> +            offsetof(struct flow, in_port)/sizeof(*dst));
> +#define DPHASH_INPORT_MAP_IDX (offsetof(struct flow, dp_hash)/sizeof(*dst))
> 
>     FLOWMAP_FOR_EACH_INDEX(idx, fmap) {
> +        if (phy_port && idx == DPHASH_INPORT_MAP_IDX) {
> +            /* This chunk contains Sw-Hash and Port Nr.  As all packets coming
> +             * from the same physical port have the same in_port value
> +             * and dp_hash set to 0, this 8-bytes chunk will be ignored. */
> +            continue;
> +        }
>         uint64_t mask_u64 = flow_u64_value(&match->wc.masks, idx);
> 
>         if (mask_u64) {
> @@ -2278,7 +2294,15 @@ dp_netdev_flow_add(struct dp_netdev_pmd_thread *pmd,
>     struct dpcls *cls;
>     odp_port_t in_port = match->flow.in_port.odp_port;
> 
> -    netdev_flow_mask_init(&mask, match);
> +    struct dpif_port dpifport;
> +
> +    if (pmd && pmd->dp) {
> +        const struct dpif * pdpif = pmd->dp->dpif;
> +        if (pdpif) {
> +            dpif_netdev_port_query_by_number(pdpif, in_port, &dpifport);
> +        }
> +    }
> +    netdev_flow_mask_init(&mask, match, dpifport.type);

Instead of the above you could move the in_port.odp_port assert later in the function to be done before the netdev_flow_mask_init() call, and then after the assert zero it out, and after the netdev_flow_mask_init() make it all-ones again.

This has the effect of always wildcarding in_port in the dpcls, which is explicitly selected based on the port number. Then, in the typical case where dp_hash is also wildcarded, the end result is the same as what you intended with this patch.

  Jarno

>     /* Make sure wc does not have metadata. */
>     ovs_assert(!FLOWMAP_HAS_FIELD(&mask.mf.map, metadata)
>                && !FLOWMAP_HAS_FIELD(&mask.mf.map, regs));
> -- 
> 2.4.11
>