Paths

Table of Contentst

linuxkpi: Fix up jiffies handling
ClosedPublic
Actions

Authored by markj on May 6 2025, 2:36 PM.

Details

Reviewers

kib
dumbbell
bz
olce

Group Reviewers

linuxkpi

Commits

rGeed3be47967f: linuxkpi: Fix up jiffies handling

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")

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 63999
Build 60883: arc lint + arc unit

Event Timeline

markj created this revision.May 6 2025, 2:36 PM

Herald added subscribers: emaste, imp. · View Herald TranscriptMay 6 2025, 2:36 PM

markj requested review of this revision.May 6 2025, 2:36 PM

Harbormaster completed remote builds in B63942: Diff 154945.May 6 2025, 2:36 PM

kib accepted this revision.May 6 2025, 5:33 PM

kib added inline comments.

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.

This revision is now accepted and ready to land.May 6 2025, 5:33 PM

Does __wait_event_common need similar treatment?

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

This revision now requires review to proceed.May 7 2025, 12:43 PM

Harbormaster completed remote builds in B63993: Diff 155056.May 7 2025, 12:43 PM

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 accepted this revision.May 7 2025, 1:34 PM

kib added inline comments.

sys/compat/linuxkpi/common/src/linux_compat.c
2083	This is UB, generally. I think it is not since we compile with -fwrapv.

This revision is now accepted and ready to land.May 7 2025, 1:34 PM

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

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.

Push the conversion into linux_add_to_sleepqueue().

This revision now requires review to proceed.May 7 2025, 6:18 PM

Harbormaster completed remote builds in B63999: Diff 155069.May 7 2025, 6:18 PM

dumbbell added a subscriber: dumbbell.May 8 2025, 10:02 AM

dumbbell added inline comments.

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.

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.

This revision is now accepted and ready to land.May 8 2025, 11:05 AM

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.

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'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?