[ovs-dev] [PATCH v1 2/6] dpif-netdev: add tunnel_valid flag to skip ip/ipv6 address comparison
Ilya Maximets
i.maximets at ovn.org
Sun Jul 5 14:11:34 UTC 2020
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.Zhang at arm.com>
> Reviewed-by: Malvika Gupta <Malvika.Gupta at arm.com>
> Signed-off-by: Yanqin Wei <Yanqin.Wei at 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);
> }
>
>
More information about the dev
mailing list