Oleg,

I've been looking at arch/Kconfig and kernel/trace/Kconfig where they deal with uprobes. The relevant items are CONFIG_UPROBES and CONFIG_UPROBE_EVENT. It just doesn't look right to me. It looks like "select" is used in part maybe just to avoid the recursive dependency error that would be generated if "depends on" were used in both places. However I don't think UPROBES should be dependent on UPROBE_EVENT, only the other way around. As RK noted in previous email (copied in part below) the select does not pull in the lower level dependencies. This all works on x86 only because arch/x86/Kconfig defines CONFIG_PERF_EVENT (which feels like a big hammer). We don't need to do this on ARM, and we don't do it. The result is that, unless PERF_EVENT is set separately, uprobes tends not to build. I was lucking-out in my testing due to other default config items turning on PERF_EVENT.

What do you think about the following patch?:

diff --git a/arch/Kconfig b/arch/Kconfig
index 80bbb8c..8793066 100644
--- a/arch/Kconfig
+++ b/arch/Kconfig
@@ -87,7 +87,8 @@ config KPROBES_ON_FTRACE

 config UPROBES
        bool "Transparent user-space probes (EXPERIMENTAL)"
-       depends on UPROBE_EVENT && PERF_EVENTS
+       depends on ARCH_SUPPORTS_UPROBES
+       depends on PERF_EVENTS
        default n
        select PERCPU_RWSEM
        help
diff --git a/kernel/trace/Kconfig b/kernel/trace/Kconfig
index 015f85a..7d2647e 100644
--- a/kernel/trace/Kconfig
+++ b/kernel/trace/Kconfig
@@ -422,9 +422,8 @@ config KPROBE_EVENT

 config UPROBE_EVENT
        bool "Enable uprobes-based dynamic events"
-       depends on ARCH_SUPPORTS_UPROBES
        depends on MMU
-       select UPROBES
+       depends on UPROBES
        select PROBE_EVENTS
        select TRACING
        default n


This removes the pseudo-recursive dependency. The downside is that now you have to set PERF_EVENTS to use UPROBES, and UPROBES to use UPROBE_EVENT, whereas before on x86 you only had to set UPROBE_EVENT (as long as PERF_EVENTS stays hardcoded on). I think this could be avoided by making each "depends on" a "selects", since they apply transitively. I'm sensing this would be an unpopular idea though.

-dl



On 03/01/14 07:30, Russell King - ARM Linux wrote:

[ ... ]

Clearly, it doesn't make sense for UPROBES to be enabled with PERF_EVENTS
disabled - and indeed the Kconfig ensures that this dependency is properly
expressed:

        config UPROBES
                bool "Transparent user-space probes (EXPERIMENTAL)"
                depends on UPROBE_EVENT && PERF_EVENTS
                default n
                select PERCPU_RWSEM

but where this all falls down is here:

        config UPROBE_EVENT
                bool "Enable uprobes-based dynamic events"
                depends on ARCH_SUPPORTS_UPROBES
                depends on MMU
                select UPROBES
                select PROBE_EVENTS
                select TRACING
                default n

Which is yet another brilliant example of why this "select" crap is soo
evil.  Yes, the failing configuration has:

        CONFIG_UPROBE_EVENT=y

Ineed, there was a Kconfig warning:

        warning: (UPROBE_EVENT) selects UPROBES which has unmet direct dependencies 
(UPROBE_EVENT && PERF_EVENTS)

This is not your fault.  It's the fault of everyone who passed through
commit f3f096cfedf8113380c56fc855275cc75cd8cf55 without properly reviewing
it and paying attention to that select crap.  Given how evil "select" is,
it's something which should always be thoroughly reviewed - with analysis
of the dependencies.  I believe commits which introduce new select
statements should document an analysis of why those new select statements
are appropriate and how they ensure that any dependencies of the selected
symbol are not violated.

Therefore, I will not take the ARM uprobes code while this kind of
select abortion is present - it needs to be fixed first to avoid these
build errors.  Sorry.


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Reply via email to