Linus,
Please pull the latest locking tree from:
git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git locking-core-2022-12-12
# HEAD: 90d758896787048fa3d4209309d4800f3920e66f futex: Resend potentially swallowed owner death notification
Two changes in this cycle:
- a micro-optimization in static_key_slow_inc_cpuslocked()
- fix futex death-notification wakeup bug
Thanks,
Ingo
------------------>
Alexey Izbyshev (1):
futex: Resend potentially swallowed owner death notification
Uros Bizjak (1):
jump_label: Use atomic_try_cmpxchg() in static_key_slow_inc_cpuslocked()
kernel/futex/core.c | 26 +++++++++++++++++---------
kernel/jump_label.c | 8 ++------
2 files changed, 19 insertions(+), 15 deletions(-)
The pull request you sent on Mon, 12 Dec 2022 21:16:13 +0100:
> git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git locking-core-2022-12-12
has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/617fe4fa82b2fe5bcb99f97f223f408603bfa5a0
Thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/prtracker.html
Linus,
Please pull the latest locking tree from:
git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git locking-core-2023-02-20
# HEAD: 3b4863fa5b7dd50dab1b10abbed938efd203752f vduse: Remove include of rwlock.h
Updates for this cycle were:
- rwsem micro-optimizations
- spinlock micro-optimizations
- cleanups, simplifications
Thanks,
Ingo
------------------>
Guo Ren (1):
locking/qspinlock: Micro-optimize pending state waiting for unlock
Sebastian Andrzej Siewior (2):
locking/lockdep: Remove lockdep_init_map_crosslock.
vduse: Remove include of rwlock.h
Uros Bizjak (2):
x86/PAT: Use try_cmpxchg() in set_page_memtype()
x86/ACPI/boot: Use try_cmpxchg() in __acpi_{acquire,release}_global_lock()
Waiman Long (3):
locking/rwsem: Prevent non-first waiter from spinning in down_write() slowpath
locking/rwsem: Disable preemption in all down_read*() and up_read() code paths
locking/rwsem: Disable preemption in all down_write*() and up_write() code paths
arch/x86/kernel/acpi/boot.c | 16 +++----
arch/x86/mm/pat/memtype.c | 4 +-
drivers/vdpa/vdpa_user/iova_domain.h | 1 -
include/linux/lockdep.h | 1 -
kernel/locking/qspinlock.c | 4 +-
kernel/locking/rwsem.c | 87 +++++++++++++++++++++---------------
6 files changed, 64 insertions(+), 49 deletions(-)
The pull request you sent on Mon, 20 Feb 2023 13:25:50 +0100:
> git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git locking-core-2023-02-20
has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/6e649d08568220ee88deef0a1ad8b3a935420cf2
Thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/prtracker.html
Linus,
Please pull the latest locking/core git tree from:
git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git locking-core-2023-04-27
# HEAD: 93eff603d6a2bc1895eeb7063dbd0661bb760b74 locking/atomic: Correct (cmp)xhcg instrumentation
Locking changes in v6.4:
- Introduce local{,64}_try_cmpxchg() - a slightly more optimal
primitive, which will be used in perf events ring-buffer code.
- Add more atomic_add_negative() variants, used by rcuref:
- Add rcuref - A scalable reference count implementation for RCU managed objects
- Add non-atomic __xchg() variant, use it in a couple of places
- Misc cleanups and fixes.
NOTE: the internal merge commit of locking/rcuref [1afa833d607b6] merging
two commits from a WIP branch has no explanation. I noticed this too
late, and we can re-do this tree if it's an issue.
Thanks,
Ingo
------------------>
Andrzej Hajda (5):
arch: rename all internal names __xchg to __arch_xchg
linux/include: add non-atomic version of xchg
arch/*/uprobes: simplify arch_uretprobe_hijack_return_addr
llist: simplify __llist_del_all
drm/i915/gt: use __xchg instead of internal helper
Mark Rutland (1):
locking/atomic: Correct (cmp)xhcg instrumentation
Sebastian Andrzej Siewior (1):
locking/rwbase: Mitigate indefinite writer starvation.
Thomas Gleixner (2):
atomics: Provide atomic_add_negative() variants
atomics: Provide rcuref - scalable reference counting
Uros Bizjak (4):
locking/atomic: Add generic try_cmpxchg{,64}_local support
locking/generic: Wire up local{,64}_try_cmpxchg
locking/arch: Wire up local_try_cmpxchg
locking/x86: Define arch_try_cmpxchg_local
arch/alpha/include/asm/cmpxchg.h | 10 +-
arch/alpha/include/asm/local.h | 12 +-
arch/arc/include/asm/cmpxchg.h | 4 +-
arch/arm/include/asm/cmpxchg.h | 7 +-
arch/arm/probes/uprobes/core.c | 8 +-
arch/arm64/include/asm/cmpxchg.h | 7 +-
arch/arm64/kernel/probes/uprobes.c | 9 +-
arch/csky/kernel/probes/uprobes.c | 9 +-
arch/hexagon/include/asm/cmpxchg.h | 10 +-
arch/ia64/include/asm/cmpxchg.h | 2 +-
arch/ia64/include/uapi/asm/cmpxchg.h | 4 +-
arch/loongarch/include/asm/cmpxchg.h | 4 +-
arch/loongarch/include/asm/local.h | 13 +-
arch/m68k/include/asm/cmpxchg.h | 6 +-
arch/mips/include/asm/cmpxchg.h | 4 +-
arch/mips/include/asm/local.h | 13 +-
arch/mips/kernel/uprobes.c | 10 +-
arch/openrisc/include/asm/cmpxchg.h | 10 +-
arch/parisc/include/asm/cmpxchg.h | 4 +-
arch/powerpc/include/asm/cmpxchg.h | 4 +-
arch/powerpc/include/asm/local.h | 11 +
arch/powerpc/kernel/uprobes.c | 10 +-
arch/riscv/include/asm/atomic.h | 2 +-
arch/riscv/include/asm/cmpxchg.h | 4 +-
arch/riscv/kernel/probes/uprobes.c | 9 +-
arch/s390/include/asm/cmpxchg.h | 8 +-
arch/s390/kernel/uprobes.c | 7 +-
arch/sh/include/asm/cmpxchg.h | 4 +-
arch/sparc/include/asm/cmpxchg_32.h | 4 +-
arch/sparc/include/asm/cmpxchg_64.h | 6 +-
arch/sparc/kernel/uprobes.c | 7 +-
arch/x86/include/asm/cmpxchg.h | 6 +
arch/x86/include/asm/local.h | 13 +-
arch/xtensa/include/asm/cmpxchg.h | 4 +-
drivers/gpu/drm/i915/gt/intel_engine_cs.c | 2 +-
drivers/gpu/drm/i915/gt/intel_engine_heartbeat.c | 4 +-
.../gpu/drm/i915/gt/intel_execlists_submission.c | 4 +-
drivers/gpu/drm/i915/gt/intel_ggtt.c | 4 +-
drivers/gpu/drm/i915/gt/intel_gsc.c | 2 +-
drivers/gpu/drm/i915/gt/intel_gt.c | 4 +-
drivers/gpu/drm/i915/gt/intel_gt_pm.c | 2 +-
drivers/gpu/drm/i915/gt/intel_lrc.c | 6 +-
drivers/gpu/drm/i915/gt/intel_migrate.c | 2 +-
drivers/gpu/drm/i915/gt/intel_rc6.c | 2 +-
drivers/gpu/drm/i915/gt/intel_rps.c | 2 +-
drivers/gpu/drm/i915/gt/selftest_context.c | 2 +-
drivers/gpu/drm/i915/gt/selftest_ring_submission.c | 2 +-
drivers/gpu/drm/i915/gt/selftest_timeline.c | 2 +-
drivers/gpu/drm/i915/gt/uc/intel_uc.c | 2 +-
drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c | 2 +-
drivers/gpu/drm/i915/i915_utils.h | 1 +
include/asm-generic/local.h | 1 +
include/asm-generic/local64.h | 12 +-
include/linux/atomic/atomic-arch-fallback.h | 230 ++++++++++++++++-
include/linux/atomic/atomic-instrumented.h | 152 ++++++++---
include/linux/atomic/atomic-long.h | 38 ++-
include/linux/llist.h | 6 +-
include/linux/non-atomic/xchg.h | 19 ++
include/linux/rcuref.h | 155 ++++++++++++
include/linux/types.h | 6 +
kernel/locking/rwbase_rt.c | 9 -
lib/Makefile | 2 +-
lib/rcuref.c | 281 +++++++++++++++++++++
scripts/atomic/atomics.tbl | 2 +-
scripts/atomic/fallbacks/add_negative | 11 +-
scripts/atomic/gen-atomic-fallback.sh | 4 +
scripts/atomic/gen-atomic-instrumented.sh | 8 +-
67 files changed, 1019 insertions(+), 207 deletions(-)
create mode 100644 include/linux/non-atomic/xchg.h
create mode 100644 include/linux/rcuref.h
create mode 100644 lib/rcuref.c
On Thu, Apr 27, 2023 at 12:58 PM Ingo Molnar <[email protected]> wrote:
>
> - Add non-atomic __xchg() variant, use it in a couple of places
Guys, this is insane, and completely unacceptable.
I pulled this, but I'm going to unpull it, because the code is
actively wrong and ugly.
It not only randomly decides to re-use a name that has existing users
that now need to be fixed up.
It then *also* decides to start "preferring" this absolutely
disgusting new name over a much more legible one in the i915 driver,
which had this same functionality except it used a prettier name:
fetch_and_zero()
But what then takes the cake for me is that this horribly ugly feature
then didn't even get that right, and only randomly converted *some* of
the users, with most of them remaining:
git grep fetch_and_zero drivers/gpu/drm/i915/ | wc
58 187 5534
git grep -w __xchg drivers/gpu/drm/i915/ | wc
22 109 1899
and it looks like the only "logic" to this is that the converted ones
were in the "gt/" subdirectory. What a random choice, but happily it
caused a trivial conflict, and as a result I noticed how bad things
were.
Anyway, I really find this all offensively ugly and pointless. I'm not
going to pull some "fixed" version of this. This needs to go away and
never come back.
What was so magically great about the name "__xchg" that it needed to
be taken over by this function? And why was that legibly named version
of it replaced so randomly?
The *whole* point of two underscores is to say "don't use this - it's
an internal implementation". That's the historical meaning, and it's
the meaning we have in the kernel too. Two underscores means "this is
special and doesn't do everything required" (it might need locking
around it, for example).
So then making a new interface with two underscores and thinking "we
should now make random drivers use this" is fundamentally bogus.
Look, just grep for "__xchg" in the main tree (ie the one *without*
this change). It all makes sense. It's all clearly an internal helper
- as marked by that double underscore - and it's not used by any
driver or filesystem code.
Exactly like K&R and God intended.
Linus
* Linus Torvalds <[email protected]> wrote:
> On Thu, Apr 27, 2023 at 12:58 PM Ingo Molnar <[email protected]> wrote:
> >
> > - Add non-atomic __xchg() variant, use it in a couple of places
>
> Guys, this is insane, and completely unacceptable.
>
> I pulled this, but I'm going to unpull it, because the code is
> actively wrong and ugly.
>
> It not only randomly decides to re-use a name that has existing users
> that now need to be fixed up.
meh - you are 100% right, I'm not sure what we were thinking there ... [
actually, I know what we were thinking, but it's a bit complicated - see
the various non-perfect nomenclature options further below. ]
So the first line of our thinking was that "__" also often & additionally
means 'lighter weight version of a similar API signature, beware, here be
dragons, use at your own risk', and more of the focus of these particular
changes was on identifying hand-coded xchg-ish pieces of code, such as in:
26ace5d28d36 ("arch/*/uprobes: simplify arch_uretprobe_hijack_return_addr")
... but while that background of '__' is somewhat valid logic that we use
quite often in various kernel facilities, it doesn't really excuse the
sloppy decision to slap __ in front of an existing API without trying
harder, *especially* that a better name with fetch_and_zero() already
existed :-/
> It then *also* decides to start "preferring" this absolutely
> disgusting new name over a much more legible one in the i915 driver,
> which had this same functionality except it used a prettier name:
>
> fetch_and_zero()
>
> But what then takes the cake for me is that this horribly ugly feature
> then didn't even get that right, and only randomly converted *some* of
> the users, with most of them remaining:
>
> git grep fetch_and_zero drivers/gpu/drm/i915/ | wc
> 58 187 5534
> git grep -w __xchg drivers/gpu/drm/i915/ | wc
> 22 109 1899
>
> and it looks like the only "logic" to this is that the converted ones
> were in the "gt/" subdirectory. What a random choice, but happily it
> caused a trivial conflict, and as a result I noticed how bad things
> were.
>
> Anyway, I really find this all offensively ugly and pointless. I'm not
> going to pull some "fixed" version of this. This needs to go away and
> never come back.
Yeah. So I've rebased locking/core to take out these changes - a simple
revert is too ugly and the history has no value here really.
Will re-send the rest of locking/core.
> What was so magically great about the name "__xchg" that it needed to be
> taken over by this function? And why was that legibly named version of it
> replaced so randomly?
Yeah.
So fetch_and_zero() has a bit of a nomenclature & ambiguity problem as
well: there's already an atomic_fetch_*() API family, and it's easy to
think that fetch_and_zero() is atomic too - a bit like how xchg() is atomic
without mentioning 'atomic'.
Adding to the confusion is that there's already atomic APIs that don't use
atomic_t:
xchg()
cmpxchg()
try_cmpxchg()
... and by *that* implicit nomenclature logic, dropping the atomic_ from a
atomic_fetch_and_zero() API means: 'atomic API, not using atomic_t'. Which
fetch_and_zero() clearly isnt ...
So by all that logic and somewhat idiosynchratic API history, the new
facility should probably not be fetch_and_zero(), but something like
nonatomic_fetch_and_zero(), but that's quite a mouthful for something so
simple - and the API family connection to xchg() is lost as well, which is
a bit sad...
In all that context the least bad approach sounded to add a __ to denote
__xchg() is 'something special and also lighter weight' (which it is).
I *think* the bigger danger in locking nomenclature is to falsely imply
atomicity - in that sense I'm not sure fetch_and_zero() is ideal - but I
can certainly live with it b/c the perfect name keeps eluding me.
> The *whole* point of two underscores is to say "don't use this - it's
> an internal implementation". That's the historical meaning, and it's
> the meaning we have in the kernel too. Two underscores means "this is
> special and doesn't do everything required" (it might need locking
> around it, for example).
Yeah. I do think we might want to keep one related change though:
e27cff3d2d43 ("arch: rename all internal names __xchg to __arch_xchg")
... not because we want to use the __xchg namespace, but because an _arch
prefix makes it even *less* likely to be used by non-infrastructure code.
> So then making a new interface with two underscores and thinking "we
> should now make random drivers use this" is fundamentally bogus.
>
> Look, just grep for "__xchg" in the main tree (ie the one *without* this
> change). It all makes sense. It's all clearly an internal helper - as
> marked by that double underscore - and it's not used by any driver or
> filesystem code.
>
> Exactly like K&R and God intended.
Yeah. We'll try this new facility again in v6.5, but with a better name.
Sorry about that!
Thanks,
Ingo
Hi all,
Sorry for late response.
Just two cents to constructively defend myself.
On 29.04.2023 08:47, Ingo Molnar wrote:
> * Linus Torvalds <[email protected]> wrote:
>
>> On Thu, Apr 27, 2023 at 12:58 PM Ingo Molnar <[email protected]> wrote:
>>> - Add non-atomic __xchg() variant, use it in a couple of places
>> Guys, this is insane, and completely unacceptable.
>>
>> I pulled this, but I'm going to unpull it, because the code is
>> actively wrong and ugly.
>>
>> It not only randomly decides to re-use a name that has existing users
>> that now need to be fixed up.
> meh - you are 100% right, I'm not sure what we were thinking there ... [
> actually, I know what we were thinking, but it's a bit complicated - see
> the various non-perfect nomenclature options further below. ]
>
> So the first line of our thinking was that "__" also often & additionally
> means 'lighter weight version of a similar API signature, beware, here be
> dragons, use at your own risk', and more of the focus of these particular
> changes was on identifying hand-coded xchg-ish pieces of code, such as in:
>
> 26ace5d28d36 ("arch/*/uprobes: simplify arch_uretprobe_hijack_return_addr")
>
> ... but while that background of '__' is somewhat valid logic that we use
> quite often in various kernel facilities, it doesn't really excuse the
> sloppy decision to slap __ in front of an existing API without trying
> harder, *especially* that a better name with fetch_and_zero() already
> existed :-/
>
>> It then *also* decides to start "preferring" this absolutely
>> disgusting new name over a much more legible one in the i915 driver,
>> which had this same functionality except it used a prettier name:
>>
>> fetch_and_zero()
My 1st approach was to follow C++ library and I have proposed 'exchange'
[1][2].
Later I have also showed/suggested Rust's approach [3][4]:
- 'replace' for __xchg,
- 'take' for fetch_and_zero
Anyway it ended as is.
[1]: https://lore.kernel.org/lkml/[email protected]/T/
[2]: https://en.cppreference.com/w/cpp/header/utility
[3]: https://doc.rust-lang.org/std/mem/index.html
[4]:
https://lore.kernel.org/lkml/[email protected]/
>>
>> But what then takes the cake for me is that this horribly ugly feature
>> then didn't even get that right, and only randomly converted *some* of
>> the users, with most of them remaining:
>>
>> git grep fetch_and_zero drivers/gpu/drm/i915/ | wc
>> 58 187 5534
>> git grep -w __xchg drivers/gpu/drm/i915/ | wc
>> 22 109 1899
>>
>> and it looks like the only "logic" to this is that the converted ones
>> were in the "gt/" subdirectory. What a random choice, but happily it
>> caused a trivial conflict, and as a result I noticed how bad things
>> were.
The logic was just to focus on introduction of the helper, providing few
(not all) examples of usage and finish i915 conversion later, when the
helper will be accepted.
Conversion should be split anyway - as i915 uses different repo for
i915/gt, sorry for not clearly stating this.
Regards
Andrzej
>>
>> Anyway, I really find this all offensively ugly and pointless. I'm not
>> going to pull some "fixed" version of this. This needs to go away and
>> never come back.
> Yeah. So I've rebased locking/core to take out these changes - a simple
> revert is too ugly and the history has no value here really.
>
> Will re-send the rest of locking/core.
>
>> What was so magically great about the name "__xchg" that it needed to be
>> taken over by this function? And why was that legibly named version of it
>> replaced so randomly?
> Yeah.
>
> So fetch_and_zero() has a bit of a nomenclature & ambiguity problem as
> well: there's already an atomic_fetch_*() API family, and it's easy to
> think that fetch_and_zero() is atomic too - a bit like how xchg() is atomic
> without mentioning 'atomic'.
>
> Adding to the confusion is that there's already atomic APIs that don't use
> atomic_t:
>
> xchg()
> cmpxchg()
> try_cmpxchg()
>
> ... and by *that* implicit nomenclature logic, dropping the atomic_ from a
> atomic_fetch_and_zero() API means: 'atomic API, not using atomic_t'. Which
> fetch_and_zero() clearly isnt ...
>
> So by all that logic and somewhat idiosynchratic API history, the new
> facility should probably not be fetch_and_zero(), but something like
> nonatomic_fetch_and_zero(), but that's quite a mouthful for something so
> simple - and the API family connection to xchg() is lost as well, which is
> a bit sad...
>
> In all that context the least bad approach sounded to add a __ to denote
> __xchg() is 'something special and also lighter weight' (which it is).
>
> I *think* the bigger danger in locking nomenclature is to falsely imply
> atomicity - in that sense I'm not sure fetch_and_zero() is ideal - but I
> can certainly live with it b/c the perfect name keeps eluding me.
>
>> The *whole* point of two underscores is to say "don't use this - it's
>> an internal implementation". That's the historical meaning, and it's
>> the meaning we have in the kernel too. Two underscores means "this is
>> special and doesn't do everything required" (it might need locking
>> around it, for example).
> Yeah. I do think we might want to keep one related change though:
>
> e27cff3d2d43 ("arch: rename all internal names __xchg to __arch_xchg")
>
> ... not because we want to use the __xchg namespace, but because an _arch
> prefix makes it even *less* likely to be used by non-infrastructure code.
>
>> So then making a new interface with two underscores and thinking "we
>> should now make random drivers use this" is fundamentally bogus.
>>
>> Look, just grep for "__xchg" in the main tree (ie the one *without* this
>> change). It all makes sense. It's all clearly an internal helper - as
>> marked by that double underscore - and it's not used by any driver or
>> filesystem code.
>>
>> Exactly like K&R and God intended.
> Yeah. We'll try this new facility again in v6.5, but with a better name.
> Sorry about that!
>
> Thanks,
>
> Ingo
On Thu, May 4, 2023 at 10:30 AM Andrzej Hajda <[email protected]> wrote:
>
> Later I have also showed/suggested Rust's approach [3][4]:
> - 'replace' for __xchg,
> - 'take' for fetch_and_zero
I don't think either of those are acceptable, and both are to me
*much* worse than the existing "fetch_and_zero()" that i915 uses.
That name just makes sense and explains what it does. There is no ambiguity.
I will not be taking a pull that makes a good name worse.
Naming matters, and "use a generic function" is not an excuse for
having bad names.
Linus