Re: [ovs-dev] [PATCH 18/20] datapath: use ktime_get_ts64() instead of ktime_get_ts()

[Arnd Bergmann]

On Fri, Feb 2, 2018 at 10:27 PM, Gregory Rose  wrote:
> On 2/2/2018 12:13 PM, Arnd Bergmann wrote:
>>
>> On Fri, Feb 2, 2018 at 7:31 PM, Gregory Rose  wrote:
>
> That's an interesting suggestion Arnd.  I'm generally opposed to divide
> operations when I can avoid
> them and ktime_get_ts64() avoids that SFAICT.  We do still support 32 bit
> systems as well so I think I'll
> just go ahead and stick with ktime_get_ts64.

Ok, makes sense. Just for completeness: Yes, ktime_get_ts64() avoids the
64-bit division, though you still get a 32-bit division when converting it to
milliseconds here.

On almost all architectures, we should be using the optimized do_div()
implementation these days, which will replace a 64-bit division with a constant
divisor with a set of multiplications and shifts, which tends do be much
cheaper than the full 64-bit division, so doing a ktime_to_msec() is
usually not too bad either.

   Arnd
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 18/20] datapath: use ktime_get_ts64() instead of ktime_get_ts()

[Gregory Rose]

On 2/2/2018 12:13 PM, Arnd Bergmann wrote:

On Fri, Feb 2, 2018 at 7:31 PM, Gregory Rose  wrote:

On 2/2/2018 10:18 AM, Pravin Shelar wrote:

On Tue, Jan 30, 2018 at 4:40 PM, Gregory Rose 

This is done in compat code, can you move it to respective header file?


Yes - my own preference is to keep these sorts of things close to where
they're used but
I suppose there is a good chance we'll use ktime_get_ts64 elsewhere in the
future.  So
that's fine by me.

You could decide to use ktime_get_ns() divided by NSEC_PER_MSEC
instead of getting that number from ktime_get_ts64(), that would be
more portable, though a little bit slower on 32-bit architectures.

Arnd


That's an interesting suggestion Arnd.  I'm generally opposed to divide 
operations when I can avoid
them and ktime_get_ts64() avoids that SFAICT.  We do still support 32 
bit systems as well so I think I'll
just go ahead and stick with ktime_get_ts64.  Also, sometimes I have to 
compare our OOT datapath
kernel code with upstream and when I stick to the same coding as much as 
possible it helps me out.


I'll move it over into our compat layer code as suggested by Pravin but 
thanks for providing helpful

suggestion.

Regards,

- Greg
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 18/20] datapath: use ktime_get_ts64() instead of ktime_get_ts()

[Arnd Bergmann]

On Fri, Feb 2, 2018 at 7:31 PM, Gregory Rose  wrote:
> On 2/2/2018 10:18 AM, Pravin Shelar wrote:
>>
>> On Tue, Jan 30, 2018 at 4:40 PM, Gregory Rose 
>>
>> This is done in compat code, can you move it to respective header file?
>
>
> Yes - my own preference is to keep these sorts of things close to where
> they're used but
> I suppose there is a good chance we'll use ktime_get_ts64 elsewhere in the
> future.  So
> that's fine by me.

You could decide to use ktime_get_ns() divided by NSEC_PER_MSEC
instead of getting that number from ktime_get_ts64(), that would be
more portable, though a little bit slower on 32-bit architectures.

   Arnd
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 18/20] datapath: use ktime_get_ts64() instead of ktime_get_ts()

[Gregory Rose]

On 2/2/2018 10:18 AM, Pravin Shelar wrote:

On Tue, Jan 30, 2018 at 4:40 PM, Gregory Rose  wrote:

On 1/30/2018 3:40 PM, Greg Rose wrote:

From: Arnd Bergmann 

Upstream commit:
  commit 311af51dcb5629f04976a8e451673f77e3301041
  Author: Arnd Bergmann 
  Date:   Mon Nov 27 12:41:38 2017 +0100

  openvswitch: use ktime_get_ts64() instead of ktime_get_ts()

  timespec is deprecated because of the y2038 overflow, so let's
convert
  this one to ktime_get_ts64(). The code is already safe even on 32-bit
  architectures, since it uses monotonic times. On 64-bit
architectures,
  nothing changes, while on 32-bit architectures this avoids one
  type conversion.

  Signed-off-by: Arnd Bergmann 
  Signed-off-by: David S. Miller 

Additional compatability check for ktime_get_ts64() exists or not.
If not, then just continue using ktime_get_ts().

Cc: Arnd Bergmann 
Signed-off-by: Greg Rose 


Oops, I screwed this up.  ktime_get_ts64 isn't a macro.  We'll need this
incremental...

diff --git a/acinclude.m4 b/acinclude.m4
index bc1ec72..5c63222 100644
--- a/acinclude.m4
+++ b/acinclude.m4
@@ -807,6 +807,9 @@ AC_DEFUN([OVS_CHECK_LINUX_COMPAT], [
OVS_GREP_IFELSE([$KSRC/include/linux/timekeeping.h],
[ktime_get_ns],
[OVS_DEFINE([HAVE_KTIME_GET_NS])])
+  OVS_GREP_IFELSE([$KSRC/include/linux/timekeeping.h],
+  [ktime_get_ts64],
+  [OVS_DEFINE([HAVE_KTIME_GET_TS64])])

if cmp -s datapath/linux/kcompat.h.new \
  datapath/linux/kcompat.h >/dev/null 2>&1; then
diff --git a/datapath/flow.c b/datapath/flow.c
index 385e481..cd8d422 100644
--- a/datapath/flow.c
+++ b/datapath/flow.c
@@ -52,7 +52,7 @@
  #include "flow_netlink.h"
  #include "vport.h"

-#ifndef ktime_get_ts64
+#ifndef HAVE_KTIME_GET_TS64
  #define ktime_get_ts64 ktime_get_ts
  #define timespec64 timespec
  #endif




---
   datapath/flow.c | 11 ---
   1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/datapath/flow.c b/datapath/flow.c
index 5da7e3e..385e481 100644
--- a/datapath/flow.c
+++ b/datapath/flow.c
@@ -52,14 +52,19 @@
   #include "flow_netlink.h"
   #include "vport.h"
   +#ifndef ktime_get_ts64
+#define ktime_get_ts64 ktime_get_ts
+#define timespec64 timespec
+#endif
+

This is done in compat code, can you move it to respective header file?


Yes - my own preference is to keep these sorts of things close to where 
they're used but
I suppose there is a good chance we'll use ktime_get_ts64 elsewhere in 
the future.  So

that's fine by me.

Thanks,

- Greg


   u64 ovs_flow_used_time(unsigned long flow_jiffies)
   {
-   struct timespec cur_ts;
+   struct timespec64 cur_ts;
 u64 cur_ms, idle_ms;
   - ktime_get_ts(_ts);
+   ktime_get_ts64(_ts);
 idle_ms = jiffies_to_msecs(jiffies - flow_jiffies);
-   cur_ms = (u64)cur_ts.tv_sec * MSEC_PER_SEC +
+   cur_ms = (u64)(u32)cur_ts.tv_sec * MSEC_PER_SEC +
  cur_ts.tv_nsec / NSEC_PER_MSEC;
 return cur_ms - idle_ms;


___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev


___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 18/20] datapath: use ktime_get_ts64() instead of ktime_get_ts()

[Pravin Shelar]

On Tue, Jan 30, 2018 at 4:40 PM, Gregory Rose  wrote:
> On 1/30/2018 3:40 PM, Greg Rose wrote:
>>
>> From: Arnd Bergmann 
>>
>> Upstream commit:
>>  commit 311af51dcb5629f04976a8e451673f77e3301041
>>  Author: Arnd Bergmann 
>>  Date:   Mon Nov 27 12:41:38 2017 +0100
>>
>>  openvswitch: use ktime_get_ts64() instead of ktime_get_ts()
>>
>>  timespec is deprecated because of the y2038 overflow, so let's
>> convert
>>  this one to ktime_get_ts64(). The code is already safe even on 32-bit
>>  architectures, since it uses monotonic times. On 64-bit
>> architectures,
>>  nothing changes, while on 32-bit architectures this avoids one
>>  type conversion.
>>
>>  Signed-off-by: Arnd Bergmann 
>>  Signed-off-by: David S. Miller 
>>
>> Additional compatability check for ktime_get_ts64() exists or not.
>> If not, then just continue using ktime_get_ts().
>>
>> Cc: Arnd Bergmann 
>> Signed-off-by: Greg Rose 
>
>
> Oops, I screwed this up.  ktime_get_ts64 isn't a macro.  We'll need this
> incremental...
>
> diff --git a/acinclude.m4 b/acinclude.m4
> index bc1ec72..5c63222 100644
> --- a/acinclude.m4
> +++ b/acinclude.m4
> @@ -807,6 +807,9 @@ AC_DEFUN([OVS_CHECK_LINUX_COMPAT], [
>OVS_GREP_IFELSE([$KSRC/include/linux/timekeeping.h],
>[ktime_get_ns],
>[OVS_DEFINE([HAVE_KTIME_GET_NS])])
> +  OVS_GREP_IFELSE([$KSRC/include/linux/timekeeping.h],
> +  [ktime_get_ts64],
> +  [OVS_DEFINE([HAVE_KTIME_GET_TS64])])
>
>if cmp -s datapath/linux/kcompat.h.new \
>  datapath/linux/kcompat.h >/dev/null 2>&1; then
> diff --git a/datapath/flow.c b/datapath/flow.c
> index 385e481..cd8d422 100644
> --- a/datapath/flow.c
> +++ b/datapath/flow.c
> @@ -52,7 +52,7 @@
>  #include "flow_netlink.h"
>  #include "vport.h"
>
> -#ifndef ktime_get_ts64
> +#ifndef HAVE_KTIME_GET_TS64
>  #define ktime_get_ts64 ktime_get_ts
>  #define timespec64 timespec
>  #endif
>
>
>
>> ---
>>   datapath/flow.c | 11 ---
>>   1 file changed, 8 insertions(+), 3 deletions(-)
>>
>> diff --git a/datapath/flow.c b/datapath/flow.c
>> index 5da7e3e..385e481 100644
>> --- a/datapath/flow.c
>> +++ b/datapath/flow.c
>> @@ -52,14 +52,19 @@
>>   #include "flow_netlink.h"
>>   #include "vport.h"
>>   +#ifndef ktime_get_ts64
>> +#define ktime_get_ts64 ktime_get_ts
>> +#define timespec64 timespec
>> +#endif
>> +

This is done in compat code, can you move it to respective header file?

>>   u64 ovs_flow_used_time(unsigned long flow_jiffies)
>>   {
>> -   struct timespec cur_ts;
>> +   struct timespec64 cur_ts;
>> u64 cur_ms, idle_ms;
>>   - ktime_get_ts(_ts);
>> +   ktime_get_ts64(_ts);
>> idle_ms = jiffies_to_msecs(jiffies - flow_jiffies);
>> -   cur_ms = (u64)cur_ts.tv_sec * MSEC_PER_SEC +
>> +   cur_ms = (u64)(u32)cur_ts.tv_sec * MSEC_PER_SEC +
>>  cur_ts.tv_nsec / NSEC_PER_MSEC;
>> return cur_ms - idle_ms;
>
>
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 18/20] datapath: use ktime_get_ts64() instead of ktime_get_ts()

[Gregory Rose]

On 1/30/2018 3:40 PM, Greg Rose wrote:

From: Arnd Bergmann 

Upstream commit:
 commit 311af51dcb5629f04976a8e451673f77e3301041
 Author: Arnd Bergmann 
 Date:   Mon Nov 27 12:41:38 2017 +0100

 openvswitch: use ktime_get_ts64() instead of ktime_get_ts()

 timespec is deprecated because of the y2038 overflow, so let's convert
 this one to ktime_get_ts64(). The code is already safe even on 32-bit
 architectures, since it uses monotonic times. On 64-bit architectures,
 nothing changes, while on 32-bit architectures this avoids one
 type conversion.

 Signed-off-by: Arnd Bergmann 
 Signed-off-by: David S. Miller 

Additional compatability check for ktime_get_ts64() exists or not.
If not, then just continue using ktime_get_ts().

Cc: Arnd Bergmann 
Signed-off-by: Greg Rose 


Oops, I screwed this up.  ktime_get_ts64 isn't a macro.  We'll need this 
incremental...


diff --git a/acinclude.m4 b/acinclude.m4
index bc1ec72..5c63222 100644
--- a/acinclude.m4
+++ b/acinclude.m4
@@ -807,6 +807,9 @@ AC_DEFUN([OVS_CHECK_LINUX_COMPAT], [
   OVS_GREP_IFELSE([$KSRC/include/linux/timekeeping.h],
   [ktime_get_ns],
   [OVS_DEFINE([HAVE_KTIME_GET_NS])])
+  OVS_GREP_IFELSE([$KSRC/include/linux/timekeeping.h],
+  [ktime_get_ts64],
+  [OVS_DEFINE([HAVE_KTIME_GET_TS64])])

   if cmp -s datapath/linux/kcompat.h.new \
 datapath/linux/kcompat.h >/dev/null 2>&1; then
diff --git a/datapath/flow.c b/datapath/flow.c
index 385e481..cd8d422 100644
--- a/datapath/flow.c
+++ b/datapath/flow.c
@@ -52,7 +52,7 @@
 #include "flow_netlink.h"
 #include "vport.h"

-#ifndef ktime_get_ts64
+#ifndef HAVE_KTIME_GET_TS64
 #define ktime_get_ts64 ktime_get_ts
 #define timespec64 timespec
 #endif



---
  datapath/flow.c | 11 ---
  1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/datapath/flow.c b/datapath/flow.c
index 5da7e3e..385e481 100644
--- a/datapath/flow.c
+++ b/datapath/flow.c
@@ -52,14 +52,19 @@
  #include "flow_netlink.h"
  #include "vport.h"
  
+#ifndef ktime_get_ts64

+#define ktime_get_ts64 ktime_get_ts
+#define timespec64 timespec
+#endif
+
  u64 ovs_flow_used_time(unsigned long flow_jiffies)
  {
-   struct timespec cur_ts;
+   struct timespec64 cur_ts;
u64 cur_ms, idle_ms;
  
-	ktime_get_ts(_ts);

+   ktime_get_ts64(_ts);
idle_ms = jiffies_to_msecs(jiffies - flow_jiffies);
-   cur_ms = (u64)cur_ts.tv_sec * MSEC_PER_SEC +
+   cur_ms = (u64)(u32)cur_ts.tv_sec * MSEC_PER_SEC +
 cur_ts.tv_nsec / NSEC_PER_MSEC;
  
  	return cur_ms - idle_ms;


___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev