Page MenuHomeFreeBSD

linuxkpi: Add time_after32
ClosedPublic

Authored by manu on Jul 17 2020, 8:02 AM.

Details

Summary

This compare two 32 bits times

Sponsored-by: The FreeBSD Foundation

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

manu requested review of this revision.Jul 17 2020, 8:02 AM
sys/compat/linuxkpi/common/include/linux/jiffies.h
48 ↗(On Diff #74562)

Define time_after32() like time_after() . int is always 32-bit on our platforms. Else a lot of things will break.

sys/compat/linuxkpi/common/include/linux/jiffies.h
48 ↗(On Diff #74562)

Linux cast the operand to uint32_t, I guess some code can rely on that.

sys/compat/linuxkpi/common/include/linux/jiffies.h
48 ↗(On Diff #74562)

I don't see how casting the arguments makes any difference in C- ? Can you investigate?

Adding Konstantin here.

Casts do make huge difference. Proposed code matches what Linux does.

Btw why not import time_before32() and time_between{,32}() as well ?

This revision is now accepted and ready to land.Jul 22 2020, 10:29 AM
In D25700#570408, @kib wrote:

Casts do make huge difference. Proposed code matches what Linux does.

Btw why not import time_before32() and time_between{,32}() as well ?

I simply import what the new drm code uses, I'll see to import them as well tomorow or friday.
Thanks

Konstantin: Please explain why:

(int)((unsigned)(x) - (unsigned)(y)) is different from
(int)((x) - (y))

I don't get it.

I mean the outer cast should be enough! Else many more macros in this file should be fixed.

Konstantin: Please explain why:

(int)((unsigned)(x) - (unsigned)(y)) is different from
(int)((x) - (y))

I don't get it.

If both x and y have type unsigned or unsigned types with rank less than unsigned, then indeed there is no change. Otherwise, the proposed code behavior depends on the relative rank of x, y, and uint32_t, and x,y signess.

Manu' code does either unsigned promotion or clipping on each argument, then it performs arithmetic mod UINT32_MAX, then reinterprets the result as signed.

Your code promotes one of the argument to integer type of rank of other argument, depending on what type is wider. If one type is unsigned, then other value is promoted unsigned, and again arithmetic mod size is performed.

But if both types are signed, then arithmetic is done as signed (*) and this potentially invokes undefined behavior if the result cannot be represented as signed.

  • - I believe that this is slightly incorrect but lets ignore some corner case.

Konstantin, doesn't the use of subtraction "-" imply that the result is signed in C?

If clipping must be predictable by use of a (unsigned) case when compiling, then more macros in this file needs an update! Doesn't freebsd specify -fwrapv, to avoid these issues?

Konstantin, doesn't the use of subtraction "-" imply that the result is signed in C?

No, the result has the promoted type of left and right operands. Basically the shorter type is promoted to wider one. But there are additional rules when operands have different signess.

If clipping must be predictable by use of a (unsigned) case when compiling, then more macros in this file needs an update! Doesn't freebsd specify -fwrapv, to avoid these issues?

Well, clipping is orthogonal to overflow. Idea with -fwrapv is to fix already broken code, but try to not introduce new broken places.

With all that corner cases and hard-to-correctly-interpret semantic, I do not see why it is useful to deviate from Linux original.

With all that corner cases and hard-to-correctly-interpret semantic, I do not see why it is useful to deviate from Linux original.

Instead of casting, would a static inline function be better, and more clean code-wise?

With all that corner cases and hard-to-correctly-interpret semantic, I do not see why it is useful to deviate from Linux original.

Instead of casting, would a static inline function be better, and more clean code-wise?

I do not see much difference between the macro and an inline function for such small code fragment. My caution with the function would be that it changes evaluation of arguments, in particular, the side-effects, comparing with the Linux version.

What do you think about using "int" instead of "int32" ?

@manu: What do you think about implementing the set of time_xxx functions we have as time_xxx_32() functions?

Even though you don't need them right now, we will have a consistent set of macros there?

What do you think about using "int" instead of "int32" ?

This is a question about required semantic of the Linux function. If (since ?) they require int32_t result, we should be explicit and not rely on the fact that all our current platforms have int32_t same as int.

@manu: What do you think about implementing the set of time_xxx functions we have as time_xxx_32() functions?

Even though you don't need them right now, we will have a consistent set of macros there?

I'll have a look at the list.

In D25700#571075, @kib wrote:

What do you think about using "int" instead of "int32" ?

This is a question about required semantic of the Linux function. If (since ?) they require int32_t result, we should be explicit and not rely on the fact that all our current platforms have int32_t same as int.

Agreed, we don't need the int32_t cast but it's clearer to leave it to show what linux expect of the macro.

This revision now requires review to proceed.Aug 4 2020, 10:47 AM
manu marked 2 inline comments as done.Aug 4 2020, 3:12 PM

Add the word LinuxKPI to the commit message and don't forget to MFC.

This revision is now accepted and ready to land.Aug 4 2020, 3:18 PM
This revision was automatically updated to reflect the committed changes.