Messages in this thread | ![/](/images/icornerl.gif) | | Date | Mon, 16 Jan 2023 10:59:34 +0100 | From | Peter Zijlstra <> | Subject | Re: [PATCH 3/5] sched: add a few helpers to wake up tasks on the current cpu |
| |
On Tue, Jan 10, 2023 at 01:30:08PM -0800, Andrei Vagin wrote:
> diff --git a/kernel/sched/completion.c b/kernel/sched/completion.c > index d57a5c1c1cd9..a1931a79c05a 100644 > --- a/kernel/sched/completion.c > +++ b/kernel/sched/completion.c > @@ -38,6 +38,18 @@ void complete(struct completion *x) > } > EXPORT_SYMBOL(complete); > > +void complete_on_current_cpu(struct completion *x) > +{ > + unsigned long flags; > + > + raw_spin_lock_irqsave(&x->wait.lock, flags); > + > + if (x->done != UINT_MAX) > + x->done++; > + swake_up_locked_on_current_cpu(&x->wait); > + raw_spin_unlock_irqrestore(&x->wait.lock, flags); > +} > + > /** > * complete_all: - signals all threads waiting on this completion > * @x: holds the state of this particular completion > diff --git a/kernel/sched/core.c b/kernel/sched/core.c > index 6478e819eb99..c81866821139 100644 > --- a/kernel/sched/core.c > +++ b/kernel/sched/core.c > @@ -6874,7 +6874,7 @@ asmlinkage __visible void __sched preempt_schedule_irq(void) > int default_wake_function(wait_queue_entry_t *curr, unsigned mode, int wake_flags, > void *key) > { > - WARN_ON_ONCE(IS_ENABLED(CONFIG_SCHED_DEBUG) && wake_flags & ~WF_SYNC); > + WARN_ON_ONCE(IS_ENABLED(CONFIG_SCHED_DEBUG) && wake_flags & ~(WF_SYNC|WF_CURRENT_CPU)); > return try_to_wake_up(curr->private, mode, wake_flags); > } > EXPORT_SYMBOL(default_wake_function); > diff --git a/kernel/sched/swait.c b/kernel/sched/swait.c > index 76b9b796e695..9ebe23868942 100644 > --- a/kernel/sched/swait.c > +++ b/kernel/sched/swait.c > @@ -31,6 +31,17 @@ void swake_up_locked(struct swait_queue_head *q) > } > EXPORT_SYMBOL(swake_up_locked); > > +void swake_up_locked_on_current_cpu(struct swait_queue_head *q) > +{ > + struct swait_queue *curr; > + > + if (list_empty(&q->task_list)) > + return; > + > + curr = list_first_entry(&q->task_list, typeof(*curr), task_list); > + try_to_wake_up(curr->task, TASK_NORMAL, WF_CURRENT_CPU); > + list_del_init(&curr->task_list); > +} > /* > * Wake up all waiters. This is an interface which is solely exposed for > * completions and not for general usage. > diff --git a/kernel/sched/wait.c b/kernel/sched/wait.c > index 133b74730738..47803a0b8d5d 100644 > --- a/kernel/sched/wait.c > +++ b/kernel/sched/wait.c > @@ -161,6 +161,11 @@ int __wake_up(struct wait_queue_head *wq_head, unsigned int mode, > } > EXPORT_SYMBOL(__wake_up); > > +void __wake_up_on_current_cpu(struct wait_queue_head *wq_head, unsigned int mode, void *key) > +{ > + __wake_up_common_lock(wq_head, mode, 1, WF_CURRENT_CPU, key); > +} > + > /* > * Same as __wake_up but called with the spinlock in wait_queue_head_t held. > */
So I hate this patch, it adds a whole ton of duplication and litter for no real reason afaict. For instance you could've just added an wake_flags argument to swake_up_locked(), there's not *that* many users -- unlike complete().
And for complete() instead of fully duplicating the function (always a bad idea) you could've made complete_flags() and implemented complete() using that, or something.
Anyway, let me go (finally) have a look at Chen's patch, since that might render all this moot.
| ![\](/images/icornerr.gif) |