[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <2c421e36-a749-7dc3-3562-7a8cf256df3c@efficios.com>
Date: Mon, 29 May 2023 15:48:44 -0400
From: Mathieu Desnoyers <mathieu.desnoyers@...icios.com>
To: Florian Weimer <fweimer@...hat.com>
Cc: Peter Zijlstra <peterz@...radead.org>,
linux-kernel@...r.kernel.org, Thomas Gleixner <tglx@...utronix.de>,
"Paul E . McKenney" <paulmck@...nel.org>,
Boqun Feng <boqun.feng@...il.com>,
"H . Peter Anvin" <hpa@...or.com>, Paul Turner <pjt@...gle.com>,
linux-api@...r.kernel.org, Christian Brauner <brauner@...nel.org>,
David.Laight@...LAB.COM, carlos@...hat.com,
Peter Oskolkov <posk@...k.io>,
Alexander Mikhalitsyn <alexander@...alicyn.com>,
Chris Kennelly <ckennelly@...gle.com>,
Ingo Molnar <mingo@...hat.com>,
Darren Hart <dvhart@...radead.org>,
Davidlohr Bueso <dave@...olabs.net>,
André Almeida <andrealmeid@...lia.com>,
libc-alpha@...rceware.org, Steven Rostedt <rostedt@...dmis.org>,
Jonathan Corbet <corbet@....net>,
Noah Goldstein <goldstein.w.n@...il.com>,
Daniel Colascione <dancol@...gle.com>, longman@...hat.com
Subject: Re: [RFC PATCH v2 1/4] rseq: Add sched_state field to struct rseq
On 5/29/23 15:35, Florian Weimer wrote:
> * Mathieu Desnoyers:
>
>> +/*
>> + * rseq_sched_state should be aligned on the cache line size.
>> + */
>> +struct rseq_sched_state {
>> + /*
>> + * Version of this structure. Populated by the kernel, read by
>> + * user-space.
>> + */
>> + __u32 version;
>> + /*
>> + * The state is updated by the kernel. Read by user-space with
>> + * single-copy atomicity semantics. This field can be read by any
>> + * userspace thread. Aligned on 32-bit. Contains a bitmask of enum
>> + * rseq_sched_state_flags. This field is provided as a hint by the
>> + * scheduler, and requires that the page holding this state is
>> + * faulted-in for the state update to be performed by the scheduler.
>> + */
>> + __u32 state;
>> + /*
>> + * Thread ID associated with the thread registering this structure.
>> + * Initialized by user-space before registration.
>> + */
>> + __u32 tid;
>> +};
>
> How does the version handshake protocol in practice? Given that this
> user-allocated?
Good point, I must admit that I have not thought this specific version
protocol through. :) As you say, userspace is responsible for
allocation, and the kernel is responsible for implementing features.
Let's first see if we can get away with embedding these fields in struct
rseq.
>
> I don't see why we can't stick this directly into struct rseq because
> it's all public anyway.
The motivation for moving this to a different cache line is to handle
the prior comment from Boqun, who is concerned that busy-waiting
repeatedly loading a field from struct rseq will cause false-sharing and
make other stores to that cache line slower, especially stores to
rseq_cs to begin rseq critical sections, thus slightly increasing the
overhead of rseq critical sections taken while mutexes are held.
If we want to embed this field into struct rseq with its own cache line,
then we need to add a lot of padding, which is inconvenient.
That being said, perhaps this is premature optimization, what do you think ?
>
> The TID field would be useful in its own right.
Indeed, good point.
While we are there, I wonder if we should use the thread_pointer() as
lock identifier, or if the address of struct rseq is fine ?
Thanks,
Mathieu
>
> Thanks,
> Florian
>
--
Mathieu Desnoyers
EfficiOS Inc.
https://www.efficios.com
Powered by blists - more mailing lists