linuxkpi: Add `woken_wake_function()` and `wait_woken()`

[dumbbell]

Summary

They are used by the i915 DRM driver starting with Linux 6.7.

(struct wait_queue)->flags is no longer always zero. I wonder if some code relied on this...

This is part of the update of DRM drivers to Linux 6.7.

Sponsored by: The FreeBSD Foundation

[dumbbell]

Add diff context. No other changes.

[bz]

I don't know the internals of this. It looks sound. Adding @markj given he seems to have written the code.

sys/compat/linuxkpi/common/include/linux/wait.h

90

Couldn't they still be re#defined ?

93

No extern for function declarations.

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

199

No need for {}

[markj]

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

189

What's the exact purpose of these memory barriers?

199

There's no mutual exclusion here, so two threads can both observe that WQ_FLAG_WOKEN is not set and schedule a timeout. Is that going to cause problems?

[dumbbell]

I made local changes to address @bz comments, but I will only upload it when other concerns are addressed.

sys/compat/linuxkpi/common/include/linux/wait.h

90

What do you mean?

93

Ok, I will remove extern for all three declarations.

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

189

This is to synchronize with the set_task_state() call at the beginning of wait_woken(). On Linux, set_task_state() contains a memory barrier. I just looked and our copy does not, except if atomic_set() implies one? I guess not.

Therefore this memory barrier alone is probably ineffective. Should I add a memory barrier before the if in wait_woken() or should set_task_current() be modified to include a barrier? I would vote for the latter to be closer to Linux behavior, but I don’t have a clear understanding of this code.

199

That I don’t know :-(

[markj]

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

189

Could you please provide some background - what do these functions actually do, and how are they used?

[dumbbell]

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

189

The i915 driver uses it to wait for the execution of a command sent to a controller. I understand the thread could be woken up for several reasons but it explicitly wants to wait for the hardware.

The function that sleeps waits for that WQ_FLAG_WOKEN using an internal copy of wait_woken() that loops, checking if the flag is set. The flag is eventually set by the woken_wake_function() that was used to define the wait.

The use of these two function is documented by an example in Linux:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/kernel/sched/wait.c?h=v6.7#n394

Here is the function in i915 that uses them:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c?h=v6.7#n4767

The internal copy of wait_woken() is earlier in the same file:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c?h=v6.7#n4735

199

After reading a bit more, it seems to me that each thread is supposed to use its own struct wait_queue local variable, so they should just observe their own flag.

Sorry, this is still blurry in my mind as I never dug deeply into this code.

[markj]

I think the implementation should look more like linux_wait_on_bit_timeout() and linux_wake_up_bit(). In particular, we should use sleepqueue locks (keyed by wq->private) to synchronize the checks; this lockless approach is basically duplicating Linux internals. For instance, their implementation makes use of the fact that set_task_state() issues a full barrier in order to synchronize some scheduler function, but that is not true in the LinuxKPI.

In particular, I think something like the following would be easier to understand and would look more like other routines in this file:

linux_wait_woken(...)
{
    void *wchan = wq->private;
    for (;;) {
        sleepq_lock(whcan);
        if (wq->flags & WQ_FLAG_WOKEN) {
            sleepq_unlock(wchan);
            break;
       }
       set_task_state(current, state);
       <sleep>
       <break if timeout elapsed>
    }
}

woken_wait_function(...)
{
    void *wchan = wq->private;

     sleepq_lock(wchan);
     wq->flags &= ~WQ_FLAG_WOKEN;
     sleepq_signal(...);
     sleepq_release(wchan);
}

[dumbbell]

Thank you @markj for the detailed feedback! I reimplemented the patch based on your suggestion and will test it shortly.

I didn’t keep the infinite loop in wait_woken() because the Linux implementation doesn’t have one: the caller is supposed to do the loop.

[dumbbell]

Implement @markj suggestions.

[dumbbell]

Remove #include <asm/barrier.h>, no longer needed.

[markj]

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

193

I think you actually need to use linux_add_to_sleepqueue() here?

195

linux_schedule_timeout() will return with current->state == TASK_RUNNING, so I think this should be

if (...)
    timeout = linux_schedule_timeout(timeout);
else
    set_task_state(current, TASK_RUNNING);

[dumbbell]

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

193

But then, wouldn’t I need to copy some of the logic already present in linux_schedule_timeout(), like the timeout computation or the check of state != TASK_WAKING?

[dumbbell]

Address comment from @markj.

[markj]

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

193

It seems so, yes. This is already somewhat duplicated among linux_add_to_sleepqueue() callers, so some refactoring would be good to add.

[dumbbell]

@markj: Thank you, I will work on a refactor. Do you want me to submit the refactor in this review or separately? I’m leaning toward the latter.

[markj]

In D48755#1130763, @dumbbell wrote:

@markj: Thank you, I will work on a refactor. Do you want me to submit the refactor in this review or separately? I’m leaning toward the latter.

A separate review seems fine to me.

[dumbbell]

@markj: I submitted D49808 and D49809 to factorize some code.

[dumbbell]

@bz and @markj: Now that D49808 and D49809 are accepted, could you please give this patch a final review?

In particular, I didn’t understand your "Couldn't they still be re#defined ?" concern, @bz.

[markj]

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

193

This is still not corrected. Here we're acquiring a sleepqueue lock, then calling schedule_timeout(), which might acquire the same sleepqueue lock. And, woken_wait_function() is waking up threads using wchan as the unique key, but here we are not using wchan as the key, so I don't think this function works.

[dumbbell]

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

193

Oh, I see what you mean. That was a misunderstanding on my part.

I reworked the linux_wait_woken() function to only lock wq->private and not the task itself.

This means I expanded D49808 and reverted D49809 because I don’t see how to factorize that code beside the handling of the timeout. In the end, linux_wait_event_common(), linux_schedule_timeout() and linux_wait_woken() have subtle changes. I will upload the new patches for D49808 and this one.

[dumbbell]

Rework the patch to correctly lock wq->private only, not the task.

[dumbbell]

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

193

I also submitted D49914 to fix a difference of behaviout between Linux and linuxkpi.

[bz]

sys/compat/linuxkpi/common/include/linux/wait.h

90

I meant that we could still have used a #define foo linuxkpi_foo and have the implementation be linuxkpi_foo. But I am not sure if this related to this review anymore or if I just read the comment above the "extern" bits as well. If we'll do this we probably should do it separately anyway. So please ignore.