linuxkpi: Fix up jiffies handling

[markj]

Summary

A few issues found by code inspection while hunting for bugzilla PR
286512:

• The "expires" field in struct delayed_work should be unsigned.
• In linux_timer_jiffies_until(), clamp the return value to INT_MAX: this return value is used as a ticks count, not a jiffies count, so we should avoid returning too large a value, lest it get truncated. It's unlikely we are dealing with values that large, but we should be careful anyway.
• In callers of linux_add_to_sleepqueue(), truncate the timeout to INT_MAX, as this value is passed to sleepq_set_timeout() as a ticks value. Typically it's multiplied by ticks_sbt to get an sbintime, and we should make sure the multiplication doesn't overflow. In drm-kmod, there is at least one call mod_delayed_work(... MAX_SCHEDULE_TIMEOUT).

Fixes: 325aa4dbd10d ("linuxkpi: Introduce a properly typed jiffies")

[kib]

sys/compat/linuxkpi/common/src/linux_schedule.c

368

To be cleanest, I would add an 'int btimeout;' local and assign to it in the if staircase above, instead of doing the cast. There, and in linux_schedule_timeout(). In other words, do not use cast to clamp, we do the comparisons anyway.

[bz]

Does __wait_event_common need similar treatment?

[markj]

Deduplicate jiffies->ticks conversions, use a local variable instead of casting.

[markj]

In D50192#1145248, @bz wrote:

Does __wait_event_common need similar treatment?

Not as far as I can see. It uses __wait_event_common(), which takes a jiffies timeout and handles conversion to ticks.

[kib]

sys/compat/linuxkpi/common/src/linux_compat.c

2083

This is UB, generally. I think it is not since we compile with -fwrapv.

[olce]

Why not call linux_jiffies_timeout_to_ticks() only once from linux_add_to_sleepqueue(), in the concerned callee, instead of in all callers?

[markj]

In D50192#1145759, @olce wrote:

Why not call linux_jiffies_timeout_to_ticks() only once from linux_add_to_sleepqueue(), in the concerned callee, instead of in all callers?

I reflexively did it that way to avoid adding more code under the sleepqueue locks, but yes it's probably better to keep things simple.

[markj]

Push the conversion into linux_add_to_sleepqueue().

[dumbbell]

sys/compat/linuxkpi/common/src/linux_schedule.c

45–56

I prepared a similar change in D49808. Do you want me to push it anyway? I can wait for this revision to come to a conclusion otherwise.

[dumbbell]

Beside the conflict with D49808, this looks good to me.

Your version of the code that conflicts with mine looks more complete now that the timeout is a long. Perhaps you should commit this one and I will rebase my patch on top of this.

[markj]

In D50192#1146268, @dumbbell wrote:

Beside the conflict with D49808, this looks good to me.

Your version of the code that conflicts with mine looks more complete now that the timeout is a long. Perhaps you should commit this one and I will rebase my patch on top of this.

That'd be easiest, I think. Sorry for the conflicts.

I need a day or two to test this on my desktop. If anyone else has a chance to test, please let me know.

[dumbbell]

In D50192#1146316, @markj wrote:

That'd be easiest, I think. Sorry for the conflicts.

No problem! In fact, I’m leaning toward abandonning D49080 because that patch only moves parts of linux_schedule_timeout() to their own functions called once. I find it doesn’t make the code easier to read anymore. What do you think?

[markj]

I've been running this patch on my desktop. I'll commit it tomorrow if there are no objections.

In D50192#1147097, @dumbbell wrote:

In D50192#1146316, @markj wrote:

That'd be easiest, I think. Sorry for the conflicts.

No problem! In fact, I’m leaning toward abandonning D49080 because that patch only moves parts of linux_schedule_timeout() to their own functions called once. I find it doesn’t make the code easier to read anymore. What do you think?

I guess you meant D49808? I think it's fine to drop it, indeed.

[dumbbell]

Correct, I mistyped the review number. Ok, I will drop it, thank you!

[dumbbell]

I’m running this patch today too.

[bz]

I've been running this since Saturday for wifi and i915.