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/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@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
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
