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

Fischetti, Antonio antonio.fischetti at intel.com
Fri Jan 6 09:47:45 UTC 2017


Thanks Jarno for your feedback, will respin a new version with the suggested fix.

Regards,
Antonio

> -----Original Message-----
> From: Jarno Rajahalme [mailto:jarno at ovn.org]
> Sent: Thursday, January 5, 2017 11:30 PM
> To: Fischetti, Antonio <antonio.fischetti at intel.com>
> Cc: dev at openvswitch.org; Bodireddy, Bhanuprakash
> <bhanuprakash.bodireddy at intel.com>
> Subject: Re: [PATCH v2] dpcls: Avoid one 8-byte chunk in subtable mask.
> 
> 
> > On Jan 5, 2017, at 1:03 AM, antonio.fischetti at intel.com wrote:
> >
> > This patch allows to skip the chunk comprising of dp_hash and in_port
> > in the subtable mask when the packet is not recirculated.  This will
> > slightly speed up the hash computation as one expensive function call
> > to hash_add64() can be skipped.
> > For each new netdev flow we wildcard in_port in the mask, so in the
> > physical case where dp_hash is also wildcarded, the resulting 8-byte
> > chunk will not be part of the subtable mask.
> >
> 
> It appears you have not run the testsuite with this patch, as it
> consistently fails. This is due to dumped flows now missing the in_port
> match, which is caused by the subtable mask change. This can be fixed like
> this:
> 
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index 0b73056..5f6fc01 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -2098,6 +2098,9 @@ dp_netdev_flow_to_dpif_flow(const struct
> dp_netdev_flow *netdev_flow,
>          };
> 
>          miniflow_expand(&netdev_flow->cr.mask->mf, &wc.masks);
> +        /* in_port is exact matched, but we have left it out from the
> mask for
> +         * optimization reasons. Add in_port back to the mask. */
> +        wc.masks.in_port.odp_port = ODPP_NONE;
> 
>          /* Key */
>          offset = key_buf->size;
> 
> > The idea behind is that all the packets within a given dpcls will
> > have the same in_port value and typically dp_hash == 0.  So they will
> > have the same 8-bytes chunk in their {dp_hash, in_port} portion of the
> > miniflow. This doesn't contribute effectively in spreading hash values
> > and avoiding collisions.
> >
> > v2: Using ovs_assert
> >
> 
> Now there are two asserts for the same thing, I think the latter (the old
> one) should be removed.
> 
>   Jarno
> 
> > 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>
> > Signed-off-by: Jarno Rajahalme <jarno at ovn.org>
> > Co-authored-by: Jarno Rajahalme <jarno at ovn.org>
> > ---
> > lib/dpif-netdev.c | 11 +++++++++++
> > 1 file changed, 11 insertions(+)
> >
> > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> > index 1a47a45..68b434f 100644
> > --- a/lib/dpif-netdev.c
> > +++ b/lib/dpif-netdev.c
> > @@ -2278,7 +2278,18 @@ dp_netdev_flow_add(struct dp_netdev_pmd_thread
> *pmd,
> >     struct dpcls *cls;
> >     odp_port_t in_port = match->flow.in_port.odp_port;
> >
> > +    /* As we select the dpcls based on the port number, each netdev
> flow
> > +     * belonging to the same dpcls will have the same odp_port value.
> > +     * For performance reasons here we wildcard odp_port in the mask.
> In the
> > +     * physical case where dp_hash is also wildcarded, the resulting 8-
> byte
> > +     * chunk {dp_hash, in_port} will be ignored by
> netdev_flow_mask_init() and
> > +     * will not be part of the subtable mask.
> > +     * This will speed up the hash computation during dpcls_lookup()
> because
> > +     * one call to hash_add64() will be skipped. */
> > +    ovs_assert(match->wc.masks.in_port.odp_port == ODPP_NONE);
> > +    match->wc.masks.in_port.odp_port = 0;
> >     netdev_flow_mask_init(&mask, match);
> > +    match->wc.masks.in_port.odp_port = ODPP_NONE;
> >     /* 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
> >



More information about the dev mailing list