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

[Ben Pfaff]

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 at 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.