[ovs-dev] [PATCH 03/12] flow: Introduce parse_dl_type().
Fischetti, Antonio
antonio.fischetti at intel.com
Tue Feb 2 10:28:04 UTC 2016
Yes, ALWAYS_INLINE looks a good solution.
miniflow_extract() is one of the most cpu-consuming functions.
Perf tool in a phy-to-phy test without CT was infact showing
29% emc_processing()
20% miniflow_extract() <==
13% _recv_raw_pkts_vec()
11% ixgbe_xmit_pkts_vec()
8% dp_netdev_process_rxq_port()
3% netdev_dpdk_rxq_recv()
...
> -----Original Message-----
> From: Daniele Di Proietto [mailto:diproiettod at vmware.com]
> Sent: Monday, February 1, 2016 7:38 PM
> To: Fischetti, Antonio
> Cc: dev at openvswitch.org
> Subject: Re: [ovs-dev] [PATCH 03/12] flow: Introduce parse_dl_type().
>
> Hi Antonio,
>
> Thanks for investigating and sharing these results.
>
> You're right, this patch changes slightly the assembly output in
> miniflow_extract() and introduces a performance regression.
>
>
> On 01/02/2016 03:13, "Fischetti, Antonio" <antonio.fischetti at intel.com>
> wrote:
>
> >Hi, I was running some Regression Test to check performance in a
> >Phy-2-Phy test, ie
> >ovs-ofctl add-flow br0 in_port=1,action=output:2
> >
> >After applying patches #1 and then #2 the performance
> >is still ok.
> >Instead, after I apply this patch #3 the throughput goes from 12.1
> >down to 11.6 Mpps.
> >
> >I found out 2 strange alternative ways to restore performance.
> >Either:
> >1. Comment the new parse_dl_type() function. I know it's odd
> >because it isn't yet called from anywhere.
>
> I guess that the compiler doesn't know that when it's compiling
> flow.c.
>
> >Or:
> >2. Adding these weird changes to the 2 function prototypes which are
> >called by parse_dl_type()
> >
> >- static inline ovs_be16
> >+/*static inline*/ ovs_be16
> >parse_vlan(const void **datap, size_t *sizep)
> >
> >and
> >
> >- static inline ovs_be16
> >+ /*static*/ inline ovs_be16
> >parse_ethertype(const void **datap, size_t *sizep)
>
> It looks like my patch prevents the compiler from inlining part of
> these functions, and this confirms it.
>
> I propose the following incremental, that seems to resolve the
> regression. What do you think?
>
> ---8<---
>
> diff --git a/lib/flow.c b/lib/flow.c
> index 08e199c..238a7f5 100644
> --- a/lib/flow.c
> +++ b/lib/flow.c
> @@ -290,7 +290,7 @@ parse_mpls(const void **datap, size_t *sizep)
> return MIN(count, FLOW_MAX_MPLS_LABELS);
> }
>
> -static inline ovs_be16
> +static inline ALWAYS_INLINE ovs_be16
> parse_vlan(const void **datap, size_t *sizep)
> {
> const struct eth_header *eth = *datap;
> @@ -312,7 +312,7 @@ parse_vlan(const void **datap, size_t *sizep)
> return 0;
> }
>
> -static inline ovs_be16
> +static inline ALWAYS_INLINE ovs_be16
> parse_ethertype(const void **datap, size_t *sizep)
> {
> const struct llc_snap_header *llc;
>
>
> ---8<---
>
> I'm not a fan of this kind of optimizations, but I believe that
> miniflow_extract() deserves some special treatment.
>
> Thanks,
>
> Daniele
>
>
> >
> >It looks like some compiler optimization is failing for some reason?
> >I didn't investigate further on the output map of the executable.
> >
> >Antonio
> >
> >
> >> -----Original Message-----
> >> From: dev [mailto:dev-bounces at openvswitch.org] On Behalf Of Daniele
> Di
> >> Proietto
> >> Sent: Monday, November 16, 2015 6:21 AM
> >> To: dev at openvswitch.org
> >> Subject: [ovs-dev] [PATCH 03/12] flow: Introduce parse_dl_type().
> >>
> >> The function simply returns the ethernet type of the packet (after
> >> eventually discarding the VLAN tag). It will be used by a following
> >> commit.
> >>
> >> Signed-off-by: Daniele Di Proietto <diproiettod at vmware.com>
> >> ---
> >> lib/flow.c | 10 ++++++++++
> >> lib/flow.h | 1 +
> >> 2 files changed, 11 insertions(+)
> >>
> >> diff --git a/lib/flow.c b/lib/flow.c
> >> index 2bdce26..d8c1228 100644
> >> --- a/lib/flow.c
> >> +++ b/lib/flow.c
> >> @@ -789,6 +789,16 @@ miniflow_extract(struct dp_packet *packet,
> struct
> >> miniflow *dst)
> >> dst->map = mf.map;
> >> }
> >>
> >> +ovs_be16
> >> +parse_dl_type(const struct eth_header *data_, size_t size)
> >> +{
> >> + const void *data = data_;
> >> +
> >> + parse_vlan(&data, &size);
> >> +
> >> + return parse_ethertype(&data, &size);
> >> +}
> >> +
> >> /* For every bit of a field that is wildcarded in 'wildcards', sets the
> >> * corresponding bit in 'flow' to zero. */
> >> void
> >> diff --git a/lib/flow.h b/lib/flow.h
> >> index de15a98..7465af8 100644
> >> --- a/lib/flow.h
> >> +++ b/lib/flow.h
> >> @@ -237,6 +237,7 @@ void flow_compose(struct dp_packet *, const
> struct
> >> flow *);
> >>
> >> bool parse_ipv6_ext_hdrs(const void **datap, size_t *sizep, uint8_t
> >> *nw_proto,
> >> uint8_t *nw_frag);
> >> +ovs_be16 parse_dl_type(const struct eth_header *data_, size_t size);
> >>
> >> static inline uint64_t
> >> flow_get_xreg(const struct flow *flow, int idx)
> >> --
> >> 2.1.4
> >>
> >> _______________________________________________
> >> dev mailing list
> >> dev at openvswitch.org
> >> http://openvswitch.org/mailman/listinfo/dev
More information about the dev
mailing list