[ovs-dev] [PATCH] packets: Reorganize the pkt_metdata structure.

Darrell Ball dball at vmware.com
Sun Jul 9 22:53:52 UTC 2017


I went thru. this patch and see the merits of the objective.
I also did various testing with it.
I had one comment inline.

However, I would feel more comfortable if Ben possibly could take a look as well.

Thanks Darrell



On 6/22/17, 2:10 PM, "Bhanuprakash Bodireddy" <bhanuprakash.bodireddy at intel.com> wrote:

    pkt_metadata_init() is called for every packet in userspace datapath and
    initializes few members in pkt_metadata. Before this the members that
    needs to be initialized are prefetched using pkt_metadata_prefetch_init().
    
    The above functions are critical to the userspace datapath performance and
    should be in sync. Any changes to the pkt_metadata should also include
    changes to metadata_init() and prefetch_init() if necessary.
    
    This commit slightly refactors the pkt_metadata structure and introduces
    cache line markers to catch any violations to the structure. Also only
    prefetch the cachelines having the members that needs to be zeroed out.
    
    Signed-off-by: Bhanuprakash Bodireddy <bhanuprakash.bodireddy at intel.com>
    ---
     lib/packets.h | 30 ++++++++++++++++++++++++++++--
     lib/util.h    |  1 +
     2 files changed, 29 insertions(+), 2 deletions(-)
    
    diff --git a/lib/packets.h b/lib/packets.h
    index 94c3dcc..2c942aa 100644
    --- a/lib/packets.h
    +++ b/lib/packets.h
    @@ -92,6 +92,12 @@ flow_tnl_equal(const struct flow_tnl *a, const struct flow_tnl *b)
     
     /* Datapath packet metadata */
     struct pkt_metadata {
    +    OVS_CACHE_LINE_MARKER cacheline0;
    +#define CACHELINE0_PAD_SZ \
    +    PAD_SIZE(sizeof(bool) + 1 * sizeof(uint8_t) + 1 * sizeof(uint16_t) + \
    +             5 * sizeof(uint32_t) + sizeof (ovs_u128) +                  \
    +             sizeof(union flow_in_port), CACHE_LINE_SIZE)
    +
         uint32_t recirc_id;         /* Recirculation id carried with the
                                        recirculating packets. 0 for packets
                                        received from the wire. */
    @@ -104,15 +110,30 @@ struct pkt_metadata {
         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. */
    +    uint8_t  _pad_cacheline0[CACHELINE0_PAD_SZ];
    +
    +    OVS_CACHE_LINE_MARKER cacheline1;
    +#define CACHELINE1_PAD_SZ \
    +         PAD_SIZE(sizeof(struct ovs_key_ct_tuple_ipv6), CACHE_LINE_SIZE)
    +
         union {                     /* Populated only for non-zero 'ct_state'. */
             struct ovs_key_ct_tuple_ipv4 ipv4;
             struct ovs_key_ct_tuple_ipv6 ipv6;   /* Used only if                */
         } ct_orig_tuple;                         /* 'ct_orig_tuple_ipv6' is set */
    -    union flow_in_port in_port; /* Input port. */
    +    uint8_t  _pad_cacheline1[CACHELINE1_PAD_SZ];
    +
    +    OVS_CACHE_LINE_MARKER cacheline2;
    +
         struct flow_tnl tunnel;     /* Encapsulating tunnel parameters. Note that
                                      * if 'ip_dst' == 0, the rest of the fields may
                                      * be uninitialized. */
     };
    +BUILD_ASSERT_DECL(offsetof(struct pkt_metadata, cacheline0) == 0);
    +BUILD_ASSERT_DECL(offsetof(struct pkt_metadata, cacheline1) == \
    +                           CACHE_LINE_SIZE);
    +BUILD_ASSERT_DECL(offsetof(struct pkt_metadata, cacheline2) == \
    +                           2 * CACHE_LINE_SIZE);
     
     static inline void
     pkt_metadata_init_tnl(struct pkt_metadata *md)
    @@ -149,7 +170,12 @@ pkt_metadata_init(struct pkt_metadata *md, odp_port_t port)
     static inline void
     pkt_metadata_prefetch_init(struct pkt_metadata *md)
     {
    -    ovs_prefetch_range(md, offsetof(struct pkt_metadata, tunnel.ip_src));
    +    /* Prefetch cacheline0 as first 17 bytes and odp_port will be
    +     * initialized later.  */

I think it is more maintainable to mention just field names here rather than ’17 bytes’


    +    OVS_PREFETCH(md->cacheline0);
    +
    +    /* Prefetch cachline2 as ip_dst & ipv6_dst fields will be initialized. */
    +    OVS_PREFETCH(md->cacheline2);
     }
     
     bool dpid_from_string(const char *s, uint64_t *dpidp);
    diff --git a/lib/util.h b/lib/util.h
    index 4706c99..13eebb6 100644
    --- a/lib/util.h
    +++ b/lib/util.h
    @@ -50,6 +50,7 @@ extern char *program_name;
      * Being wrong hurts performance but not correctness. */
     #define CACHE_LINE_SIZE 64
     BUILD_ASSERT_DECL(IS_POW2(CACHE_LINE_SIZE));
    +typedef void *OVS_CACHE_LINE_MARKER[0];
     
     static inline void
     ovs_prefetch_range(const void *start, size_t size)
    -- 
    2.4.11
    
    



More information about the dev mailing list