[ovs-dev] [PATCH 04/15] datapath-windows: We don't need wrappers for Interlocked ops

Samuel Ghinet sghinet at cloudbasesolutions.com
Wed Aug 13 13:46:05 UTC 2014


Perhaps a neater solution would be to simply have e.g. the OVS_BUFFER_CONTEXT's refCount declared as volatile and ULONG.
If the variable is meant to be used (read & written) using atomic operations, we could better declare it volatile.

Also, even though sizeof(ULONG) == sizeof(UINT32) and sizeof(int) == sizeof(long) on Visual C++, they are different types.

Sam
________________________________________
From: Saurabh Shah [ssaurabh at vmware.com]
Sent: Tuesday, August 12, 2014 1:50 AM
To: Samuel Ghinet
Cc: dev at openvswitch.org
Subject: RE: [ovs-dev] [PATCH 04/15] datapath-windows: We don't need wrappers for Interlocked ops

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:sghinet at cloudbasesolutions.com]
> Sent: Friday, August 08, 2014 6:27 AM
> To: Saurabh Shah; dev at 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 [ssaurabh at vmware.com]
> Sent: Wednesday, August 06, 2014 10:12 PM
> To: Samuel Ghinet; dev at 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 <sghinet at 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 at 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




More information about the dev mailing list