I am happy to rename these to OvsAtomicAdd64, OvsAtomicInc64, etc. The code
that was using the atomic adds probably got deleted and hence it has no
references anymore. It was certianly used at some point.
Without the casting, I get the following error (we treat warnings as error for
the kernel build):
Error:
1>OvsBufferMgmt.c(1264): error C2220: warning treated as error - no 'object'
file generated
1>OvsBufferMgmt.c(1264): warning C4057: 'function' : 'volatile LONG *' differs
in indirection to slightly different base types from 'UINT32 *'
Patch:
bash-3.1$ git diff
diff --git a/datapath-windows/ovsext/OvsBufferMgmt.c b/datapath-windows/ovsext/O
index 8aa8060..f83d071 100644
--- a/datapath-windows/ovsext/OvsBufferMgmt.c
+++ b/datapath-windows/ovsext/OvsBufferMgmt.c
@@ -1261,7 +1261,7 @@ OvsTcpSegmentNBL(PVOID ovsContext,
dstCtx->magic = OVS_CTX_MAGIC;
dstCtx->dataOffsetDelta = hdrSize + headRoom;
- InterlockedIncrement((LONG volatile *)&srcCtx->refCount);
+ InterlockedIncrement(&srcCtx->refCount);
#ifdef DBG
InterlockedIncrement((LONG volatile *)&ovsPool->fragNBLCount);
-- Saurabh
> -----Original Message-----
> From: Samuel Ghinet [mailto:sghi...@cloudbasesolutions.com]
> Sent: Friday, August 08, 2014 6:27 AM
> To: Saurabh Shah; dev@openvswitch.org
> Subject: RE: [ovs-dev] [PATCH 04/15] datapath-windows: We don't need
> wrappers for Interlocked ops
>
> Hi Saurabh,
>
> What I particularly don't like about these:
> a) "atomic_add64" is not that much shorter than "InterlockedAdd64". When
> used in code, I don't think you need to do much casting (to be actually worth
> using the casting from within "atomic_add64")
> b) Interlocked functions may not be called everywhere in the code. ATM, as I
> see, they are used nowhere in code.
> And I believe there is no point in having a separate file with two functions,
> if
> they would probably be used in the future in two or three places in code.
> c) I personally don't like mangling the linux-kernel code style with windows-
> style.
> d) I would personally prefer the windows names rather than linux names on
> windows. I.e. if windows calls it "interlocked", why call them using a
> synonym? Not necessarily for this case only, but for each code we write, it
> would be clearer to use the windows namings, for those that are familiar
> with windows / windows kernel API.
>
> Sam
> ________________________________________
> From: Saurabh Shah [ssaur...@vmware.com]
> Sent: Wednesday, August 06, 2014 10:12 PM
> To: Samuel Ghinet; dev@openvswitch.org
> Subject: Re: [ovs-dev] [PATCH 04/15] datapath-windows: We don't need
> wrappers for Interlocked ops
>
> Hi Samuel,
>
> I like the wrapper because it keeps the code looking tidy. With the long
> names & casting the code line typically ends up way to long & unwieldy.
>
> InterlockedAdd64((LONGLONG volatile *)
> BasePointer->ChildObject->SomeStatVariable, (LONGLONG) val);
>
> Vs
> atomic_add64(BasePointer->ChildObject->SomeStatVariable, 10);
>
> Some other benefits that it brings:
> - It is more natural to think of the operation as Œatomic add¹ rather than
> Œinterlocked add¹.
> - may provide a way to benchmark perf impact of doing atomics (especially
> for stats).
> - Microsoft API¹s probably remains backward compatible, but if they
> improve the atomics & come up with new API¹s. We could easily switch over
> to the new API without touching a lot of existing code.
>
> I don¹t see a downside to using these wrapper, could you elaborate why you
> don¹t like it?
>
>
> >We don't need wrappers for Interlocked ops.
> >
> >
> >
> >Atomic.h contain simple wrappers for windows kernel API interlocked
> >functions.
> >
> >
> >
> >There is no reason to use the wrappers instead of the Kernel API
> >functions directly.
> >
> >
> >
> >Signed-off-by: Samuel Ghinet <sghi...@cloudbasesolutions.com>
> >
> >---
> >
> > datapath-windows/ovsext/Core/Atomic.h | 32
> >--------------------------
> >
> > datapath-windows/ovsext/ovsext.vcxproj | 1 -
> >
> > datapath-windows/ovsext/ovsext.vcxproj.filters | 3 ---
> >
> > 3 files changed, 36 deletions(-)
> >
> > delete mode 100644 datapath-windows/ovsext/Core/Atomic.h
> >
> >
> >
> >diff --git a/datapath-windows/ovsext/Core/Atomic.h
> >b/datapath-windows/ovsext/Core/Atomic.h
> >
> >deleted file mode 100644
> >
> >index a94d1fb..0000000
> >
> >--- a/datapath-windows/ovsext/Core/Atomic.h
> >
> >+++ /dev/null
> >
> >@@ -1,32 +0,0 @@
> >
> >-/*
> >
> >- * Copyright (c) 2014 VMware, Inc.
> >
> >- *
> >
> >- * Licensed under the Apache License, Version 2.0 (the "License");
> >
> >- * you may not use this file except in compliance with the License.
> >
> >- * You may obtain a copy of the License at:
> >
> >- *
> >
> >- *
> >https://urldefense.proofpoint.com/v1/url?u=http://www.apache.org/licen
> s
> >es/
> >LICENSE-
> 2.0&k=oIvRg1%2BdGAgOoM1BIlLLqw%3D%3D%0A&r=pEkjsHfytvHEWufeZPp
> gq
> >SOJ
> >MdMjuZPbesVsNhCUc0E%3D%0A&m=dGugXOr5PbckUoj%2FSkMla30eF3u
> pwFHL51toYo5nV
> >Bw%
> >3D%0A&s=582a4218a2a644f12c19b27e6f3f4dcd609a90e8752bdfd22aa62b66
> 769847c
> >9
> >
> >- *
> >
> >- * Unless required by applicable law or agreed to in writing, software
> >
> >- * distributed under the License is distributed on an "AS IS" BASIS,
> >
> >- * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
> >implied.
> >
> >- * See the License for the specific language governing permissions and
> >
> >- * limitations under the License.
> >
> >- */
> >
> >-
> >
> >-#ifndef __OVS_ATOMIC_H_
> >
> >-#define __OVS_ATOMIC_H_ 1
> >
> >-
> >
> >-static __inline UINT64
> >
> >-atomic_add64(UINT64 *ptr, UINT32 val)
> >
> >-{
> >
> >- return InterlockedAdd64((LONGLONG volatile *) ptr, (LONGLONG) val);
> >
> >-}
> >
> >-
> >
> >-static __inline UINT64
> >
> >-atomic_inc64(UINT64 *ptr)
> >
> >-{
> >
> >- return InterlockedIncrement64((LONGLONG volatile *) ptr);
> >
> >-}
> >
> >-
> >
> >-#endif /* __OVS_ATOMIC_H_ */
> >
> >diff --git a/datapath-windows/ovsext/ovsext.vcxproj
> >b/datapath-windows/ovsext/ovsext.vcxproj
> >
> >index 9c11a34..e955a73 100644
> >
> >--- a/datapath-windows/ovsext/ovsext.vcxproj
> >
> >+++ b/datapath-windows/ovsext/ovsext.vcxproj
> >
> >@@ -70,7 +70,6 @@
> >
> > <Import
> >Project="$(UserRootDir)\Microsoft.Cpp.$(Platform).user.props"
> >Condition="exists('$(UserRootDir)\Microsoft.Cpp.$(Platform).user.props')"
> >/>
> >
> > </ImportGroup>
> >
> > <ItemGroup Label="WrappedTaskItems">
> >
> >- <ClInclude Include="Core\Atomic.h" />
> >
> > <ClInclude Include="Core\Debug.h" />
> >
> > <ClInclude Include="Core\IpHelper.h" />
> >
> > <ClInclude Include="Core\Jhash.h" />
> >
> >diff --git a/datapath-windows/ovsext/ovsext.vcxproj.filters
> >b/datapath-windows/ovsext/ovsext.vcxproj.filters
> >
> >index d2fdf5f..24bcfbe 100644
> >
> >--- a/datapath-windows/ovsext/ovsext.vcxproj.filters
> >
> >+++ b/datapath-windows/ovsext/ovsext.vcxproj.filters
> >
> >@@ -1,9 +1,6 @@
> >
> > <?xml version="1.0" encoding="utf-8"?>
> >
> > <Project ToolsVersion="12.0"
> >xmlns="https://urldefense.proofpoint.com/v1/url?u=http://schemas.micr
> os
> >oft
> >.com/developer/msbuild/2003&k=oIvRg1%2BdGAgOoM1BIlLLqw%3D%3D%
> 0A&r=pEkjs
> >Hfy
> >tvHEWufeZPpgqSOJMdMjuZPbesVsNhCUc0E%3D%0A&m=dGugXOr5PbckU
> oj%2FSkMla30eF
> >3up
> >wFHL51toYo5nVBw%3D%0A&s=23bcbe802599560d0a2cd611617c8456f38dd
> d3c1df04d9
> >0ec
> >dc0e05e85315bb">
> >
> > <ItemGroup>
> >
> >- <ClInclude Include="Core\Atomic.h">
> >
> >- <Filter>Core</Filter>
> >
> >- </ClInclude>
> >
> > <ClInclude Include="Core\Debug.h">
> >
> > <Filter>Core</Filter>
> >
> > </ClInclude>
> >
> >--
> >
> >1.8.3.msysgit.0
> >
> >
> >
> >
> >
> >_______________________________________________
> >dev mailing list
> >dev@openvswitch.org
> >https://urldefense.proofpoint.com/v1/url?u=http://openvswitch.org/mail
> m
> >an/
> >listinfo/dev&k=oIvRg1%2BdGAgOoM1BIlLLqw%3D%3D%0A&r=pEkjsHfytvH
> EWufeZPpg
> >qSO
> >JMdMjuZPbesVsNhCUc0E%3D%0A&m=dGugXOr5PbckUoj%2FSkMla30eF3
> upwFHL51toYo5n
> >VBw
> >%3D%0A&s=3a166e50206456fa68e25c7562df2b6ddee1fe28f5af90a3679af26
> 3cf3aef
> >85
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
