Messages in this thread | | | Date | Tue, 23 May 2023 13:30:09 -0400 | Subject | Re: [RFC PATCH 1/4] rseq: Add sched_state field to struct rseq | From | Mathieu Desnoyers <> |
| |
On 2023-05-23 12:32, Noah Goldstein wrote: > On Tue, May 23, 2023 at 7:49 AM Mathieu Desnoyers > <mathieu.desnoyers@efficios.com> wrote: >> >> On 2023-05-19 16:51, Noah Goldstein wrote: >>> On Wed, May 17, 2023 at 10:28 AM Mathieu Desnoyers via Libc-alpha >>> <libc-alpha@sourceware.org> wrote: >>>> >>>> Expose the "on-cpu" state for each thread through struct rseq to allow >>>> adaptative mutexes to decide more accurately between busy-waiting and >>>> calling sys_futex() to release the CPU, based on the on-cpu state of the >>>> mutex owner. >>>> >>>> It is only provided as an optimization hint, because there is no >>>> guarantee that the page containing this field is in the page cache, and >>>> therefore the scheduler may very well fail to clear the on-cpu state on >>>> preemption. This is expected to be rare though, and is resolved as soon >>>> as the task returns to user-space. >>>> >>>> The goal is to improve use-cases where the duration of the critical >>>> sections for a given lock follows a multi-modal distribution, preventing >>>> statistical guesses from doing a good job at choosing between busy-wait >>>> and futex wait behavior. >>>> >>>> Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com> >>>> Cc: Peter Zijlstra (Intel) <peterz@infradead.org> >>>> Cc: Jonathan Corbet <corbet@lwn.net> >>>> Cc: Steven Rostedt (Google) <rostedt@goodmis.org> >>>> Cc: Carlos O'Donell <carlos@redhat.com> >>>> Cc: Florian Weimer <fweimer@redhat.com> >>>> Cc: libc-alpha@sourceware.org >>>> --- >>>> include/linux/sched.h | 12 ++++++++++++ >>>> include/uapi/linux/rseq.h | 17 +++++++++++++++++ >>>> kernel/rseq.c | 14 ++++++++++++++ >>>> 3 files changed, 43 insertions(+) >>>> >>>> diff --git a/include/linux/sched.h b/include/linux/sched.h >>>> index eed5d65b8d1f..c7e9248134c1 100644 >>>> --- a/include/linux/sched.h >>>> +++ b/include/linux/sched.h >>>> @@ -2351,11 +2351,20 @@ static inline void rseq_signal_deliver(struct ksignal *ksig, >>>> rseq_handle_notify_resume(ksig, regs); >>>> } >>>> >>>> +void __rseq_set_sched_state(struct task_struct *t, unsigned int state); >>>> + >>>> +static inline void rseq_set_sched_state(struct task_struct *t, unsigned int state) >>>> +{ >>>> + if (t->rseq) >>>> + __rseq_set_sched_state(t, state); >>>> +} >>>> + >>>> /* rseq_preempt() requires preemption to be disabled. */ >>>> static inline void rseq_preempt(struct task_struct *t) >>>> { >>>> __set_bit(RSEQ_EVENT_PREEMPT_BIT, &t->rseq_event_mask); >>>> rseq_set_notify_resume(t); >>>> + rseq_set_sched_state(t, 0); >>> >>> Should rseq_migrate also be made to update the cpu_id of the new core? >>> I imagine the usage of this will be something along the lines of: >>> >>> if(!on_cpu(mutex->owner_rseq_struct) && >>> cpu(mutex->owner_rseq_struct) == this_threads_cpu) >>> // goto futex >>> >>> So I would think updating on migrate would be useful as well. >> >> I don't think we want to act differently based on the cpu on which the >> owner is queued. >> >> If the mutex owner is not on-cpu, and queued on the same cpu as the >> current thread, we indeed want to call sys_futex WAIT. >> >> If the mutex owner is not on-cpu, but queued on a different cpu than the >> current thread, we *still* want to call sys_futex WAIT, because >> busy-waiting for a thread which is queued but not currently running is >> wasteful. >> > I think this is less clear. In some cases sure but not always. Going > to the futex > has more latency that userland waits, and if the system is not busy (other than > the one process) most likely less latency that yield. Also going to the futex > requires a syscall on unlock. > > For example if the critical section is expected to be very small, it > would be easy > to imagine the lock be better implemented with: > while(is_locked) > if (owner->on_cpu || owner->cpu != my_cpu) > exponential backoff > else > yield > > Its not that "just go to futex" doesn't ever make sense, but I don't > think its fair > to say that *always* the case. > > Looking at the kernel code, it doesn't seem to be a particularly high cost to > keep the CPU field updated during migration so seems like a why not > kind of question.
We already have the owner rseq_abi cpu_id field populated on every return-to-userspace. I wonder if it's really relevant that migration populates an updated value in this field immediately ? It's another case where this would be provided as a hint updated only if the struct rseq is in the page cache, because AFAIU the scheduler migration path cannot take a page fault.
Also, if a thread bounces around many runqueues before being scheduled again, we would be adding those useless stores to the rseq_abi structure at each migration between runqueues.
Given this would add some complexity to the scheduler migration code, I would want to see metrics/benchmarks showing that it indeed improves real-world use-cases before adding this to the rseq ABI.
It's not only a question of added lines of code as of today, but also a question of added userspace ABI guarantees which can prevent future scheduler optimizations. I'm *very* careful about keeping those to a strict minimum, which I hope Peter Zijlstra appreciates.
Thanks,
Mathieu
>> Or am I missing something ? >> >> Thanks, >> >> Mathieu >> >> -- >> Mathieu Desnoyers >> EfficiOS Inc. >> https://www.efficios.com >>
-- Mathieu Desnoyers EfficiOS Inc. https://www.efficios.com
| |