mbox series

[v3,00/57] Scope-based Resource Management

Message ID 20230612090713.652690195@infradead.org (mailing list archive)
Headers show
Series Scope-based Resource Management | expand

Message

Peter Zijlstra June 12, 2023, 9:07 a.m. UTC 
Hi,

After a wee bit of bike-shedding on the syntax/API of the thing I think we're
in a reasonable shape.

There's still the no_free_ptr() vs take_ptr() thing, but that can be easily
sorted with a little script over the patches if we reach consensus.

I've taken to converting kernel/sched/core.c and kernel/events/core.c to see
how well this stuff actually works. Unlike last time, I've split them up into a
gazillion little patches. Both for my sanity -- bisect is genius when you're
trying to debug stuff in the middle of the night as well as reviewer sanity.

These are by no means 'complete' convertions, I've mostly focussed on functions
that had 'goto' in them. Since that's a large part of why I started on all this.

x86_x64-defconfig boots and passes perf test. I'll go figure out how to point
syzcaller at it.

The patches have gone through a few cycles of 0day over the weekend, and mostly
builds fine now.

Dan Carpenter poked me about how sparse doesn't yet understand the __cleanup__
attribute and seems to suffer from decl-after-stmt issues, so that might be
something that needs attention.

Anyway, does this look like something we can live with?

---
 Makefile                            |    6 +-
 arch/arm64/kernel/vdso32/Makefile   |    2 -
 drivers/dma/ioat/dma.c              |   12 +-
 include/linux/cleanup.h             |  167 ++++
 include/linux/compiler-clang.h      |    9 +
 include/linux/compiler_attributes.h |    6 +
 include/linux/cpu.h                 |    2 +
 include/linux/device.h              |    7 +
 include/linux/file.h                |   11 +
 include/linux/irqflags.h            |    7 +
 include/linux/mutex.h               |    4 +
 include/linux/percpu.h              |    4 +
 include/linux/perf_event.h          |   14 +-
 include/linux/preempt.h             |    5 +
 include/linux/rcupdate.h            |    3 +
 include/linux/rwsem.h               |    8 +
 include/linux/sched/task.h          |    2 +
 include/linux/slab.h                |    3 +
 include/linux/spinlock.h            |   31 +
 include/linux/srcu.h                |    5 +
 kernel/events/core.c                | 1642 +++++++++++++++--------------------
 kernel/sched/core.c                 |  931 +++++++++-----------
 kernel/sched/sched.h                |   40 +
 scripts/checkpatch.pl               |    2 +-
 security/apparmor/include/lib.h     |    6 +-
 25 files changed, 1433 insertions(+), 1496 deletions(-)

Comments

Peter Zijlstra June 12, 2023, 9:51 a.m. UTC | #1 
On Mon, Jun 12, 2023 at 11:07:13AM +0200, Peter Zijlstra wrote:
> Hi,
> 
> After a wee bit of bike-shedding on the syntax/API of the thing I think we're
> in a reasonable shape.
> 
> There's still the no_free_ptr() vs take_ptr() thing, but that can be easily
> sorted with a little script over the patches if we reach consensus.
> 
> I've taken to converting kernel/sched/core.c and kernel/events/core.c to see
> how well this stuff actually works. Unlike last time, I've split them up into a
> gazillion little patches. Both for my sanity -- bisect is genius when you're
> trying to debug stuff in the middle of the night as well as reviewer sanity.
> 
> These are by no means 'complete' convertions, I've mostly focussed on functions
> that had 'goto' in them. Since that's a large part of why I started on all this.
> 
> x86_x64-defconfig boots and passes perf test. I'll go figure out how to point
> syzcaller at it.
> 
> The patches have gone through a few cycles of 0day over the weekend, and mostly
> builds fine now.
> 
> Dan Carpenter poked me about how sparse doesn't yet understand the __cleanup__
> attribute and seems to suffer from decl-after-stmt issues, so that might be
> something that needs attention.
> 
> Anyway, does this look like something we can live with?

Forgot to mention; also available at:

  git://git.kernel.org/pub/scm/linux/kernel/git/peterz/queue.git core/guards2

(core/guards also exists, but that is a previous version that I kept
because I wanted to discuss some 'interesting' clang build fails)

> 
> ---
>  Makefile                            |    6 +-
>  arch/arm64/kernel/vdso32/Makefile   |    2 -
>  drivers/dma/ioat/dma.c              |   12 +-
>  include/linux/cleanup.h             |  167 ++++
>  include/linux/compiler-clang.h      |    9 +
>  include/linux/compiler_attributes.h |    6 +
>  include/linux/cpu.h                 |    2 +
>  include/linux/device.h              |    7 +
>  include/linux/file.h                |   11 +
>  include/linux/irqflags.h            |    7 +
>  include/linux/mutex.h               |    4 +
>  include/linux/percpu.h              |    4 +
>  include/linux/perf_event.h          |   14 +-
>  include/linux/preempt.h             |    5 +
>  include/linux/rcupdate.h            |    3 +
>  include/linux/rwsem.h               |    8 +
>  include/linux/sched/task.h          |    2 +
>  include/linux/slab.h                |    3 +
>  include/linux/spinlock.h            |   31 +
>  include/linux/srcu.h                |    5 +
>  kernel/events/core.c                | 1642 +++++++++++++++--------------------
>  kernel/sched/core.c                 |  931 +++++++++-----------
>  kernel/sched/sched.h                |   40 +
>  scripts/checkpatch.pl               |    2 +-
>  security/apparmor/include/lib.h     |    6 +-
>  25 files changed, 1433 insertions(+), 1496 deletions(-)
>
Linus Torvalds June 12, 2023, 4:37 p.m. UTC | #2 
On Mon, Jun 12, 2023 at 2:39 AM Peter Zijlstra <peterz@infradead.org> wrote:
>
> These are by no means 'complete' convertions, I've mostly focussed on functions
> that had 'goto' in them. Since that's a large part of why I started on all this.

So I found what looks like two patches being completely broken because
the conversion was fundamentally misguided.

And honestly, by the end of walking through the patches I was just
scanning so quickly that I might have missed some.

Let's make the rule be that

 - max 10 patches in the series so that I think they were actually
thought about, and people can actually review them

 - only clear "goto out of scope" conversions. When you see a
"continue" or a "goto repeat", you TAKE A BIG STEP BACK.

Because this is all clever, and I am now happy with the syntax, but I
am *not* happy with that 60-patch monster series with what looks like
horrible bugs.

             Linus