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

Samuel Ghinet sghinet at cloudbasesolutions.com
Fri Aug 8 13:26:32 UTC 2014


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/licenses/
>LICENSE-2.0&k=oIvRg1%2BdGAgOoM1BIlLLqw%3D%3D%0A&r=pEkjsHfytvHEWufeZPpgqSOJ
>MdMjuZPbesVsNhCUc0E%3D%0A&m=dGugXOr5PbckUoj%2FSkMla30eF3upwFHL51toYo5nVBw%
>3D%0A&s=582a4218a2a644f12c19b27e6f3f4dcd609a90e8752bdfd22aa62b66769847c9
>
>- *
>
>- * 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.microsoft
>.com/developer/msbuild/2003&k=oIvRg1%2BdGAgOoM1BIlLLqw%3D%3D%0A&r=pEkjsHfy
>tvHEWufeZPpgqSOJMdMjuZPbesVsNhCUc0E%3D%0A&m=dGugXOr5PbckUoj%2FSkMla30eF3up
>wFHL51toYo5nVBw%3D%0A&s=23bcbe802599560d0a2cd611617c8456f38ddd3c1df04d90ec
>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/mailman/
>listinfo/dev&k=oIvRg1%2BdGAgOoM1BIlLLqw%3D%3D%0A&r=pEkjsHfytvHEWufeZPpgqSO
>JMdMjuZPbesVsNhCUc0E%3D%0A&m=dGugXOr5PbckUoj%2FSkMla30eF3upwFHL51toYo5nVBw
>%3D%0A&s=3a166e50206456fa68e25c7562df2b6ddee1fe28f5af90a3679af263cf3aef85




More information about the dev mailing list