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

[Ingo Molnar]

* 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