This seems like a worthwhile optimization but the patch is corrupted. Will you rebase and resend?
Thanks, Ben. On Tue, Nov 01, 2016 at 06:28:36PM +0000, Zoltán Balogh wrote: > > Hi, > > I measured the packet processing cost of OVS DPDK datapath for different > OpenFlow actions. I configured OVS to use a single pmd thread and measured > the > packet throughput in a phy-to-phy setup. I used 10G interfaces bounded to > DPDK > driver and overloaded the vSwitch with 64 byte packets through one of the 10G > interfaces. > > The processing cost of the dec_ttl action seemed to be gratuitously high > compared with other actions. > > I looked into the code and saw that dec_ttl is encoded as a masked nested > attribute in OVS_ACTION_ATTR_SET_MASKED(OVS_KEY_ATTR_IPV4). That way, OVS > datapath can modify several IP header fields (TTL, TOS, source and > destination > IP addresses) by a single invocation of packet_set_ipv4() in the > odp_set_ipv4() > function in the lib/odp-execute.c file. > The packet_set_ipv4() function takes the new TOS, TTL and IP addresses as > arguments, compares them with the actual ones and updates the fields if > needed. > This means, that even if only TTL needs to be updated, each of the four IP > header fields is passed to the callee and is compared to the actual field for > each packet. > > The odp_set_ipv4() caller function possesses information about the fields > that > need to be updated in the 'mask' structure. The idea is to spare invocation > of > thepacket_set_ipv4() function but use its code parts directly. So the 'mask' > can be used to decide which IP header fields need to be updated. In addition, > a faster packet processing can be achieved if the values of local variables > are > calculated right before their usage. > > These are the results of my measurements. > > | T | T | I | I | > | T | O | P | P | Vanilla OVS | OVS + patch > | L | S | s | d | (nsec/packet) | (nsec/packet) > -------+---+---+---+---+---------------+--------------- > output | | | | | 67.20 | 67.20 > | X | | | | 74.34 | 70.03 > | | X | | | 74.20 | 71.06 > | | | X | | 83.14 | 78.67 > | | | | X | 83.27 | 78.00 > | | | X | X | 96.68 | 91.81 > | X | | X | X | 99.58 | 96.00 > | X | X | X | X | 103.31 | 100.73 > > The table shows the average processing cost of packets in nanoseconds for the > following actions: > output; output + dec_ttl; output + mod_nw_tos; output + mod_nw_src; > output + mod_nw_dst and some of their combinations. > > I added OVS_LIKELY to the 'if' condition for the TTL field, since as far as I > know, this field will typically be decremented when any field of the IP > header > is modified. > > Please find the patch below. > > Signed-off-by: Zoltán Balogh <[email protected]> > > --- > > diff --git a/lib/odp-execute.c b/lib/odp-execute.c > index 65a6fcd..131532d 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,42 @@ 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; > + > + ip_src_nh = get_16aligned_be32(&nh->ip_src); > + new_ip_src = key->ipv4_src | (ip_src_nh & ~mask->ipv4_src); > + > + if (mask->ipv4_src && 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)); > + ip_dst_nh = get_16aligned_be32(&nh->ip_dst); > + new_ip_dst = key->ipv4_dst | (ip_dst_nh & ~mask->ipv4_dst); > + > + if (mask->ipv4_dst && ip_dst_nh != new_ip_dst) { > + packet_set_ipv4_addr(packet, &nh->ip_dst, new_ip_dst); > + } > + > + new_tos = key->ipv4_tos | (nh->ip_tos & ~mask->ipv4_tos); > + > + if (mask->ipv4_tos && 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; > + } > + > + new_ttl = key->ipv4_ttl | (nh->ip_ttl & ~mask->ipv4_ttl); > + > + if (OVS_LIKELY(mask->ipv4_ttl && 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 990c407..179841a 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); > _______________________________________________ > dev mailing list > [email protected] > http://openvswitch.org/mailman/listinfo/dev _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
