| Message ID | 1498165849-19391-1-git-send-email-bhanuprakash.bodireddy@intel.com |
|---|---|
| State | Superseded |
| Headers | show |
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@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@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
On Thu, Jun 22, 2017 at 10:10:49PM +0100, Bhanuprakash Bodireddy 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@intel.com> OVS has a PADDED_MEMBERS macro that makes this easier. Unfortunately it isn't currently adequate for use more than once per struct. But it's fixable, so I sent a patch to fix it: https://mail.openvswitch.org/pipermail/ovs-dev/2017-July/335204.html and a fixed-up version of the original patch: https://mail.openvswitch.org/pipermail/ovs-dev/2017-July/335205.html However, even with the fix, this is going to cause problems with MSVC, because it does not allow 0-length arrays. Maybe you can find another way to mark the beginning of a cache line.
On Sun, Jul 09, 2017 at 10:53:52PM +0000, Darrell Ball wrote: > 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 a lot, I did have some comments, so I followed up directly to the patch.
Hi Ben, >On Thu, Jun 22, 2017 at 10:10:49PM +0100, Bhanuprakash Bodireddy 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@intel.com> > >OVS has a PADDED_MEMBERS macro that makes this easier. Unfortunately it >isn't currently adequate for use more than once per struct. But it's fixable, so I >sent a patch to fix it: > https://mail.openvswitch.org/pipermail/ovs-dev/2017-July/335204.html Thanks for this patch. PADDED_MEMBERS macro is pretty handy. >and a fixed-up version of the original patch: > https://mail.openvswitch.org/pipermail/ovs-dev/2017-July/335205.html Thanks for improving the patch. > >However, even with the fix, this is going to cause problems with MSVC, >because it does not allow 0-length arrays. Maybe you can find another way to >mark the beginning of a cache line. Microsoft links mentions that the warning "zero-sized array in struct/union" we encountered is a 'Level-4' warning and is numbered 'C4200'. C4200: https://msdn.microsoft.com/en-us/library/79wf64bc.aspx I can't think of alternate ways to mark the cachelines, so how about this incremental change in lib/util.h that disables the warning when building with MSVC? I am currently on vacation and have limited access to lab to download and test the changes with Microsoft compiliers. Apologies for this. --8<--------------------------cut here-------------------------->8-- #define CACHE_LINE_SIZE 64 BUILD_ASSERT_DECL(IS_POW2(CACHE_LINE_SIZE)); #ifndef _MSC_VER typedef void *OVS_CACHE_LINE_MARKER[0]; #else __pragma (warning(push)) __pragma (warning(disable:4200)) typedef void *OVS_CACHE_LINE_MARKER[0]; __pragma (warning(pop)) #endif - Bhanuprakash.
>On Sun, Jul 09, 2017 at 10:53:52PM +0000, Darrell Ball wrote: >> I went thru. this patch and see the merits of the objective. >> I also did various testing with it. >> I had one comment inline. Thanks Darrell for testing this patch. I will factor in your comment in next version of patch. >> >> However, I would feel more comfortable if Ben possibly could take a look as >well. > >Thanks a lot, I did have some comments, so I followed up directly to the patch. I sent out a possible fix in other mail by disabling the C4200 warnings. I have limited knowledge in compilers and unfortunately don't have MSVC installed to test my incremental changes. Please review it and let me know if it fixes the issue. Note: Typo in subject, pkt_metdata should be changed to 'pkt_metadata'. - Bhanuprakash.
On Wed, Jul 12, 2017 at 10:38:30AM +0000, Bodireddy, Bhanuprakash wrote: > Hi Ben, > > >On Thu, Jun 22, 2017 at 10:10:49PM +0100, Bhanuprakash Bodireddy 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@intel.com> > > > >OVS has a PADDED_MEMBERS macro that makes this easier. Unfortunately it > >isn't currently adequate for use more than once per struct. But it's fixable, so I > >sent a patch to fix it: > > https://mail.openvswitch.org/pipermail/ovs-dev/2017-July/335204.html > > Thanks for this patch. PADDED_MEMBERS macro is pretty handy. > > >and a fixed-up version of the original patch: > > https://mail.openvswitch.org/pipermail/ovs-dev/2017-July/335205.html > > Thanks for improving the patch. > > > > >However, even with the fix, this is going to cause problems with MSVC, > >because it does not allow 0-length arrays. Maybe you can find another way to > >mark the beginning of a cache line. > > Microsoft links mentions that the warning "zero-sized array in struct/union" we encountered > is a 'Level-4' warning and is numbered 'C4200'. > > C4200: https://msdn.microsoft.com/en-us/library/79wf64bc.aspx > > I can't think of alternate ways to mark the cachelines, so how about this incremental change in lib/util.h > that disables the warning when building with MSVC? > > I am currently on vacation and have limited access to lab to download and test the changes with Microsoft compiliers. > Apologies for this. > > --8<--------------------------cut here-------------------------->8-- > > #define CACHE_LINE_SIZE 64 > BUILD_ASSERT_DECL(IS_POW2(CACHE_LINE_SIZE)); > #ifndef _MSC_VER > typedef void *OVS_CACHE_LINE_MARKER[0]; > #else > __pragma (warning(push)) > __pragma (warning(disable:4200)) > typedef void *OVS_CACHE_LINE_MARKER[0]; > __pragma (warning(pop)) > #endif > > - Bhanuprakash. I doubt that this is about a warning, because as I understand it OVS on MSVC causes a lot of warnings, so it's probably a more serious issue.
> >On Wed, Jul 12, 2017 at 10:38:30AM +0000, Bodireddy, Bhanuprakash wrote: >> Hi Ben, >> >> >On Thu, Jun 22, 2017 at 10:10:49PM +0100, Bhanuprakash Bodireddy 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@intel.com> >> > >> >OVS has a PADDED_MEMBERS macro that makes this easier. >Unfortunately >> >it isn't currently adequate for use more than once per struct. But >> >it's fixable, so I sent a patch to fix it: >> > >> >https://mail.openvswitch.org/pipermail/ovs-dev/2017-July/335204.html >> >> Thanks for this patch. PADDED_MEMBERS macro is pretty handy. >> >> >and a fixed-up version of the original patch: >> > >> >https://mail.openvswitch.org/pipermail/ovs-dev/2017-July/335205.html >> >> Thanks for improving the patch. >> >> > >> >However, even with the fix, this is going to cause problems with >> >MSVC, because it does not allow 0-length arrays. Maybe you can find >> >another way to mark the beginning of a cache line. >> >> Microsoft links mentions that the warning "zero-sized array in >> struct/union" we encountered is a 'Level-4' warning and is numbered >'C4200'. >> >> C4200: https://msdn.microsoft.com/en-us/library/79wf64bc.aspx >> >> I can't think of alternate ways to mark the cachelines, so how about >> this incremental change in lib/util.h that disables the warning when building >with MSVC? >> >> I am currently on vacation and have limited access to lab to download and >test the changes with Microsoft compiliers. >> Apologies for this. >> >> --8<--------------------------cut here-------------------------->8-- >> >> #define CACHE_LINE_SIZE 64 >> BUILD_ASSERT_DECL(IS_POW2(CACHE_LINE_SIZE)); >> #ifndef _MSC_VER >> typedef void *OVS_CACHE_LINE_MARKER[0]; #else __pragma >(warning(push)) >> __pragma (warning(disable:4200)) typedef void >> *OVS_CACHE_LINE_MARKER[0]; __pragma (warning(pop)) #endif >> >> - Bhanuprakash. > >I doubt that this is about a warning, because as I understand it OVS on MSVC >causes a lot of warnings, so it's probably a more serious issue. In will try to get MSVC installed and verify the OvS build. Will get back to you on this. - Bhanuprakash.
On Thu, Jul 13, 2017 at 02:10:24AM +0000, Bodireddy, Bhanuprakash wrote: > > > >On Wed, Jul 12, 2017 at 10:38:30AM +0000, Bodireddy, Bhanuprakash wrote: > >> Hi Ben, > >> > >> >On Thu, Jun 22, 2017 at 10:10:49PM +0100, Bhanuprakash Bodireddy 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@intel.com> > >> > > >> >OVS has a PADDED_MEMBERS macro that makes this easier. > >Unfortunately > >> >it isn't currently adequate for use more than once per struct. But > >> >it's fixable, so I sent a patch to fix it: > >> > > >> >https://mail.openvswitch.org/pipermail/ovs-dev/2017-July/335204.html > >> > >> Thanks for this patch. PADDED_MEMBERS macro is pretty handy. > >> > >> >and a fixed-up version of the original patch: > >> > > >> >https://mail.openvswitch.org/pipermail/ovs-dev/2017-July/335205.html > >> > >> Thanks for improving the patch. > >> > >> > > >> >However, even with the fix, this is going to cause problems with > >> >MSVC, because it does not allow 0-length arrays. Maybe you can find > >> >another way to mark the beginning of a cache line. > >> > >> Microsoft links mentions that the warning "zero-sized array in > >> struct/union" we encountered is a 'Level-4' warning and is numbered > >'C4200'. > >> > >> C4200: https://msdn.microsoft.com/en-us/library/79wf64bc.aspx > >> > >> I can't think of alternate ways to mark the cachelines, so how about > >> this incremental change in lib/util.h that disables the warning when building > >with MSVC? > >> > >> I am currently on vacation and have limited access to lab to download and > >test the changes with Microsoft compiliers. > >> Apologies for this. > >> > >> --8<--------------------------cut here-------------------------->8-- > >> > >> #define CACHE_LINE_SIZE 64 > >> BUILD_ASSERT_DECL(IS_POW2(CACHE_LINE_SIZE)); > >> #ifndef _MSC_VER > >> typedef void *OVS_CACHE_LINE_MARKER[0]; #else __pragma > >(warning(push)) > >> __pragma (warning(disable:4200)) typedef void > >> *OVS_CACHE_LINE_MARKER[0]; __pragma (warning(pop)) #endif > >> > >> - Bhanuprakash. > > > >I doubt that this is about a warning, because as I understand it OVS on MSVC > >causes a lot of warnings, so it's probably a more serious issue. > > In will try to get MSVC installed and verify the OvS build. Will get back to you on this. If you push to a branch on github in your own repo, then appveyor will automatically do a build, so you don't have to install MSVC.
Hello Ben, >> > >> >I doubt that this is about a warning, because as I understand it OVS >> >on MSVC causes a lot of warnings, so it's probably a more serious issue. >> >> In will try to get MSVC installed and verify the OvS build. Will get back to you >on this. > >If you push to a branch on github in your own repo, then appveyor will >automatically do a build, so you don't have to install MSVC. Appveyor is very helpful to verify OvS build on MSVC. BTW I sent out 2 separate patches. Introduced new 'PADDED_MEMBERS_CACHELINE_MARKER' macro to mark cachelines. https://mail.openvswitch.org/pipermail/ovs-dev/2017-July/336186.html v3 patch using the new macro. https://mail.openvswitch.org/pipermail/ovs-dev/2017-July/336187.html With the above patches the build is successful on MSVC. I confirmed it using Appveyor. - Bhanuprakash.
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. */ + 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)
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@intel.com> --- lib/packets.h | 30 ++++++++++++++++++++++++++++-- lib/util.h | 1 + 2 files changed, 29 insertions(+), 2 deletions(-)