On 6/2/20 9:10 AM, Yanqin Wei wrote: > miniflow_extract checks the validation of tunnel metadata by comparing > tunnel destination address, including 16 bytes ipv6 address. > This patch introduces a 'tunnel_valid' flag. If it is false, > md->cacheline2 will not be touched. This improvement is beneficial to > miniflow_extract performance for all kinds of traffic. > > Reviewed-by: Lijian Zhang <lijian.zh...@arm.com> > Reviewed-by: Malvika Gupta <malvika.gu...@arm.com> > Signed-off-by: Yanqin Wei <yanqin....@arm.com> > ---
Hi. First of all, thanks for working on performance improvements! However, this doesn't look as a clean patch. Why we need both pkt_metadata_datapath_init() and pkt_metadata_init() ? Why we can't just not initialize ip_dst and use tunnel_valid flag everywhere? Current version complicates code making it less readable and prone to errors. Best regards, Ilya Maximets. > lib/dpif-netdev.c | 14 +++++++++++--- > lib/flow.c | 2 +- > lib/packets.h | 46 ++++++++++++++++++++++++++++++++++++++++------ > 3 files changed, 52 insertions(+), 10 deletions(-) > > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c > index 51c888501..c94d5e8c7 100644 > --- a/lib/dpif-netdev.c > +++ b/lib/dpif-netdev.c > @@ -6625,12 +6625,16 @@ dfc_processing(struct dp_netdev_pmd_thread *pmd, > if (i != cnt - 1) { > struct dp_packet **packets = packets_->packets; > /* Prefetch next packet data and metadata. */ > - OVS_PREFETCH(dp_packet_data(packets[i+1])); > - pkt_metadata_prefetch_init(&packets[i+1]->md); > + OVS_PREFETCH(dp_packet_data(packets[i + 1])); > + if (md_is_valid) { > + pkt_metadata_prefetch(&packets[i + 1]->md); > + } else { > + pkt_metadata_prefetch_init(&packets[i + 1]->md); > + } > } > > if (!md_is_valid) { > - pkt_metadata_init(&packet->md, port_no); > + pkt_metadata_datapath_init(&packet->md, port_no); > } > > if ((*recirc_depth_get() == 0) && > @@ -6730,6 +6734,10 @@ handle_packet_upcall(struct dp_netdev_pmd_thread *pmd, > miniflow_expand(&key->mf, &match.flow); > memset(&match.wc, 0, sizeof match.wc); > > + if (!packet->md.tunnel_valid) { > + pkt_metadata_tnl_dst_init(&packet->md); > + } > + > ofpbuf_clear(actions); > ofpbuf_clear(put_actions); > > diff --git a/lib/flow.c b/lib/flow.c > index cc1b3f2db..1f0b3d4dc 100644 > --- a/lib/flow.c > +++ b/lib/flow.c > @@ -747,7 +747,7 @@ miniflow_extract(struct dp_packet *packet, struct > miniflow *dst) > ovs_be16 ct_tp_src = 0, ct_tp_dst = 0; > > /* Metadata. */ > - if (flow_tnl_dst_is_set(&md->tunnel)) { > + if (md->tunnel_valid && flow_tnl_dst_is_set(&md->tunnel)) { > miniflow_push_words(mf, tunnel, &md->tunnel, > offsetof(struct flow_tnl, metadata) / > sizeof(uint64_t)); > diff --git a/lib/packets.h b/lib/packets.h > index 447e6f6fa..3b507d2a3 100644 > --- a/lib/packets.h > +++ b/lib/packets.h > @@ -103,15 +103,16 @@ PADDED_MEMBERS_CACHELINE_MARKER(CACHE_LINE_SIZE, > cacheline0, > action. */ > uint32_t skb_priority; /* Packet priority for QoS. */ > uint32_t pkt_mark; /* Packet mark. */ > + struct conn *conn; /* Cached conntrack connection. */ > uint8_t ct_state; /* Connection state. */ > bool ct_orig_tuple_ipv6; > uint16_t ct_zone; /* Connection zone. */ > uint32_t ct_mark; /* Connection mark. */ > ovs_u128 ct_label; /* Connection label. */ > union flow_in_port in_port; /* Input port. */ > - struct conn *conn; /* Cached conntrack connection. */ > bool reply; /* True if reply direction. */ > bool icmp_related; /* True if ICMP related. */ > + bool tunnel_valid; > ); > > PADDED_MEMBERS_CACHELINE_MARKER(CACHE_LINE_SIZE, cacheline1, > @@ -141,6 +142,7 @@ pkt_metadata_init_tnl(struct pkt_metadata *md) > * are before this and as long as they are empty, the options won't > * be looked at. */ > memset(md, 0, offsetof(struct pkt_metadata, tunnel.metadata.opts)); > + md->tunnel_valid = true; > } > > static inline void > @@ -151,6 +153,25 @@ pkt_metadata_init_conn(struct pkt_metadata *md) > > static inline void > pkt_metadata_init(struct pkt_metadata *md, odp_port_t port) > +{ > + /* Initialize only till ct_state. Once the ct_state is zeroed out rest > + * of ct fields will not be looked at unless ct_state != 0. > + */ > + memset(md, 0, offsetof(struct pkt_metadata, ct_orig_tuple_ipv6)); > + > + /* It can be expensive to zero out all of the tunnel metadata. However, > + * we can just zero out ip_dst and the rest of the data will never be > + * looked at. */ > + md->tunnel_valid = true; > + md->tunnel.ip_dst = 0; > + md->tunnel.ipv6_dst = in6addr_any; > + > + md->in_port.odp_port = port; > +} > + > +/* This function initializes those members used by userspace datapath */ > +static inline void > +pkt_metadata_datapath_init(struct pkt_metadata *md, odp_port_t port) > { > /* This is called for every packet in userspace datapath and affects > * performance if all the metadata is initialized. Hence, fields should > @@ -162,12 +183,19 @@ pkt_metadata_init(struct pkt_metadata *md, odp_port_t > port) > memset(md, 0, offsetof(struct pkt_metadata, ct_orig_tuple_ipv6)); > > /* It can be expensive to zero out all of the tunnel metadata. However, > - * we can just zero out ip_dst and the rest of the data will never be > - * looked at. */ > + * we can just clear tunnel_valid */ > + md->tunnel_valid = false; > + > + md->in_port.odp_port = port; > +} > + > +/* This function initializes tunnel dst for upcall */ > +static inline void > +pkt_metadata_tnl_dst_init(struct pkt_metadata *md) > +{ > md->tunnel.ip_dst = 0; > md->tunnel.ipv6_dst = in6addr_any; > - md->in_port.odp_port = port; > - md->conn = NULL; > + md->tunnel_valid = true; > } > > /* This function prefetches the cachelines touched by pkt_metadata_init() > @@ -184,7 +212,13 @@ pkt_metadata_prefetch_init(struct pkt_metadata *md) > * in pkt_metadata_init_tnl(). */ > OVS_PREFETCH(md->cacheline1); > > - /* Prefetch cachline2 as ip_dst & ipv6_dst fields will be initialized. */ > +} > +/* This function prefetches the cachelines touched by miniflow_extract().*/ > +static inline void > +pkt_metadata_prefetch(struct pkt_metadata *md) > +{ > + OVS_PREFETCH(md->cacheline0); > + OVS_PREFETCH(md->cacheline1); > OVS_PREFETCH(md->cacheline2); > } > > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev