[ovs-dev] [PATCH v2] odp-execute: Optimize IP header modification in OVS datapath

Zoltán Balogh zoltan.balogh at ericsson.com
Thu Dec 22 10:05:33 UTC 2016


Hi Daniele,

Thank you for the confirmation. I've used Ivy Bridge Xeon E5-2658 v2 running at 3GHz. This is an older architecture than yours. Your results look better for the new patch. I think we should take the new patch.

Best regards,
Zoltan

-----Original Message-----
From: Daniele Di Proietto [mailto:diproiettod at ovn.org] 
Sent: Thursday, December 22, 2016 4:06 AM
To: Zoltán Balogh <zoltan.balogh at ericsson.com>
Cc: dev at openvswitch.org
Subject: Re: [ovs-dev] [PATCH v2] odp-execute: Optimize IP header modification in OVS datapath

2016-12-13 9:27 GMT-08:00 Zoltán Balogh <zoltan.balogh at ericsson.com>:
>
> Hi Daniele,
>
>> Have you tried avoiding also computing the new field if the mask is 0?
>> For example, if
>> mask->ipv4_src is 0, there's not reason to compute new_ip_src, or to
>> extract ip_src_nh.
>
> Yes, I have investigated this case in mid-October. My results was
> that execution of "output + dec_ttl" and "output + mod_nw_tos" became even
> faster, but execution of rest of the actions I investigated became slower
> compared to the patch I posted previously.
> So I rejected this modification.
>
> Now, I rebased the code to 9th Dec master and ran the tests with DPDK 16.11
> again. Please find the new rebased patch below.
> The results of my original and the new patch:
>
>        | T | T | I | I |
>        | T | O | P | P |  Vanilla OVS  ||  + old patch  |  + new patch
>        | L | S | s | d | (nsec/packet) || (nsec/packet) | (nsec/packet)
> -------+---+---+---+---+---------------++---------------+---------------
> output |   |   |   |   |    67.19      ||    67.19      |    67.19
>        | X |   |   |   |    74.48      ||    69.53      |    68.78    (+)
>        |   | X |   |   |    74.42      ||    71.20      |    70.07    (+)
>        |   |   | X |   |    84.62      ||    78.70      |    78.03    (+)
>        |   |   |   | X |    84.25      ||    78.51      |    77.94    (+)
>        |   |   | X | X |    97.46      ||    91.98      |    91.86    (+)
>        | X |   | X | X |   100.42      ||    95.00      |    96.00    (-)
>        | X | X | X | X |   102.80      ||   100.40      |   100.73    (-)
>
> I ran each test five times. The values are the mean of the readings obtained.
>
> As you can see, there is an improvement in the first 5 cases. But, in case of
> "output + dec_ttl + mod_nw_src + mod_nw_dst" and
> "output + dec_ttl + mod_nw_tos + mod_nw_src + mod_nw_dst", the execution of
> actions became slower compared to the old patch. However, this is still faster
> than vanilla OVS.
>
> Could you please confirm the results?
> Are these values acceptable?
> Is there anything still to improve in the new patch?

Sorry for the delay and thanks for the details and the new version

I've tried to reproduce your tests (64 bytes UDP phy-phy throughput)
on my system (2 Ghz haswell with ixgbe).

Here are my results:

       | T | T | I | I |
       | T | O | P | P |  Vanilla OVS  ||  + old patch  |  + new patch
       | L | S | s | d | (nsec/packet) || (nsec/packet) | (nsec/packet)
-------+---+---+---+---+---------------++---------------+---------------
output |   |   |   |   |    67.20      ||    67.20      |    67.20
       | X |   |   |   |    87.03      ||    82.17      |    80.19    (+)
       |   | X |   |   |    87.03      ||    81.37      |    81.50    (-)
       |   |   | X |   |    95.88      ||    90.66      |    87.26    (+)
       |   |   |   | X |    95.51      ||    90.42      |    87.18    (+)
       |   |   | X | X |   107.53      ||   103.41      |   100.20    (+)
       | X |   | X | X |   111.98      ||   108.23      |   105.49    (+)
       | X | X | X | X |   116.01      ||   112.49      |   111.11    (+)

There are no regressions compared to master in any case, which is good.

Both versions look good to me.  Since you did the original profiling, which
version do you prefer?

Thanks,

Daniele

>
> The patch was applied to: 7971b36c3acc279f8e931d360f16c200752a3be2
>
> Best regards,
> Zoltan
>
> Signed-off-by: Zoltán Balogh <zoltan.balogh at ericsson.com>
>
> ---
>
> diff --git a/lib/odp-execute.c b/lib/odp-execute.c
> index 65a6fcd..cc555b8 100644
> --- a/lib/odp-execute.c
> +++ b/lib/odp-execute.c
> @@ -33,6 +33,7 @@
>  #include "flow.h"
>  #include "unaligned.h"
>  #include "util.h"
> +#include "csum.h"
>
>  /* Masked copy of an ethernet address. 'src' is already properly masked. */
>  static void
> @@ -68,13 +69,50 @@ odp_set_ipv4(struct dp_packet *packet, const struct ovs_key_ipv4 *key,
>               const struct ovs_key_ipv4 *mask)
>  {
>      struct ip_header *nh = dp_packet_l3(packet);
> +    ovs_be32 ip_src_nh;
> +    ovs_be32 ip_dst_nh;
> +    ovs_be32 new_ip_src;
> +    ovs_be32 new_ip_dst;
> +    uint8_t new_tos;
> +    uint8_t new_ttl;
> +
> +    if (mask->ipv4_src) {
> +        ip_src_nh = get_16aligned_be32(&nh->ip_src);
> +        new_ip_src = key->ipv4_src | (ip_src_nh & ~mask->ipv4_src);
> +
> +        if (ip_src_nh != new_ip_src) {
> +            packet_set_ipv4_addr(packet, &nh->ip_src, new_ip_src);
> +        }
> +    }
>
> -    packet_set_ipv4(
> -        packet,
> -        key->ipv4_src | (get_16aligned_be32(&nh->ip_src) & ~mask->ipv4_src),
> -        key->ipv4_dst | (get_16aligned_be32(&nh->ip_dst) & ~mask->ipv4_dst),
> -        key->ipv4_tos | (nh->ip_tos & ~mask->ipv4_tos),
> -        key->ipv4_ttl | (nh->ip_ttl & ~mask->ipv4_ttl));
> +    if (mask->ipv4_dst) {
> +        ip_dst_nh = get_16aligned_be32(&nh->ip_dst);
> +        new_ip_dst = key->ipv4_dst | (ip_dst_nh & ~mask->ipv4_dst);
> +
> +        if (ip_dst_nh != new_ip_dst) {
> +            packet_set_ipv4_addr(packet, &nh->ip_dst, new_ip_dst);
> +        }
> +    }
> +
> +    if (mask->ipv4_tos) {
> +        new_tos = key->ipv4_tos | (nh->ip_tos & ~mask->ipv4_tos);
> +
> +        if (nh->ip_tos != new_tos) {
> +            nh->ip_csum = recalc_csum16(nh->ip_csum, htons((uint16_t) nh->ip_tos),
> +                                        htons((uint16_t) new_tos));
> +            nh->ip_tos = new_tos;
> +        }
> +    }
> +
> +    if (OVS_LIKELY(mask->ipv4_ttl)) {
> +        new_ttl = key->ipv4_ttl | (nh->ip_ttl & ~mask->ipv4_ttl);
> +
> +        if (OVS_LIKELY(nh->ip_ttl != new_ttl)) {
> +            nh->ip_csum = recalc_csum16(nh->ip_csum, htons(nh->ip_ttl << 8),
> +                                        htons(new_ttl << 8));
> +            nh->ip_ttl = new_ttl;
> +        }
> +    }
>  }
>
>  static const ovs_be32 *
> diff --git a/lib/packets.c b/lib/packets.c
> index ae81f46..4a5eb72 100644
> --- a/lib/packets.c
> +++ b/lib/packets.c
> @@ -849,7 +849,7 @@ eth_compose(struct dp_packet *b, const struct eth_addr eth_dst,
>      return data;
>  }
>
> -static void
> +void
>  packet_set_ipv4_addr(struct dp_packet *packet,
>                       ovs_16aligned_be32 *addr, ovs_be32 new_addr)
>  {
> diff --git a/lib/packets.h b/lib/packets.h
> index 21bd35c..97d1b38 100644
> --- a/lib/packets.h
> +++ b/lib/packets.h
> @@ -1093,6 +1093,8 @@ void *snap_compose(struct dp_packet *, const struct eth_addr eth_dst,
>                     unsigned int oui, uint16_t snap_type, size_t size);
>  void packet_set_ipv4(struct dp_packet *, ovs_be32 src, ovs_be32 dst, uint8_t tos,
>                       uint8_t ttl);
> +void packet_set_ipv4_addr(struct dp_packet *packet, ovs_16aligned_be32 *addr,
> +                          ovs_be32 new_addr);
>  void packet_set_ipv6(struct dp_packet *, const ovs_be32 src[4],
>                       const ovs_be32 dst[4], uint8_t tc,
>                       ovs_be32 fl, uint8_t hlmit);
>
>


More information about the dev mailing list