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(-)

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

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(-)
>

Hi Peter,

I greatly admire the Scope-based Resource Management infrastructure 
you introduced to the Linux kernel, as it elegantly aligns with the 
Resource Acquisition Is Initialization (RAII) programming idiom, 
improving code safety and maintainability.

I am interested in driving a comprehensive conversion of traditional 
manual lock/unlock patterns to use guard/scoped_guard,
starting with the sched module. 

Before proceeding, I’d like to confirm if you believe 
this effort is valuable and whether you’d support such a conversion.

Best,
Jemmy