Re: [PATCH v4 00/14] liblockdep fixes for 5.9-rc1

From: Ingo Molnar
Date: Thu Aug 06 2020 - 04:10:27 EST



* Sasha Levin <sashal@xxxxxxxxxx> wrote:

> Hi Linus,
>
> Please consider applying these patches for liblockdep, or alternatively
> pull from:
>
> git://git.kernel.org/pub/scm/linux/kernel/git/sashal/linux.git tags/liblockdep-fixes-040820
>
> The patches fix up compilation and functionality of liblockdep on 5.8,
> they were tested using liblockdep's internal testsuite.
>
> I was unable to get the x86 folks to pull these fixes for the past few
> months:

So the primary reason I didn't pull is that liblockdep was permanently
build-broken from February 2019 to around February 2020, despite me
pinging you multiple times about it.

> - https://lkml.org/lkml/2020/2/17/1089

This pull request still said that if fixes "most of" liblockdep, not
"all of", which is the benchmark really after such a long series of
breakage.

> - https://lkml.org/lkml/2020/4/18/817

This still said "most of".

> - https://lkml.org/lkml/2020/6/22/1262

Same 'most of' verbiage.

> Which is why this pull request ends up going straight to you.

So at this point I think we need to ask whether it's worth it: are
there any actual users of liblockdep, besides the testcases in
liblockdep itself? I see there's a 'liblockdep-dev' package for
Debian, but not propagated to Ubuntu or other popular variants AFAICS.

Also, could you please specify whether all bugs are fixed or just
'most'?

> Sasha Levin (14):
> tools headers: Add kprobes.h header
> tools headers: Add rcupdate.h header
> tools/kernel.h: extend with dummy RCU functions
> tools bitmap: add bitmap_andnot definition
> tools/lib/lockdep: add definition required for IRQ flag tracing
> tools bitmap: add bitmap_clear definition
> tools/lib/lockdep: Hook up vsprintf, find_bit, hweight libraries
> tools/lib/lockdep: Enable building with CONFIG_TRACE_IRQFLAGS
> tools/lib/lockdep: New stacktrace API
> tools/lib/lockdep: call lockdep_init_task on init
> tools/lib/lockdep: switch to using lockdep_init_map_waits
> tools/kernel.h: hide noinstr
> tools/lib/lockdep: explicitly declare lockdep_init_task()
> tools/kernel.h: hide task_struct.hardirq_chain_key

Style nits, please use consistent titles for patches:

- First word should be capitalized consistently, instead of mismash
of lower case mixed with upper case.

- First word should preferably be a verb, i.e. "Add new stacktrace
API stubs", not "New stacktrace API"

Also, please always check linux-next whether there's some new upstream
changes that liblockdep needs to adapt to. Right now there's a new
build breakage even with all your fixes applied:

thule:~/tip/tools/lib/lockdep> make
CC common.o
In file included from ../../include/linux/lockdep.h:24,
from common.c:5:
../../include/linux/../../../include/linux/lockdep.h:13:10: fatal error: linux/lockdep_types.h: No such file or directory
13 | #include <linux/lockdep_types.h>
| ^~~~~~~~~~~~~~~~~~~~~~~

At which point we need to step back and analyze the development model:
this comparatively high rate of breakage derives from the unorthodox
direct coupling of a kernel subsystem to a user-space library.

The solution for that would be to use the method how perf syncs to
kernel space headers, by maintaining a 100% copy in tools/include/ and
having automatic mechanism that warns about out of sync headers but
doesn't break functionality.

See tools/perf/check-headers.sh for details.

I believe this same half-automated sync-on-upstream-changes model
could be used for liblockdep as well, i.e. lets copy kernel/lockdep.c
and lockdep*.h over to tools/lib/lockdep/, and reuse the perf header
syncing method to keep it synchronized from that point on.

That would result in a far more maintainable liblockdep end result
IMO?

Thanks,

Ingo