* Re: [PATCH bpf-next v3 3/6] bpf: fix the usage of BPF_TRAMP_F_SKIP_FRAME
From: Menglong Dong @ 2025-12-19 2:22 UTC (permalink / raw)
To: Menglong Dong, Andreas Schwab
Cc: ast, rostedt, daniel, john.fastabend, andrii, martin.lau, eddyz87,
song, yonghong.song, kpsingh, sdf, haoluo, jolsa, mhiramat,
mark.rutland, mathieu.desnoyers, jiang.biao, bpf, linux-kernel,
linux-trace-kernel, linux-riscv
In-Reply-To: <874ipnkfvt.fsf@igel.home>
On 2025/12/18 22:45 Andreas Schwab <schwab@linux-m68k.org> write:
> On Nov 18 2025, Menglong Dong wrote:
>
> > Some places calculate the origin_call by checking if
> > BPF_TRAMP_F_SKIP_FRAME is set. However, it should use
> > BPF_TRAMP_F_ORIG_STACK for this propose. Just fix them.
> >
> > Signed-off-by: Menglong Dong <dongml2@chinatelecom.cn>
> > Acked-by: Alexei Starovoitov <ast@kernel.org>
>
> This breaks RISC-V:
Hi, Andreas. Can you offer more information here? After my analysis,
I didn't see the problem. BPF_TRAMP_F_SKIP_FRAME and
BPF_TRAMP_F_ORIG_STACK are set together all the time in RISC-V, so
I changed BPF_TRAMP_F_SKIP_FRAME to BPF_TRAMP_F_ORIG_STACK
*should* have no influence.
Thanks!
Menglong Dong
>
> [ 8.584381][ T1] systemd[1]: bpf-restrict-fs: LSM BPF program attached
> [ 8.588359][ T1] Insufficient stack space to handle exception!
> [ 8.588823][ T1] Task stack: [0xff20000000010000..0xff20000000014000]
> [ 8.589219][ T1] Overflow stack: [0xff600000ffdad070..0xff600000ffdae070]
> [ 8.590133][ T1] CPU: 1 UID: 0 PID: 1 Comm: systemd Not tainted 6.18.0-rc5+ #15 PREEMPT(voluntary) c900881ed1c1988ec5cf3e914d0edeb1b4d83ca3
> [ 8.590898][ T1] Hardware name: riscv-virtio qemu/qemu, BIOS 2025.10 10/01/2025
> [ 8.591494][ T1] epc : copy_from_kernel_nofault+0xa/0x198
> [ 8.592292][ T1] ra : bpf_probe_read_kernel+0x20/0x60
> [ 8.592658][ T1] epc : ffffffff802b732a ra : ffffffff801e6070 sp : ff2000000000ffe0
> [ 8.593121][ T1] gp : ffffffff82262ed0 tp : 0000000000000000 t0 : ffffffff80022320
> [ 8.593566][ T1] t1 : ffffffff801e6056 t2 : 0000000000000000 s0 : ff20000000010040
> [ 8.593997][ T1] s1 : 0000000000000008 a0 : ff20000000010050 a1 : ff60000083b3d320
> [ 8.594446][ T1] a2 : 0000000000000008 a3 : 0000000000000097 a4 : 0000000000000000
> [ 8.594940][ T1] a5 : 0000000000000000 a6 : 0000000000000021 a7 : 0000000000000003
> [ 8.595396][ T1] s2 : ff20000000010050 s3 : ff6000008459fc18 s4 : ff60000083b3d340
> [ 8.595831][ T1] s5 : ff20000000010060 s6 : 0000000000000000 s7 : ff20000000013aa8
> [ 8.596215][ T1] s8 : 0000000000000000 s9 : 0000000000008000 s10: 000000000058dcb0
> [ 8.596641][ T1] s11: 000000000058dca7 t3 : 000000006925116d t4 : ff6000008090f026
> [ 8.597065][ T1] t5 : 00007fff9b0cbaa8 t6 : 0000000000000016
> [ 8.597363][ T1] status: 0000000200000120 badaddr: 0000000000000000 cause: 8000000000000005
> [ 8.598033][ T1] Kernel panic - not syncing: Kernel stack overflow
> [ 8.598597][ T1] CPU: 1 UID: 0 PID: 1 Comm: systemd Not tainted 6.18.0-rc5+ #15 PREEMPT(voluntary) c900881ed1c1988ec5cf3e914d0edeb1b4d83ca3
> [ 8.599244][ T1] Hardware name: riscv-virtio qemu/qemu, BIOS 2025.10 10/01/2025
> [ 8.599659][ T1] Call Trace:
> [ 8.600117][ T1] [<ffffffff8001a1f8>] dump_backtrace+0x28/0x38
> [ 8.600517][ T1] [<ffffffff80002502>] show_stack+0x3a/0x50
> [ 8.600844][ T1] [<ffffffff800122be>] dump_stack_lvl+0x56/0x80
> [ 8.601176][ T1] [<ffffffff80012300>] dump_stack+0x18/0x22
> [ 8.601518][ T1] [<ffffffff80002abe>] vpanic+0xf6/0x328
> [ 8.601819][ T1] [<ffffffff80002d2e>] panic+0x3e/0x40
> [ 8.602088][ T1] [<ffffffff80019ef0>] handle_bad_stack+0x98/0xa0
> [ 8.602395][ T1] [<ffffffff801e6070>] bpf_probe_read_kernel+0x20/0x60
>
> --
> Andreas Schwab, schwab@linux-m68k.org
> GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510 2552 DF73 E780 A9DA AEC1
> "And now for something completely different."
>
>
^ permalink raw reply
* Re: [PATCH 2/2] bpf: Implement kretprobe fallback for kprobe multi link
From: Menglong Dong @ 2025-12-19 1:57 UTC (permalink / raw)
To: ast, daniel, andrii, mhiramat, martin.lau, eddyz87, song,
yonghong.song, john.fastabend, kpsingh, sdf, haoluo, jolsa,
liujing40
Cc: bpf, linux-kernel, linux-trace-kernel, liujing40
In-Reply-To: <20251218130629.365398-3-liujing40@xiaomi.com>
On 2025/12/18 21:06 liujing40 <liujing.root@gmail.com> write:
> When fprobe is not available, provide a fallback implementation of
> kprobe_multi using the traditional kretprobe API.
>
> Uses kretprobe's entry_handler and handler callbacks to simulate fprobe's
> entry/exit functionality.
>
> Signed-off-by: Jing Liu <liujing40@xiaomi.com>
> ---
> kernel/trace/bpf_trace.c | 307 +++++++++++++++++++++++++++++++++++++--
> 1 file changed, 295 insertions(+), 12 deletions(-)
>
> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index 1fd07c10378f..426a1c627508 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -2274,12 +2274,44 @@ struct bpf_session_run_ctx {
> void *data;
> };
>
> -#ifdef CONFIG_FPROBE
> +#if defined(CONFIG_FPROBE) || defined(CONFIG_KRETPROBES)
> +#ifndef CONFIG_FPROBE
> +struct bpf_kprobe {
> + struct bpf_kprobe_multi_link *link;
> + u64 cookie;
> + struct kretprobe rp;
> +};
> +
> +static void bpf_kprobe_unregister(struct bpf_kprobe *kps, u32 cnt)
> +{
> + for (int i = 0; i < cnt; i++)
> + unregister_kretprobe(&kps[i].rp);
> +}
> +
> +static int bpf_kprobe_register(struct bpf_kprobe *kps, u32 cnt)
> +{
> + int ret = 0, i;
> +
> + for (i = 0; i < cnt; i++) {
> + ret = register_kretprobe(&kps[i].rp);
> + if (ret < 0) {
> + bpf_kprobe_unregister(kps, i);
> + break;
> + }
> + }
> + return ret;
> +}
Hi, Jing. I don't see the point of the fallback logic. If we want to
use the kprobe-multi, we enable CONFIG_FPROBE. Is there any reason
that we can't enable CONFIG_FPROBE? As you said, for a "old kernel",
I think we don't introduce new feature for an old kernel.
Besides, did you measure the performance of attaching bench?
You will register a kretprobe for each of the target. AFAIK, the
kprobe will use ftrace for optimization if we hook the entry of the
target function. So I suspect it will be quite slow here.
Thanks!
Menglong Dong
> +#endif
> +
> struct bpf_kprobe_multi_link {
> struct bpf_link link;
> +#ifdef CONFIG_FPROBE
> struct fprobe fp;
> unsigned long *addrs;
> u64 *cookies;
> +#else
> + struct bpf_kprobe *kprobes;
> +#endif
> u32 cnt;
> u32 mods_cnt;
> struct module **mods;
> @@ -2287,7 +2319,11 @@ struct bpf_kprobe_multi_link {
>
> struct bpf_kprobe_multi_run_ctx {
> struct bpf_session_run_ctx session_ctx;
> +#ifdef CONFIG_FPROBE
> struct bpf_kprobe_multi_link *link;
> +#else
> + struct bpf_kprobe *kprobe;
> +#endif
> unsigned long entry_ip;
> };
>
> @@ -2304,7 +2340,11 @@ static void bpf_kprobe_multi_link_release(struct bpf_link *link)
> struct bpf_kprobe_multi_link *kmulti_link;
>
> kmulti_link = container_of(link, struct bpf_kprobe_multi_link, link);
> +#ifdef CONFIG_FPROBE
> unregister_fprobe(&kmulti_link->fp);
> +#else
> + bpf_kprobe_unregister(kmulti_link->kprobes, kmulti_link->cnt);
> +#endif
> kprobe_multi_put_modules(kmulti_link->mods, kmulti_link->mods_cnt);
> }
>
> @@ -2313,8 +2353,12 @@ static void bpf_kprobe_multi_link_dealloc(struct bpf_link *link)
> struct bpf_kprobe_multi_link *kmulti_link;
>
> kmulti_link = container_of(link, struct bpf_kprobe_multi_link, link);
> +#ifdef CONFIG_FPROBE
> kvfree(kmulti_link->addrs);
> kvfree(kmulti_link->cookies);
> +#else
> + kvfree(kmulti_link->kprobes);
> +#endif
> kfree(kmulti_link->mods);
> kfree(kmulti_link);
> }
> @@ -2326,6 +2370,7 @@ static int bpf_kprobe_multi_link_fill_link_info(const struct bpf_link *link,
> u64 __user *uaddrs = u64_to_user_ptr(info->kprobe_multi.addrs);
> struct bpf_kprobe_multi_link *kmulti_link;
> u32 ucount = info->kprobe_multi.count;
> + bool kallsyms_show = kallsyms_show_value(current_cred());
> int err = 0, i;
>
> if (!uaddrs ^ !ucount)
> @@ -2336,7 +2381,12 @@ static int bpf_kprobe_multi_link_fill_link_info(const struct bpf_link *link,
> kmulti_link = container_of(link, struct bpf_kprobe_multi_link, link);
> info->kprobe_multi.count = kmulti_link->cnt;
> info->kprobe_multi.flags = kmulti_link->link.flags;
> +#ifdef CONFIG_FPROBE
> info->kprobe_multi.missed = kmulti_link->fp.nmissed;
> +#else
> + for (i = 0; i < kmulti_link->cnt; i++)
> + info->kprobe_multi.missed += kmulti_link->kprobes[i].rp.nmissed;
> +#endif
>
> if (!uaddrs)
> return 0;
> @@ -2345,6 +2395,7 @@ static int bpf_kprobe_multi_link_fill_link_info(const struct bpf_link *link,
> else
> ucount = kmulti_link->cnt;
>
> +#ifdef CONFIG_FPROBE
> if (ucookies) {
> if (kmulti_link->cookies) {
> if (copy_to_user(ucookies, kmulti_link->cookies, ucount * sizeof(u64)))
> @@ -2357,7 +2408,7 @@ static int bpf_kprobe_multi_link_fill_link_info(const struct bpf_link *link,
> }
> }
>
> - if (kallsyms_show_value(current_cred())) {
> + if (kallsyms_show) {
> if (copy_to_user(uaddrs, kmulti_link->addrs, ucount * sizeof(u64)))
> return -EFAULT;
> } else {
> @@ -2366,6 +2417,16 @@ static int bpf_kprobe_multi_link_fill_link_info(const struct bpf_link *link,
> return -EFAULT;
> }
> }
> +#else
> + for (i = 0; i < ucount; i++) {
> + if (ucookies && put_user(kmulti_link->kprobes[i].cookie, ucookies + i))
> + return -EFAULT;
> +
> + if (put_user(kallsyms_show ? (uintptr_t)kmulti_link->kprobes[i].rp.kp.addr : 0,
> + uaddrs + i))
> + return -EFAULT;
> + }
> +#endif
> return err;
> }
>
> @@ -2374,21 +2435,32 @@ static void bpf_kprobe_multi_show_fdinfo(const struct bpf_link *link,
> struct seq_file *seq)
> {
> struct bpf_kprobe_multi_link *kmulti_link;
> + unsigned long kprobe_multi_missed = 0;
>
> kmulti_link = container_of(link, struct bpf_kprobe_multi_link, link);
> +#ifdef CONFIG_FPROBE
> + kprobe_multi_missed = kmulti_link->fp.nmissed;
> +#else
> + for (int i = 0; i < kmulti_link->cnt; i++)
> + kprobe_multi_missed += kmulti_link->kprobes[i].rp.nmissed;
> +#endif
>
> seq_printf(seq,
> "kprobe_cnt:\t%u\n"
> "missed:\t%lu\n",
> kmulti_link->cnt,
> - kmulti_link->fp.nmissed);
> + kprobe_multi_missed);
>
> seq_printf(seq, "%s\t %s\n", "cookie", "func");
> for (int i = 0; i < kmulti_link->cnt; i++) {
> - seq_printf(seq,
> - "%llu\t %pS\n",
> - kmulti_link->cookies[i],
> - (void *)kmulti_link->addrs[i]);
> +#ifdef CONFIG_FPROBE
> + u64 cookie = kmulti_link->cookies[i];
> + void *addr = (void *)kmulti_link->addrs[i];
> +#else
> + u64 cookie = kmulti_link->kprobes[i].cookie;
> + void *addr = (void *)kmulti_link->kprobes[i].rp.kp.addr;
> +#endif
> + seq_printf(seq, "%llu\t %pS\n", cookie, addr);
> }
> }
> #endif
> @@ -2445,17 +2517,22 @@ static bool has_module(struct modules_array *arr, struct module *mod)
> return false;
> }
>
> -static int get_modules_for_addrs(struct module ***mods, unsigned long *addrs, u32 addrs_cnt)
> +static int get_modules_for_addrs(struct bpf_kprobe_multi_link *link)
> {
> struct modules_array arr = {};
> u32 i, err = 0;
>
> - for (i = 0; i < addrs_cnt; i++) {
> + for (i = 0; i < link->cnt; i++) {
> bool skip_add = false;
> struct module *mod;
> +#ifdef CONFIG_FPROBE
> + unsigned long addr = link->addrs[i];
> +#else
> + unsigned long addr = (unsigned long)link->kprobes[i].rp.kp.addr;
> +#endif
>
> scoped_guard(rcu) {
> - mod = __module_address(addrs[i]);
> + mod = __module_address(addr);
> /* Either no module or it's already stored */
> if (!mod || has_module(&arr, mod)) {
> skip_add = true;
> @@ -2483,10 +2560,11 @@ static int get_modules_for_addrs(struct module ***mods, unsigned long *addrs, u3
> }
>
> /* or number of modules found if everything is ok. */
> - *mods = arr.mods;
> + link->mods = arr.mods;
> return arr.mods_cnt;
> }
>
> +#ifdef CONFIG_FPROBE
> struct user_syms {
> const char **syms;
> char *buf;
> @@ -2843,7 +2921,7 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
> link);
> }
>
> - err = get_modules_for_addrs(&link->mods, addrs, cnt);
> + err = get_modules_for_addrs(link);
> if (err < 0) {
> bpf_link_cleanup(&link_primer);
> return err;
> @@ -2866,6 +2944,211 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
> return err;
> }
> #else /* !CONFIG_FPROBE */
> +static u64 bpf_kprobe_multi_cookie(struct bpf_run_ctx *ctx)
> +{
> + struct bpf_kprobe_multi_run_ctx *run_ctx;
> +
> + run_ctx = container_of(current->bpf_ctx, struct bpf_kprobe_multi_run_ctx,
> + session_ctx.run_ctx);
> + return run_ctx->kprobe->cookie;
> +}
> +
> +static __always_inline int
> +kprobe_multi_link_prog_run(struct bpf_kprobe *kprobe, unsigned long entry_ip,
> + struct pt_regs *regs, bool is_return, void *data)
> +{
> + struct bpf_kprobe_multi_link *link = kprobe->link;
> + struct bpf_kprobe_multi_run_ctx run_ctx = {
> + .session_ctx = {
> + .is_return = is_return,
> + .data = data,
> + },
> + .kprobe = kprobe,
> + .entry_ip = entry_ip,
> + };
> + struct bpf_run_ctx *old_run_ctx;
> + int err;
> +
> + cant_sleep();
> +
> + if (unlikely(__this_cpu_inc_return(bpf_prog_active) != 1)) {
> + bpf_prog_inc_misses_counter(link->link.prog);
> + err = 1;
> + goto out;
> + }
> +
> + rcu_read_lock();
> + migrate_disable();
> + old_run_ctx = bpf_set_run_ctx(&run_ctx.session_ctx.run_ctx);
> + err = bpf_prog_run(link->link.prog, regs);
> + bpf_reset_run_ctx(old_run_ctx);
> + migrate_enable();
> + rcu_read_unlock();
> +
> + out:
> + __this_cpu_dec(bpf_prog_active);
> + return err;
> +}
> +
> +static int
> +kprobe_multi_link_handler(struct kretprobe_instance *ri, struct pt_regs *regs)
> +{
> + struct kretprobe *rp = get_kretprobe(ri);
> + struct bpf_kprobe *kprobe;
> + int err;
> +
> + if (unlikely(!rp))
> + return 1;
> +
> + kprobe = container_of(rp, struct bpf_kprobe, rp);
> + err = kprobe_multi_link_prog_run(kprobe, get_entry_ip((uintptr_t)rp->kp.addr),
> + regs, false, ri->data);
> + return is_kprobe_session(kprobe->link->link.prog) ? err : 0;
> +}
> +
> +static int
> +kprobe_multi_link_exit_handler(struct kretprobe_instance *ri, struct pt_regs *regs)
> +{
> + struct kretprobe *rp = get_kretprobe(ri);
> + struct bpf_kprobe *kprobe;
> +
> + if (unlikely(!rp))
> + return 0;
> +
> + kprobe = container_of(rp, struct bpf_kprobe, rp);
> + kprobe_multi_link_prog_run(kprobe, get_entry_ip((uintptr_t)rp->kp.addr),
> + regs, true, ri->data);
> + return 0;
> +}
> +
> +int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *prog)
> +{
> + struct bpf_kprobe_multi_link *link = NULL;
> + struct bpf_link_primer link_primer;
> + struct bpf_kprobe *kprobes = NULL;
> + u32 flags, cnt;
> + u64 __user *ucookies;
> + unsigned long __user *uaddrs;
> + unsigned long __user *usyms;
> + int err, i;
> +
> + /* no support for 32bit archs yet */
> + if (sizeof(u64) != sizeof(void *))
> + return -EOPNOTSUPP;
> +
> + if (attr->link_create.flags)
> + return -EINVAL;
> +
> + if (!is_kprobe_multi(prog))
> + return -EINVAL;
> +
> + /* Writing to context is not allowed for kprobes. */
> + if (prog->aux->kprobe_write_ctx)
> + return -EINVAL;
> +
> + flags = attr->link_create.kprobe_multi.flags;
> + if (flags & ~BPF_F_KPROBE_MULTI_RETURN)
> + return -EINVAL;
> +
> + uaddrs = u64_to_user_ptr(attr->link_create.kprobe_multi.addrs);
> + usyms = u64_to_user_ptr(attr->link_create.kprobe_multi.syms);
> + if (!!uaddrs == !!usyms)
> + return -EINVAL;
> +
> + cnt = attr->link_create.kprobe_multi.cnt;
> + if (!cnt)
> + return -EINVAL;
> + if (cnt > MAX_KPROBE_MULTI_CNT)
> + return -E2BIG;
> +
> + ucookies = u64_to_user_ptr(attr->link_create.kprobe_multi.cookies);
> + kprobes = kvcalloc(cnt, sizeof(*kprobes), GFP_KERNEL);
> + link = kzalloc(sizeof(*link), GFP_KERNEL);
> + if (!link || !kprobes) {
> + err = -ENOMEM;
> + goto error;
> + }
> +
> + for (i = 0; i < cnt; i++) {
> + unsigned long addr;
> +
> + if (uaddrs) {
> + if (__get_user(addr, uaddrs + i)) {
> + err = -EFAULT;
> + goto error;
> + }
> + } else {
> + unsigned long __user usymbol;
> + char buf[KSYM_NAME_LEN];
> +
> + if (__get_user(usymbol, usyms + i)) {
> + err = -EFAULT;
> + goto error;
> + }
> + err = strncpy_from_user(buf, (const char __user *) usymbol, KSYM_NAME_LEN);
> + if (err == KSYM_NAME_LEN)
> + err = -E2BIG;
> + if (err < 0)
> + goto error;
> +
> + addr = kallsyms_lookup_name(buf);
> + if (!addr)
> + goto error;
> + }
> + if (prog->kprobe_override && !within_error_injection_list(addr)) {
> + err = -EINVAL;
> + goto error;
> + }
> + if (ucookies && __get_user(kprobes[i].cookie, ucookies + i)) {
> + err = -EFAULT;
> + goto error;
> + }
> +
> + kprobes[i].link = link;
> + kprobes[i].rp.kp.addr = (kprobe_opcode_t *)addr;
> +
> + if (!(flags & BPF_F_KPROBE_MULTI_RETURN))
> + kprobes[i].rp.entry_handler = kprobe_multi_link_handler;
> + if ((flags & BPF_F_KPROBE_MULTI_RETURN) || is_kprobe_session(prog))
> + kprobes[i].rp.handler = kprobe_multi_link_exit_handler;
> + if (is_kprobe_session(prog))
> + kprobes[i].rp.data_size = sizeof(u64);
> + }
> +
> + bpf_link_init(&link->link, BPF_LINK_TYPE_KPROBE_MULTI,
> + &bpf_kprobe_multi_link_lops, prog, attr->link_create.attach_type);
> +
> + err = bpf_link_prime(&link->link, &link_primer);
> + if (err)
> + goto error;
> +
> + link->kprobes = kprobes;
> + link->cnt = cnt;
> + link->link.flags = flags;
> +
> + err = get_modules_for_addrs(link);
> + if (err < 0) {
> + bpf_link_cleanup(&link_primer);
> + return err;
> + }
> + link->mods_cnt = err;
> +
> + err = bpf_kprobe_register(kprobes, cnt);
> + if (err) {
> + kprobe_multi_put_modules(link->mods, link->mods_cnt);
> + bpf_link_cleanup(&link_primer);
> + return err;
> + }
> +
> + return bpf_link_settle(&link_primer);
> +
> +error:
> + kvfree(kprobes);
> + kfree(link);
> + return err;
> +}
> +#endif
> +#else /* !CONFIG_FPROBE && !CONFIG_KRETPROBES*/
> int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *prog)
> {
> return -EOPNOTSUPP;
> --
> 2.25.1
>
>
>
^ permalink raw reply
* Re: [PATCH v3 4/4] tracing: move tracing declarations from kernel.h to a dedicated header
From: Yury Norov @ 2025-12-18 21:53 UTC (permalink / raw)
To: Steven Rostedt
Cc: Randy Dunlap, Andrew Morton, Masami Hiramatsu, Mathieu Desnoyers,
Andy Shevchenko, Christophe Leroy, Ingo Molnar, Jani Nikula,
Joonas Lahtinen, David Laight, Petr Pavlu, Andi Shyti,
Rodrigo Vivi, Tvrtko Ursulin, Daniel Gomez, Greg Kroah-Hartman,
Rafael J. Wysocki, Danilo Krummrich, linux-kernel, intel-gfx,
dri-devel, linux-modules, linux-trace-kernel, Kees Cook
In-Reply-To: <20251218164103.3c535de3@gandalf.local.home>
On Thu, Dec 18, 2025 at 04:41:03PM -0500, Steven Rostedt wrote:
> On Thu, 18 Dec 2025 16:25:42 -0500
> Steven Rostedt <rostedt@goodmis.org> wrote:
>
> > On Thu, 18 Dec 2025 15:33:47 -0500
> > Yury Norov <yury.norov@gmail.com> wrote:
> >
> > > > I don't actually remember why I had __trace_puts() pass in the size. I
> > > > could change it to:
> > >
> > > This is the best approach. I'll schedule it for v4. Would you like me to
> > > take it as-is, or you'd send a patch?
> > >
> >
> > Let me send an official patch.
> >
>
> You can find it here (I Cc'd you too). Feel free to add it to your patch set.
>
> https://lore.kernel.org/all/20251218163739.5605f9ea@gandalf.local.home/
Thanks, will do.
^ permalink raw reply
* Re: [PATCH] tracing: Remove size parameter in __trace_puts()
From: Yury Norov @ 2025-12-18 21:56 UTC (permalink / raw)
To: Steven Rostedt
Cc: LKML, Linux Trace Kernel, Masami Hiramatsu, Mathieu Desnoyers,
Randy Dunlap
In-Reply-To: <20251218163739.5605f9ea@gandalf.local.home>
On Thu, Dec 18, 2025 at 04:37:39PM -0500, Steven Rostedt wrote:
> From: Steven Rostedt <rostedt@goodmis.org>
>
> The __trace_puts() function takes a string pointer and the size of the
> string itself. All users currently simply pass in the strlen() of the
> string it is also passing in. There's no reason to pass in the size.
> Instead have the __trace_puts() function do the strlen() within the
> function itself.
>
> This fixes a header recursion issue where using strlen() in the macro
> calling __trace_puts() requires adding #include <linux/string.h> in order
> to use strlen(). Removing the use of strlen() from the header fixes the
> recursion issue.
>
> Link: https://lore.kernel.org/all/aUN8Hm377C5A0ILX@yury/
>
> Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
Thanks, Steven. I'll take it with v4 of the above.
Thanks,
Yury
^ permalink raw reply
* Re: [PATCH] selftests/ftrace: Add test dependency
From: Steven Rostedt @ 2025-12-18 23:32 UTC (permalink / raw)
To: Thibault Ferrante, Masami Hiramatsu
Cc: shuah, linux-kernel, linux-trace-kernel, linux-kselftest
In-Reply-To: <20230614105031.13543-1-thibault.ferrante@canonical.com>
On Wed, 14 Jun 2023 12:50:31 +0200
Thibault Ferrante <thibault.ferrante@canonical.com> wrote:
> test_duplicates miss a running dependency and leads to test failures
> on kernel with specific configuration.
>
Found another old patch. But this one still needs review.
Masami, does this look good to you?
-- Steve
> Signed-off-by: Thibault Ferrante <thibault.ferrante@canonical.com>
> ---
> .../testing/selftests/ftrace/test.d/dynevent/test_duplicates.tc | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tools/testing/selftests/ftrace/test.d/dynevent/test_duplicates.tc b/tools/testing/selftests/ftrace/test.d/dynevent/test_duplicates.tc
> index d3a79da215c8..0b5e4543e70b 100644
> --- a/tools/testing/selftests/ftrace/test.d/dynevent/test_duplicates.tc
> +++ b/tools/testing/selftests/ftrace/test.d/dynevent/test_duplicates.tc
> @@ -1,7 +1,7 @@
> #!/bin/sh
> # SPDX-License-Identifier: GPL-2.0
> # description: Generic dynamic event - check if duplicate events are caught
> -# requires: dynamic_events "e[:[<group>/][<event>]] <attached-group>.<attached-event> [<args>]":README
> +# requires: dynamic_events events/syscalls/sys_enter_openat "e[:[<group>/][<event>]] <attached-group>.<attached-event> [<args>]":README
>
> echo 0 > events/enable
>
^ permalink raw reply
* Re: [PATCH] selftests/ftrace: Test toplevel-enable for instance
From: Steven Rostedt @ 2025-12-18 23:30 UTC (permalink / raw)
To: Zheng Yejian
Cc: mhiramat, shuah, linux-kernel, linux-trace-kernel,
linux-kselftest, Shuah Khan
In-Reply-To: <20230907214020.34981ffd@gandalf.local.home>
On Thu, 7 Sep 2023 21:40:20 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:
> On Mon, 10 Jul 2023 17:54:36 -0400
> Steven Rostedt <rostedt@goodmis.org> wrote:
>
> > Hi Shuah,
> >
> > I think this dropped through the cracks. Can you take this through your
> > tree?
>
> ping?
Found another one that never got applied.
-- Steve
>
>
> >
> > -- Steve
> >
> > On Fri, 9 Jun 2023 17:46:26 -0400
> > Steven Rostedt <rostedt@goodmis.org> wrote:
> >
> > > On Wed, 10 May 2023 04:36:59 +0800
> > > Zheng Yejian <zhengyejian1@huawei.com> wrote:
> > >
> > > > 'available_events' is actually not required by
> > > > 'test.d/event/toplevel-enable.tc' and its Existence has been tested in
> > > > 'test.d/00basic/basic4.tc'.
> > > >
> > > > So the require of 'available_events' can be dropped and then we can add
> > > > 'instance' flag to test 'test.d/event/toplevel-enable.tc' for instance.
> > > >
> > > > Test result show as below:
> > > > # ./ftracetest test.d/event/toplevel-enable.tc
> > > > === Ftrace unit tests ===
> > > > [1] event tracing - enable/disable with top level files [PASS]
> > > > [2] (instance) event tracing - enable/disable with top level files [PASS]
> > > >
> > > > # of passed: 2
> > > > # of failed: 0
> > > > # of unresolved: 0
> > > > # of untested: 0
> > > > # of unsupported: 0
> > > > # of xfailed: 0
> > > > # of undefined(test bug): 0
> > > >
> > > > Signed-off-by: Zheng Yejian <zhengyejian1@huawei.com>
> > >
> > > Acked-by: Steven Rostedt (Google) <rostedt@goodmis.org>
> > >
> > > -- Steve
> > >
> > > > ---
> > > > tools/testing/selftests/ftrace/test.d/event/toplevel-enable.tc | 3 ++-
> > > > 1 file changed, 2 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/tools/testing/selftests/ftrace/test.d/event/toplevel-enable.tc b/tools/testing/selftests/ftrace/test.d/event/toplevel-enable.tc
> > > > index 93c10ea42a68..8b8e1aea985b 100644
> > > > --- a/tools/testing/selftests/ftrace/test.d/event/toplevel-enable.tc
> > > > +++ b/tools/testing/selftests/ftrace/test.d/event/toplevel-enable.tc
> > > > @@ -1,7 +1,8 @@
> > > > #!/bin/sh
> > > > # SPDX-License-Identifier: GPL-2.0
> > > > # description: event tracing - enable/disable with top level files
> > > > -# requires: available_events set_event events/enable
> > > > +# requires: set_event events/enable
> > > > +# flags: instance
> > > >
> > > > do_reset() {
> > > > echo > set_event
^ permalink raw reply
* Re: [PATCH v3] selftests/ftrace: traceonoff_triggers: strip off names
From: Steven Rostedt @ 2025-12-18 23:27 UTC (permalink / raw)
To: Yipeng Zou
Cc: shuah, mhiramat, linux-trace-kernel, linux-kselftest, Shuah Khan
In-Reply-To: <33503bb2-6248-426b-a614-9f0f92899d70@huawei.com>
On Sat, 13 Apr 2024 10:47:14 +0800
Yipeng Zou <zouyipeng@huawei.com> wrote:
> 在 2023/8/21 23:13, Steven Rostedt 写道:
> > On Fri, 18 Aug 2023 09:32:26 +0800
> > Yipeng Zou <zouyipeng@huawei.com> wrote:
> >
> >> The func_traceonoff_triggers.tc sometimes goes to fail
> >> on my board, Kunpeng-920.
> > Reviewed-by: Steven Rostedt (Google) <rostedt@goodmis.org>
> >
> > Shuah, can you take this through your tree?
> >
> > -- Steve
>
> Hi:
>
> I just notice that it haven't been picked in Linux-next.
>
> It's there other issue about this patch?
Wow, this is a couple of years old (noticed while looking at the archived
patches in patchwork).
Shuah, I believe this is still needed.
-- Steve
^ permalink raw reply
* Re: [PATCH v10 2/3] mm: Fix OOM killer inaccuracy on large many-core systems
From: Mathieu Desnoyers @ 2025-12-18 22:18 UTC (permalink / raw)
To: Mark Brown, Thomas Gleixner
Cc: Andrew Morton, linux-kernel, Paul E. McKenney, Steven Rostedt,
Masami Hiramatsu, Dennis Zhou, Tejun Heo, Christoph Lameter,
Martin Liu, David Rientjes, christian.koenig, Shakeel Butt,
SeongJae Park, Michal Hocko, Johannes Weiner, Sweet Tea Dorminy,
Lorenzo Stoakes, Liam R . Howlett, Mike Rapoport,
Suren Baghdasaryan, Vlastimil Babka, Christian Brauner, Wei Yang,
David Hildenbrand, Miaohe Lin, Al Viro, linux-mm,
linux-trace-kernel, Yu Zhao, Roman Gushchin, Mateusz Guzik,
Matthew Wilcox, Baolin Wang, Aboorva Devarajan, Aishwarya TCV
In-Reply-To: <c8acf650-ef96-4449-ba85-557dedba0ffc@sirena.org.uk>
On 2025-12-18 13:00, Mark Brown wrote:
> On Sat, Dec 13, 2025 at 01:56:07PM -0500, Mathieu Desnoyers wrote:
>
>> Use hierarchical per-cpu counters for rss tracking to fix the per-mm RSS
>> tracking which has become too inaccurate for OOM killer purposes on
>> large many-core systems.
>
> We're seeing boot time crashes in -next on the Arm FVP and Ampere Altra
> which bisect to this patch which is commit 240587b6cca2822d. Many other
> platforms aren't showing this, though we do have some other breakage in
> -next which might be obscuring things. We get a NULL dereference:
>
> [ 2.481143] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000000
>
> ...
>
> [ 2.485036] Call trace:
> [ 2.485094] acct_account_cputime+0x40/0xa4 (P)
> [ 2.485226] irqtime_account_process_tick+0x17c/0x1d8
> [ 2.485382] account_process_tick+0x12c/0x148
> [ 2.485531] update_process_times+0x28/0xdc
> [ 2.485656] tick_nohz_handler+0xbc/0x1bc
> [ 2.485809] __hrtimer_run_queues+0x130/0x184
>
> I note that __acct_update_integrals is being called from here most
> likely inline and doing get_mm_rss(). That uses get_mm_counter() which
> we've updated in this patch, though I didn't spot the specific issue
> yet.
>
There is something fishy in mm/init-mm.c:init_mm. The initialization
of
.cpu_bitmap = CPU_BITS_NONE,
Keeps room for a NR_CPUs cpumask in that structure, but does not take
into account the new extra room needed for mm_cid and the hierarchical
per-cpu counters:
in mm_cache_init() we have:
mm_size = sizeof(struct mm_struct) + cpumask_size() + mm_cid_size() + get_rss_stat_items_size();
So AFAIU we should extend this end-of-mm size to include room for
mm_cid_size() (2 * cpumask_size), which would be an upstream bug,
and now room for get_rss_stat_items_size() (which is an issue specific
to -next due to hierarchical per-cpu counters).
An ugly work-around that may work (and then we can improve on this),
at the end of mm/init-mm.c:init_mm (completely untested):
.cpu_bitmap = { [0 ... ((3*BITS_TO_LONGS(NR_CPUS))-1 + ((69905 * NR_MM_COUNTERS * 64) / BYTES_PER_LONG))] = 0UL },
Thoughts ?
Thanks,
Mathieu
--
Mathieu Desnoyers
EfficiOS Inc.
https://www.efficios.com
^ permalink raw reply
* Re: [PATCH v3 4/4] tracing: move tracing declarations from kernel.h to a dedicated header
From: Steven Rostedt @ 2025-12-18 21:41 UTC (permalink / raw)
To: Yury Norov
Cc: Randy Dunlap, Andrew Morton, Masami Hiramatsu, Mathieu Desnoyers,
Andy Shevchenko, Christophe Leroy, Ingo Molnar, Jani Nikula,
Joonas Lahtinen, David Laight, Petr Pavlu, Andi Shyti,
Rodrigo Vivi, Tvrtko Ursulin, Daniel Gomez, Greg Kroah-Hartman,
Rafael J. Wysocki, Danilo Krummrich, linux-kernel, intel-gfx,
dri-devel, linux-modules, linux-trace-kernel, Kees Cook
In-Reply-To: <20251218162542.476009db@gandalf.local.home>
On Thu, 18 Dec 2025 16:25:42 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:
> On Thu, 18 Dec 2025 15:33:47 -0500
> Yury Norov <yury.norov@gmail.com> wrote:
>
> > > I don't actually remember why I had __trace_puts() pass in the size. I
> > > could change it to:
> >
> > This is the best approach. I'll schedule it for v4. Would you like me to
> > take it as-is, or you'd send a patch?
> >
>
> Let me send an official patch.
>
You can find it here (I Cc'd you too). Feel free to add it to your patch set.
https://lore.kernel.org/all/20251218163739.5605f9ea@gandalf.local.home/
-- Steve
^ permalink raw reply
* [PATCH] tracing: Remove size parameter in __trace_puts()
From: Steven Rostedt @ 2025-12-18 21:37 UTC (permalink / raw)
To: LKML, Linux Trace Kernel
Cc: Masami Hiramatsu, Mathieu Desnoyers, Yury Norov, Randy Dunlap
From: Steven Rostedt <rostedt@goodmis.org>
The __trace_puts() function takes a string pointer and the size of the
string itself. All users currently simply pass in the strlen() of the
string it is also passing in. There's no reason to pass in the size.
Instead have the __trace_puts() function do the strlen() within the
function itself.
This fixes a header recursion issue where using strlen() in the macro
calling __trace_puts() requires adding #include <linux/string.h> in order
to use strlen(). Removing the use of strlen() from the header fixes the
recursion issue.
Link: https://lore.kernel.org/all/aUN8Hm377C5A0ILX@yury/
Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
---
include/linux/kernel.h | 4 ++--
kernel/trace/trace.c | 7 +++----
kernel/trace/trace.h | 2 +-
3 files changed, 6 insertions(+), 7 deletions(-)
diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 5b46924fdff5..d5a939b8c391 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -331,10 +331,10 @@ int __trace_printk(unsigned long ip, const char *fmt, ...);
if (__builtin_constant_p(str)) \
__trace_bputs(_THIS_IP_, trace_printk_fmt); \
else \
- __trace_puts(_THIS_IP_, str, strlen(str)); \
+ __trace_puts(_THIS_IP_, str); \
})
extern int __trace_bputs(unsigned long ip, const char *str);
-extern int __trace_puts(unsigned long ip, const char *str, int size);
+extern int __trace_puts(unsigned long ip, const char *str);
extern void trace_dump_stack(int skip);
diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index e575956ef9b5..686741edb803 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -1178,11 +1178,10 @@ EXPORT_SYMBOL_GPL(__trace_array_puts);
* __trace_puts - write a constant string into the trace buffer.
* @ip: The address of the caller
* @str: The constant string to write
- * @size: The size of the string.
*/
-int __trace_puts(unsigned long ip, const char *str, int size)
+int __trace_puts(unsigned long ip, const char *str)
{
- return __trace_array_puts(printk_trace, ip, str, size);
+ return __trace_array_puts(printk_trace, ip, str, strlen(str));
}
EXPORT_SYMBOL_GPL(__trace_puts);
@@ -1201,7 +1200,7 @@ int __trace_bputs(unsigned long ip, const char *str)
int size = sizeof(struct bputs_entry);
if (!printk_binsafe(tr))
- return __trace_puts(ip, str, strlen(str));
+ return __trace_puts(ip, str);
if (!(tr->trace_flags & TRACE_ITER(PRINTK)))
return 0;
diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
index b6d42fe06115..de4e6713b84e 100644
--- a/kernel/trace/trace.h
+++ b/kernel/trace/trace.h
@@ -2116,7 +2116,7 @@ extern void tracing_log_err(struct trace_array *tr,
* about performance). The internal_trace_puts() is for such
* a purpose.
*/
-#define internal_trace_puts(str) __trace_puts(_THIS_IP_, str, strlen(str))
+#define internal_trace_puts(str) __trace_puts(_THIS_IP_, str)
#undef FTRACE_ENTRY
#define FTRACE_ENTRY(call, struct_name, id, tstruct, print) \
--
2.51.0
^ permalink raw reply related
* Re: [PATCH v3 4/4] tracing: move tracing declarations from kernel.h to a dedicated header
From: Steven Rostedt @ 2025-12-18 21:25 UTC (permalink / raw)
To: Yury Norov
Cc: Randy Dunlap, Andrew Morton, Masami Hiramatsu, Mathieu Desnoyers,
Andy Shevchenko, Christophe Leroy, Ingo Molnar, Jani Nikula,
Joonas Lahtinen, David Laight, Petr Pavlu, Andi Shyti,
Rodrigo Vivi, Tvrtko Ursulin, Daniel Gomez, Greg Kroah-Hartman,
Rafael J. Wysocki, Danilo Krummrich, linux-kernel, intel-gfx,
dri-devel, linux-modules, linux-trace-kernel, Kees Cook
In-Reply-To: <aURlK1gpCrfLEKN9@yury>
On Thu, 18 Dec 2025 15:33:47 -0500
Yury Norov <yury.norov@gmail.com> wrote:
> > I don't actually remember why I had __trace_puts() pass in the size. I
> > could change it to:
>
> This is the best approach. I'll schedule it for v4. Would you like me to
> take it as-is, or you'd send a patch?
>
Let me send an official patch.
-- Steve
^ permalink raw reply
* Re: [PATCH 2/2] bpf: Implement kretprobe fallback for kprobe multi link
From: Steven Rostedt @ 2025-12-18 21:09 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: liujing40, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Masami Hiramatsu, Martin KaFai Lau, Eduard, Song Liu,
Yonghong Song, John Fastabend, KP Singh, Stanislav Fomichev,
Hao Luo, Jiri Olsa, bpf, LKML, linux-trace-kernel, liujing40
In-Reply-To: <CAADnVQKC312JbOhjQZmMN-Me2V0GQ9qxoHeQkF+=PbYk0zc9KA@mail.gmail.com>
On Thu, 18 Dec 2025 09:53:16 -0800
Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:
> > +static void bpf_kprobe_unregister(struct bpf_kprobe *kps, u32 cnt)
> > +{
> > + for (int i = 0; i < cnt; i++)
> > + unregister_kretprobe(&kps[i].rp);
> > +}
>
> Nack.
> This is not a good idea.
> unregister_kretprobe() calls synchronize_rcu().
> So the above loop will cause soft lockups for sure.
Looks like it could be replaced with:
unregister_kretprobes(kps, cnt);
Which unregisters an array of kreptrobes and does a single
synchronize_rcu().
-- Steve
^ permalink raw reply
* Re: [PATCH v3 4/4] tracing: move tracing declarations from kernel.h to a dedicated header
From: Yury Norov @ 2025-12-18 20:52 UTC (permalink / raw)
To: Randy Dunlap
Cc: Steven Rostedt, Andrew Morton, Masami Hiramatsu,
Mathieu Desnoyers, Andy Shevchenko, Christophe Leroy, Ingo Molnar,
Jani Nikula, Joonas Lahtinen, David Laight, Petr Pavlu,
Andi Shyti, Rodrigo Vivi, Tvrtko Ursulin, Daniel Gomez,
Greg Kroah-Hartman, Rafael J. Wysocki, Danilo Krummrich,
linux-kernel, intel-gfx, dri-devel, linux-modules,
linux-trace-kernel, Kees Cook
In-Reply-To: <95f0c65b-15ff-45db-9845-579f7adf4c86@infradead.org>
On Thu, Dec 18, 2025 at 10:34:07AM -0800, Randy Dunlap wrote:
>
>
> On 12/18/25 9:33 AM, Steven Rostedt wrote:
> > On Wed, 17 Dec 2025 22:59:33 -0500
> > Yury Norov <yury.norov@gmail.com> wrote:
> >
> >> I deem to drop trace_printk.h from kernel.h - it is more aligned with
> >> the idea of unloading the header. The original motivation to keep
> >> trace_printk.h in kernel.h was just because a similar printk.h is living
> >> there. But after all, this is a purely debugging header, so no need for
> >> almost every C file to bear debugging stuff.
> >
> > It is a big deal for debugging stuff. A lot of developers debug their code
> > with trace_printk(), and do the "shotgun approach", where they cut and
> > paste trace_printk()s all over their code in several files. Having to now add:
> >
> > #include <linux/trace_printk.h>
> >
> > whenever a trace_printk() is added is going to be a big PITA and slow down
> > all debugging efforts.
>
> Eh? Maybe a PITA, but surely not a big one.
> Slow down "all debugging efforts?"
> Please cut down on the hyperbole.
For me, removing trace_prink.h saves 1.5-2% of compile time:
Before:
#1 real 5m12.602s
#2 real 5m11.333s
After:
#1 real 5m6.190s
#2 real 5m7.151s
I'm building ubuntu-derived localyesconfig with a couple extra drivers.
Steven, if you're not absolutely against, lets drop the trace_printk.h?
Thanks,
Yury
^ permalink raw reply
* Re: [PATCH v3 4/4] tracing: move tracing declarations from kernel.h to a dedicated header
From: Yury Norov @ 2025-12-18 20:33 UTC (permalink / raw)
To: Steven Rostedt
Cc: Randy Dunlap, Andrew Morton, Masami Hiramatsu, Mathieu Desnoyers,
Andy Shevchenko, Christophe Leroy, Ingo Molnar, Jani Nikula,
Joonas Lahtinen, David Laight, Petr Pavlu, Andi Shyti,
Rodrigo Vivi, Tvrtko Ursulin, Daniel Gomez, Greg Kroah-Hartman,
Rafael J. Wysocki, Danilo Krummrich, linux-kernel, intel-gfx,
dri-devel, linux-modules, linux-trace-kernel, Kees Cook
In-Reply-To: <20251218124326.22334325@gandalf.local.home>
On Thu, Dec 18, 2025 at 12:43:26PM -0500, Steven Rostedt wrote:
> On Thu, 18 Dec 2025 12:33:49 -0500
> Steven Rostedt <rostedt@goodmis.org> wrote:
>
> > On Wed, 17 Dec 2025 22:59:33 -0500
> > Yury Norov <yury.norov@gmail.com> wrote:
> >
> > > I deem to drop trace_printk.h from kernel.h - it is more aligned with
> > > the idea of unloading the header. The original motivation to keep
> > > trace_printk.h in kernel.h was just because a similar printk.h is living
> > > there. But after all, this is a purely debugging header, so no need for
> > > almost every C file to bear debugging stuff.
> >
> > It is a big deal for debugging stuff. A lot of developers debug their code
> > with trace_printk(), and do the "shotgun approach", where they cut and
> > paste trace_printk()s all over their code in several files. Having to now add:
> >
> > #include <linux/trace_printk.h>
> >
> > whenever a trace_printk() is added is going to be a big PITA and slow down
> > all debugging efforts.
> >
>
> I don't actually remember why I had __trace_puts() pass in the size. I
> could change it to:
This is the best approach. I'll schedule it for v4. Would you like me to
take it as-is, or you'd send a patch?
> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> index 5b46924fdff5..d5a939b8c391 100644
> --- a/include/linux/kernel.h
> +++ b/include/linux/kernel.h
> @@ -331,10 +331,10 @@ int __trace_printk(unsigned long ip, const char *fmt, ...);
> if (__builtin_constant_p(str)) \
> __trace_bputs(_THIS_IP_, trace_printk_fmt); \
> else \
> - __trace_puts(_THIS_IP_, str, strlen(str)); \
> + __trace_puts(_THIS_IP_, str); \
> })
> extern int __trace_bputs(unsigned long ip, const char *str);
> -extern int __trace_puts(unsigned long ip, const char *str, int size);
> +extern int __trace_puts(unsigned long ip, const char *str);
>
> extern void trace_dump_stack(int skip);
>
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index e575956ef9b5..686741edb803 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -1178,11 +1178,10 @@ EXPORT_SYMBOL_GPL(__trace_array_puts);
> * __trace_puts - write a constant string into the trace buffer.
> * @ip: The address of the caller
> * @str: The constant string to write
> - * @size: The size of the string.
> */
> -int __trace_puts(unsigned long ip, const char *str, int size)
> +int __trace_puts(unsigned long ip, const char *str)
> {
> - return __trace_array_puts(printk_trace, ip, str, size);
> + return __trace_array_puts(printk_trace, ip, str, strlen(str));
> }
> EXPORT_SYMBOL_GPL(__trace_puts);
>
> @@ -1201,7 +1200,7 @@ int __trace_bputs(unsigned long ip, const char *str)
> int size = sizeof(struct bputs_entry);
>
> if (!printk_binsafe(tr))
> - return __trace_puts(ip, str, strlen(str));
> + return __trace_puts(ip, str);
>
> if (!(tr->trace_flags & TRACE_ITER(PRINTK)))
> return 0;
> diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
> index b6d42fe06115..de4e6713b84e 100644
> --- a/kernel/trace/trace.h
> +++ b/kernel/trace/trace.h
> @@ -2116,7 +2116,7 @@ extern void tracing_log_err(struct trace_array *tr,
> * about performance). The internal_trace_puts() is for such
> * a purpose.
> */
> -#define internal_trace_puts(str) __trace_puts(_THIS_IP_, str, strlen(str))
> +#define internal_trace_puts(str) __trace_puts(_THIS_IP_, str)
>
> #undef FTRACE_ENTRY
> #define FTRACE_ENTRY(call, struct_name, id, tstruct, print) \
>
>
>
> Which removes the strlen() altogether.
>
> -- Steve
^ permalink raw reply
* Re: [PATCH v3 4/4] tracing: move tracing declarations from kernel.h to a dedicated header
From: Randy Dunlap @ 2025-12-18 18:34 UTC (permalink / raw)
To: Steven Rostedt, Yury Norov
Cc: Andrew Morton, Masami Hiramatsu, Mathieu Desnoyers,
Andy Shevchenko, Christophe Leroy, Ingo Molnar, Jani Nikula,
Joonas Lahtinen, David Laight, Petr Pavlu, Andi Shyti,
Rodrigo Vivi, Tvrtko Ursulin, Daniel Gomez, Greg Kroah-Hartman,
Rafael J. Wysocki, Danilo Krummrich, linux-kernel, intel-gfx,
dri-devel, linux-modules, linux-trace-kernel, Kees Cook
In-Reply-To: <20251218123349.35339242@gandalf.local.home>
On 12/18/25 9:33 AM, Steven Rostedt wrote:
> On Wed, 17 Dec 2025 22:59:33 -0500
> Yury Norov <yury.norov@gmail.com> wrote:
>
>> I deem to drop trace_printk.h from kernel.h - it is more aligned with
>> the idea of unloading the header. The original motivation to keep
>> trace_printk.h in kernel.h was just because a similar printk.h is living
>> there. But after all, this is a purely debugging header, so no need for
>> almost every C file to bear debugging stuff.
>
> It is a big deal for debugging stuff. A lot of developers debug their code
> with trace_printk(), and do the "shotgun approach", where they cut and
> paste trace_printk()s all over their code in several files. Having to now add:
>
> #include <linux/trace_printk.h>
>
> whenever a trace_printk() is added is going to be a big PITA and slow down
> all debugging efforts.
Eh? Maybe a PITA, but surely not a big one.
Slow down "all debugging efforts?"
Please cut down on the hyperbole.
--
~Randy
^ permalink raw reply
* Re: [PATCH v10 2/3] mm: Fix OOM killer inaccuracy on large many-core systems
From: Mark Brown @ 2025-12-18 18:00 UTC (permalink / raw)
To: Mathieu Desnoyers
Cc: Andrew Morton, linux-kernel, Paul E. McKenney, Steven Rostedt,
Masami Hiramatsu, Dennis Zhou, Tejun Heo, Christoph Lameter,
Martin Liu, David Rientjes, christian.koenig, Shakeel Butt,
SeongJae Park, Michal Hocko, Johannes Weiner, Sweet Tea Dorminy,
Lorenzo Stoakes, Liam R . Howlett, Mike Rapoport,
Suren Baghdasaryan, Vlastimil Babka, Christian Brauner, Wei Yang,
David Hildenbrand, Miaohe Lin, Al Viro, linux-mm,
linux-trace-kernel, Yu Zhao, Roman Gushchin, Mateusz Guzik,
Matthew Wilcox, Baolin Wang, Aboorva Devarajan, Aishwarya TCV
In-Reply-To: <20251213185608.3418096-3-mathieu.desnoyers@efficios.com>
[-- Attachment #1: Type: text/plain, Size: 8516 bytes --]
On Sat, Dec 13, 2025 at 01:56:07PM -0500, Mathieu Desnoyers wrote:
> Use hierarchical per-cpu counters for rss tracking to fix the per-mm RSS
> tracking which has become too inaccurate for OOM killer purposes on
> large many-core systems.
We're seeing boot time crashes in -next on the Arm FVP and Ampere Altra
which bisect to this patch which is commit 240587b6cca2822d. Many other
platforms aren't showing this, though we do have some other breakage in
-next which might be obscuring things. We get a NULL dereference:
[ 2.481143] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000000
...
[ 2.485036] Call trace:
[ 2.485094] acct_account_cputime+0x40/0xa4 (P)
[ 2.485226] irqtime_account_process_tick+0x17c/0x1d8
[ 2.485382] account_process_tick+0x12c/0x148
[ 2.485531] update_process_times+0x28/0xdc
[ 2.485656] tick_nohz_handler+0xbc/0x1bc
[ 2.485809] __hrtimer_run_queues+0x130/0x184
I note that __acct_update_integrals is being called from here most
likely inline and doing get_mm_rss(). That uses get_mm_counter() which
we've updated in this patch, though I didn't spot the specific issue
yet.
Full log:
https://lava.sirena.org.uk/scheduler/job/2269797#L1305
Bisect log:
# bad: [1058ca9db0edaedcb16480cc74b78ed06f0d1f54] Add linux-next specific files for 20251218
# good: [b67535593a28aff9d355799ec5efc2e90bc405a6] Merge branch 'for-linux-next-fixes' of https://gitlab.freedesktop.org/drm/misc/kernel.git
# good: [f4acea9eef704607d1a950909ce3a52a770d6be2] spi: dt-bindings: st,stm32-spi: add 'power-domains' property
# good: [f25c7d709b93602ee9a08eba522808a18e1f5d56] ASoC: SOF: Intel: pci-nvl: Set on_demand_dsp_boot for NVL-S
# good: [524ee559948d8d079b13466e70fa741f909699c0] ASoC: SOF: Intel: hda: Only check SSP MCLK mask in case of IPC3
# good: [fa08b566860bca8ebf9300090b85174c34de7ca5] spi: rzv2h-rspi: add support for DMA mode
# good: [fee876b2ec75dcc18fdea154eae1f5bf14d82659] spi: stm32-qspi: Simplify SMIE interrupt test
# good: [b884e34994ca41f7b7819f3c41b78ff494787b27] spi: spi-fsl-lpspi: convert min_t() to simple min()
# good: [ba9b28652c75b07383e267328f1759195d5430f7] spi: imx: enable DMA mode for target operation
# good: [124f6155f3d97b0e33f178c10a5138a42c8fd207] ASoC: renesas: rz-ssi: Add support for 32 bits sample width
# good: [aa30193af8873b3ccfd70a4275336ab6cbd4e5e6] ASoC: Intel: catpt: Drop superfluous space in PCM code
# good: [9e92c559d49d6fb903af17a31a469aac51b1766d] regulator: max77675: Add MAX77675 regulator driver
# good: [81acbdc51bbbec822a1525481f2f70677c47aee0] ASoC: sdw-mockup: Drop dummy remove function
# good: [0bb160c92ad400c692984763996b758458adea17] ASoC: qcom: Minor readability improve with new lines
# good: [03d281f384768610bf90697bce9e35d3d596de77] rust: regulator: add __rust_helper to helpers
# good: [e39011184f23de3d04ca8e80b4df76c9047b4026] ASoC: SDCA: functions: Fix confusing cleanup.h syntax
git bisect start '1058ca9db0edaedcb16480cc74b78ed06f0d1f54' 'b67535593a28aff9d355799ec5efc2e90bc405a6' 'f4acea9eef704607d1a950909ce3a52a770d6be2' 'f25c7d709b93602ee9a08eba522808a18e1f5d56' '524ee559948d8d079b13466e70fa741f909699c0' 'fa08b566860bca8ebf9300090b85174c34de7ca5' 'fee876b2ec75dcc18fdea154eae1f5bf14d82659' 'b884e34994ca41f7b7819f3c41b78ff494787b27' 'ba9b28652c75b07383e267328f1759195d5430f7' '124f6155f3d97b0e33f178c10a5138a42c8fd207' 'aa30193af8873b3ccfd70a4275336ab6cbd4e5e6' '9e92c559d49d6fb903af17a31a469aac51b1766d' '81acbdc51bbbec822a1525481f2f70677c47aee0' '0bb160c92ad400c692984763996b758458adea17' '03d281f384768610bf90697bce9e35d3d596de77' 'e39011184f23de3d04ca8e80b4df76c9047b4026'
# test job: [f4acea9eef704607d1a950909ce3a52a770d6be2] https://lava.sirena.org.uk/scheduler/job/2243946
# test job: [f25c7d709b93602ee9a08eba522808a18e1f5d56] https://lava.sirena.org.uk/scheduler/job/2244079
# test job: [524ee559948d8d079b13466e70fa741f909699c0] https://lava.sirena.org.uk/scheduler/job/2243984
# test job: [fa08b566860bca8ebf9300090b85174c34de7ca5] https://lava.sirena.org.uk/scheduler/job/2232928
# test job: [fee876b2ec75dcc18fdea154eae1f5bf14d82659] https://lava.sirena.org.uk/scheduler/job/2231264
# test job: [b884e34994ca41f7b7819f3c41b78ff494787b27] https://lava.sirena.org.uk/scheduler/job/2231779
# test job: [ba9b28652c75b07383e267328f1759195d5430f7] https://lava.sirena.org.uk/scheduler/job/2231420
# test job: [124f6155f3d97b0e33f178c10a5138a42c8fd207] https://lava.sirena.org.uk/scheduler/job/2232853
# test job: [aa30193af8873b3ccfd70a4275336ab6cbd4e5e6] https://lava.sirena.org.uk/scheduler/job/2232678
# test job: [9e92c559d49d6fb903af17a31a469aac51b1766d] https://lava.sirena.org.uk/scheduler/job/2232518
# test job: [81acbdc51bbbec822a1525481f2f70677c47aee0] https://lava.sirena.org.uk/scheduler/job/2232960
# test job: [0bb160c92ad400c692984763996b758458adea17] https://lava.sirena.org.uk/scheduler/job/2233063
# test job: [03d281f384768610bf90697bce9e35d3d596de77] https://lava.sirena.org.uk/scheduler/job/2231118
# test job: [e39011184f23de3d04ca8e80b4df76c9047b4026] https://lava.sirena.org.uk/scheduler/job/2232449
# test job: [1058ca9db0edaedcb16480cc74b78ed06f0d1f54] https://lava.sirena.org.uk/scheduler/job/2269797
# bad: [1058ca9db0edaedcb16480cc74b78ed06f0d1f54] Add linux-next specific files for 20251218
git bisect bad 1058ca9db0edaedcb16480cc74b78ed06f0d1f54
# test job: [066839a14b076089272a60ed81f11e423d5c9361] https://lava.sirena.org.uk/scheduler/job/2270122
# bad: [066839a14b076089272a60ed81f11e423d5c9361] Merge branch 'for-linux-next' of https://gitlab.freedesktop.org/drm/misc/kernel.git
git bisect bad 066839a14b076089272a60ed81f11e423d5c9361
# test job: [3f506139d1ada1f7dbb8593973ed287379747c06] https://lava.sirena.org.uk/scheduler/job/2270335
# bad: [3f506139d1ada1f7dbb8593973ed287379747c06] Merge branch 'xtensa-for-next' of https://github.com/jcmvbkbc/linux-xtensa.git
git bisect bad 3f506139d1ada1f7dbb8593973ed287379747c06
# test job: [b5d3cb02801b2e109f9dd0e5e39ca47ab1edaf14] https://lava.sirena.org.uk/scheduler/job/2270661
# bad: [b5d3cb02801b2e109f9dd0e5e39ca47ab1edaf14] Merge branch 'for-next' of https://git.kernel.org/pub/scm/linux/kernel/git/khilman/linux-omap.git
git bisect bad b5d3cb02801b2e109f9dd0e5e39ca47ab1edaf14
# test job: [8cf5d38999d1dca70f34de411b72a099d07c1b6a] https://lava.sirena.org.uk/scheduler/job/2270863
# bad: [8cf5d38999d1dca70f34de411b72a099d07c1b6a] Merge branch 'kbuild-next' of https://git.kernel.org/pub/scm/linux/kernel/git/kbuild/linux.git
git bisect bad 8cf5d38999d1dca70f34de411b72a099d07c1b6a
# test job: [6f7df192578220290c5ee01dc146f01c919fdb7b] https://lava.sirena.org.uk/scheduler/job/2271024
# good: [6f7df192578220290c5ee01dc146f01c919fdb7b] kallsyms/bpf: rename __bpf_address_lookup() to bpf_address_lookup()
git bisect good 6f7df192578220290c5ee01dc146f01c919fdb7b
# test job: [a525b83d913f000bed66b69f6d9c05c0c04551dd] https://lava.sirena.org.uk/scheduler/job/2271528
# bad: [a525b83d913f000bed66b69f6d9c05c0c04551dd] mm: add basic tests for lazy_mmu
git bisect bad a525b83d913f000bed66b69f6d9c05c0c04551dd
# test job: [667c24fb34a273ffc323d591ac628285602bd324] https://lava.sirena.org.uk/scheduler/job/2271625
# bad: [667c24fb34a273ffc323d591ac628285602bd324] sparc/mm: implement arch_flush_lazy_mmu_mode()
git bisect bad 667c24fb34a273ffc323d591ac628285602bd324
# test job: [d70090581c46c001d0886afbaf08bcbc85a5e8bc] https://lava.sirena.org.uk/scheduler/job/2271840
# bad: [d70090581c46c001d0886afbaf08bcbc85a5e8bc] mm: implement precise OOM killer task selection
git bisect bad d70090581c46c001d0886afbaf08bcbc85a5e8bc
# test job: [eb526e6344d1dd7784bef5aa4cbe7f7fada3bf12] https://lava.sirena.org.uk/scheduler/job/2271925
# good: [eb526e6344d1dd7784bef5aa4cbe7f7fada3bf12] mm/damon/core: fix memory leak of repeat mode damon_call_control objects
git bisect good eb526e6344d1dd7784bef5aa4cbe7f7fada3bf12
# test job: [240587b6cca2822dd579caa0ff05a7f5e459c597] https://lava.sirena.org.uk/scheduler/job/2272240
# bad: [240587b6cca2822dd579caa0ff05a7f5e459c597] mm: fix OOM killer inaccuracy on large many-core systems
git bisect bad 240587b6cca2822dd579caa0ff05a7f5e459c597
# test job: [f9ff5ba6bbfcc8f8a61cd7dd61a0c33b7c4deb30] https://lava.sirena.org.uk/scheduler/job/2272347
# good: [f9ff5ba6bbfcc8f8a61cd7dd61a0c33b7c4deb30] lib: introduce hierarchical per-cpu counters
git bisect good f9ff5ba6bbfcc8f8a61cd7dd61a0c33b7c4deb30
# first bad commit: [240587b6cca2822dd579caa0ff05a7f5e459c597] mm: fix OOM killer inaccuracy on large many-core systems
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH 2/2] bpf: Implement kretprobe fallback for kprobe multi link
From: Alexei Starovoitov @ 2025-12-18 17:53 UTC (permalink / raw)
To: liujing40
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Masami Hiramatsu, Martin KaFai Lau, Eduard, Song Liu,
Yonghong Song, John Fastabend, KP Singh, Stanislav Fomichev,
Hao Luo, Jiri Olsa, bpf, LKML, linux-trace-kernel, liujing40
In-Reply-To: <20251218130629.365398-3-liujing40@xiaomi.com>
On Thu, Dec 18, 2025 at 5:07 AM liujing40 <liujing.root@gmail.com> wrote:
>
> When fprobe is not available, provide a fallback implementation of
> kprobe_multi using the traditional kretprobe API.
>
> Uses kretprobe's entry_handler and handler callbacks to simulate fprobe's
> entry/exit functionality.
>
> Signed-off-by: Jing Liu <liujing40@xiaomi.com>
> ---
> kernel/trace/bpf_trace.c | 307 +++++++++++++++++++++++++++++++++++++--
> 1 file changed, 295 insertions(+), 12 deletions(-)
>
> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index 1fd07c10378f..426a1c627508 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -2274,12 +2274,44 @@ struct bpf_session_run_ctx {
> void *data;
> };
>
> -#ifdef CONFIG_FPROBE
> +#if defined(CONFIG_FPROBE) || defined(CONFIG_KRETPROBES)
> +#ifndef CONFIG_FPROBE
> +struct bpf_kprobe {
> + struct bpf_kprobe_multi_link *link;
> + u64 cookie;
> + struct kretprobe rp;
> +};
> +
> +static void bpf_kprobe_unregister(struct bpf_kprobe *kps, u32 cnt)
> +{
> + for (int i = 0; i < cnt; i++)
> + unregister_kretprobe(&kps[i].rp);
> +}
Nack.
This is not a good idea.
unregister_kretprobe() calls synchronize_rcu().
So the above loop will cause soft lockups for sure.
pw-bot: cr
^ permalink raw reply
* Re: [PATCH v3 4/4] tracing: move tracing declarations from kernel.h to a dedicated header
From: Steven Rostedt @ 2025-12-18 17:43 UTC (permalink / raw)
To: Yury Norov
Cc: Randy Dunlap, Andrew Morton, Masami Hiramatsu, Mathieu Desnoyers,
Andy Shevchenko, Christophe Leroy, Ingo Molnar, Jani Nikula,
Joonas Lahtinen, David Laight, Petr Pavlu, Andi Shyti,
Rodrigo Vivi, Tvrtko Ursulin, Daniel Gomez, Greg Kroah-Hartman,
Rafael J. Wysocki, Danilo Krummrich, linux-kernel, intel-gfx,
dri-devel, linux-modules, linux-trace-kernel, Kees Cook
In-Reply-To: <20251218123349.35339242@gandalf.local.home>
On Thu, 18 Dec 2025 12:33:49 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:
> On Wed, 17 Dec 2025 22:59:33 -0500
> Yury Norov <yury.norov@gmail.com> wrote:
>
> > I deem to drop trace_printk.h from kernel.h - it is more aligned with
> > the idea of unloading the header. The original motivation to keep
> > trace_printk.h in kernel.h was just because a similar printk.h is living
> > there. But after all, this is a purely debugging header, so no need for
> > almost every C file to bear debugging stuff.
>
> It is a big deal for debugging stuff. A lot of developers debug their code
> with trace_printk(), and do the "shotgun approach", where they cut and
> paste trace_printk()s all over their code in several files. Having to now add:
>
> #include <linux/trace_printk.h>
>
> whenever a trace_printk() is added is going to be a big PITA and slow down
> all debugging efforts.
>
I don't actually remember why I had __trace_puts() pass in the size. I
could change it to:
diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 5b46924fdff5..d5a939b8c391 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -331,10 +331,10 @@ int __trace_printk(unsigned long ip, const char *fmt, ...);
if (__builtin_constant_p(str)) \
__trace_bputs(_THIS_IP_, trace_printk_fmt); \
else \
- __trace_puts(_THIS_IP_, str, strlen(str)); \
+ __trace_puts(_THIS_IP_, str); \
})
extern int __trace_bputs(unsigned long ip, const char *str);
-extern int __trace_puts(unsigned long ip, const char *str, int size);
+extern int __trace_puts(unsigned long ip, const char *str);
extern void trace_dump_stack(int skip);
diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index e575956ef9b5..686741edb803 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -1178,11 +1178,10 @@ EXPORT_SYMBOL_GPL(__trace_array_puts);
* __trace_puts - write a constant string into the trace buffer.
* @ip: The address of the caller
* @str: The constant string to write
- * @size: The size of the string.
*/
-int __trace_puts(unsigned long ip, const char *str, int size)
+int __trace_puts(unsigned long ip, const char *str)
{
- return __trace_array_puts(printk_trace, ip, str, size);
+ return __trace_array_puts(printk_trace, ip, str, strlen(str));
}
EXPORT_SYMBOL_GPL(__trace_puts);
@@ -1201,7 +1200,7 @@ int __trace_bputs(unsigned long ip, const char *str)
int size = sizeof(struct bputs_entry);
if (!printk_binsafe(tr))
- return __trace_puts(ip, str, strlen(str));
+ return __trace_puts(ip, str);
if (!(tr->trace_flags & TRACE_ITER(PRINTK)))
return 0;
diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
index b6d42fe06115..de4e6713b84e 100644
--- a/kernel/trace/trace.h
+++ b/kernel/trace/trace.h
@@ -2116,7 +2116,7 @@ extern void tracing_log_err(struct trace_array *tr,
* about performance). The internal_trace_puts() is for such
* a purpose.
*/
-#define internal_trace_puts(str) __trace_puts(_THIS_IP_, str, strlen(str))
+#define internal_trace_puts(str) __trace_puts(_THIS_IP_, str)
#undef FTRACE_ENTRY
#define FTRACE_ENTRY(call, struct_name, id, tstruct, print) \
Which removes the strlen() altogether.
-- Steve
^ permalink raw reply related
* Re: [PATCH v3 4/4] tracing: move tracing declarations from kernel.h to a dedicated header
From: Steven Rostedt @ 2025-12-18 17:33 UTC (permalink / raw)
To: Yury Norov
Cc: Randy Dunlap, Andrew Morton, Masami Hiramatsu, Mathieu Desnoyers,
Andy Shevchenko, Christophe Leroy, Ingo Molnar, Jani Nikula,
Joonas Lahtinen, David Laight, Petr Pavlu, Andi Shyti,
Rodrigo Vivi, Tvrtko Ursulin, Daniel Gomez, Greg Kroah-Hartman,
Rafael J. Wysocki, Danilo Krummrich, linux-kernel, intel-gfx,
dri-devel, linux-modules, linux-trace-kernel, Kees Cook
In-Reply-To: <aUN8Hm377C5A0ILX@yury>
On Wed, 17 Dec 2025 22:59:33 -0500
Yury Norov <yury.norov@gmail.com> wrote:
> I deem to drop trace_printk.h from kernel.h - it is more aligned with
> the idea of unloading the header. The original motivation to keep
> trace_printk.h in kernel.h was just because a similar printk.h is living
> there. But after all, this is a purely debugging header, so no need for
> almost every C file to bear debugging stuff.
It is a big deal for debugging stuff. A lot of developers debug their code
with trace_printk(), and do the "shotgun approach", where they cut and
paste trace_printk()s all over their code in several files. Having to now add:
#include <linux/trace_printk.h>
whenever a trace_printk() is added is going to be a big PITA and slow down
all debugging efforts.
-- Steve
^ permalink raw reply
* Re: [PATCH 12/12] mm/damon/core: add trace point for damos stat per apply interval
From: Steven Rostedt @ 2025-12-18 17:29 UTC (permalink / raw)
To: Andrew Morton
Cc: SeongJae Park, Masami Hiramatsu, Mathieu Desnoyers, damon,
linux-kernel, linux-mm, linux-trace-kernel
In-Reply-To: <20251217182915.5284d66b58ac7b744b8dcf7f@linux-foundation.org>
On Wed, 17 Dec 2025 18:29:15 -0800
Andrew Morton <akpm@linux-foundation.org> wrote:
> > > Nit, but it's been stated that strings should not be broken up because of
> > > the column limit.
>
> screw the rules
Heh.
>
> > --- a/include/trace/events/damon.h
> > +++ b/include/trace/events/damon.h
> > @@ -40,9 +40,7 @@ TRACE_EVENT(damos_stat_after_apply_interval,
> > __entry->nr_snapshots = stat->nr_snapshots;
> > ),
> >
> > - TP_printk("ctx_idx=%u scheme_idx=%u nr_tried=%lu sz_tried=%lu "
> > - "nr_applied=%lu sz_tried=%lu sz_ops_filter_passed=%lu "
> > - "qt_exceeds=%lu nr_snapshots=%lu",
> > + TP_printk("ctx_idx=%u scheme_idx=%u nr_tried=%lu sz_tried=%lu nr_applied=%lu sz_tried=%lu sz_ops_filter_passed=%lu qt_exceeds=%lu nr_snapshots=%lu",
> > __entry->context_idx, __entry->scheme_idx,
> > __entry->nr_tried, __entry->sz_tried,
> > __entry->nr_applied, __entry->sz_applied,
>
> because that's just crazy. Let's use some judgment here!
Does it really matter? Actually, I prefer this way because it better shows
where the format ends and the parameters start. I care more about the
parameters than the format string, except to look for each "%*" value.
-- Steve
^ permalink raw reply
* Re: [PATCH 1/3] mm/khugepaged: remove unnecessary goto 'skip' label
From: Liam R. Howlett @ 2025-12-18 16:41 UTC (permalink / raw)
To: Shivank Garg
Cc: Andrew Morton, David Hildenbrand, Lorenzo Stoakes, Zi Yan,
Baolin Wang, Nico Pache, Ryan Roberts, Dev Jain, Barry Song,
Lance Yang, linux-mm, linux-kernel, linux-trace-kernel
In-Reply-To: <20251216111139.95438-4-shivankg@amd.com>
* Shivank Garg <shivankg@amd.com> [251216 06:12]:
> Replace 'goto skip' with actual logic for better code readability.
>
> No functional change.
>
> Signed-off-by: Shivank Garg <shivankg@amd.com>
Reviewed-by: Liam R. Howlett <Liam.Howlett@oracle.com>
> ---
> mm/khugepaged.c | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/mm/khugepaged.c b/mm/khugepaged.c
> index 6c8c35d3e0c9..107146f012b1 100644
> --- a/mm/khugepaged.c
> +++ b/mm/khugepaged.c
> @@ -2442,14 +2442,15 @@ static unsigned int khugepaged_scan_mm_slot(unsigned int pages, int *result,
> break;
> }
> if (!thp_vma_allowable_order(vma, vma->vm_flags, TVA_KHUGEPAGED, PMD_ORDER)) {
> -skip:
> progress++;
> continue;
> }
> hstart = round_up(vma->vm_start, HPAGE_PMD_SIZE);
> hend = round_down(vma->vm_end, HPAGE_PMD_SIZE);
> - if (khugepaged_scan.address > hend)
> - goto skip;
> + if (khugepaged_scan.address > hend) {
> + progress++;
> + continue;
> + }
> if (khugepaged_scan.address < hstart)
> khugepaged_scan.address = hstart;
> VM_BUG_ON(khugepaged_scan.address & ~HPAGE_PMD_MASK);
> --
> 2.43.0
>
^ permalink raw reply
* Re: [PATCHv5 bpf-next 9/9] bpf,x86: Use single ftrace_ops for direct calls
From: Steven Rostedt @ 2025-12-18 16:26 UTC (permalink / raw)
To: Jiri Olsa
Cc: Steven Rostedt, Florent Revest, Mark Rutland, bpf, linux-kernel,
linux-trace-kernel, linux-arm-kernel, Alexei Starovoitov,
Daniel Borkmann, Andrii Nakryiko, Menglong Dong, Song Liu
In-Reply-To: <20251215211402.353056-10-jolsa@kernel.org>
On Mon, 15 Dec 2025 22:14:02 +0100
Jiri Olsa <jolsa@kernel.org> wrote:
> Using single ftrace_ops for direct calls update instead of allocating
> ftrace_ops object for each trampoline.
>
> With single ftrace_ops object we can use update_ftrace_direct_* api
> that allows multiple ip sites updates on single ftrace_ops object.
>
> Adding HAVE_SINGLE_FTRACE_DIRECT_OPS config option to be enabled on
> each arch that supports this.
>
> At the moment we can enable this only on x86 arch, because arm relies
> on ftrace_ops object representing just single trampoline image (stored
> in ftrace_ops::direct_call). Ach that do not support this will continue
My back "Ach" and doesn't support me well. ;-)
> to use *_ftrace_direct api.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
> arch/x86/Kconfig | 1 +
> kernel/bpf/trampoline.c | 195 ++++++++++++++++++++++++++++++++++------
> kernel/trace/Kconfig | 3 +
> kernel/trace/ftrace.c | 7 +-
> 4 files changed, 177 insertions(+), 29 deletions(-)
>
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index 17a107cc5244..d0c36e49e66e 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -335,6 +335,7 @@ config X86
> select SCHED_SMT if SMP
> select ARCH_SUPPORTS_SCHED_CLUSTER if SMP
> select ARCH_SUPPORTS_SCHED_MC if SMP
> + select HAVE_SINGLE_FTRACE_DIRECT_OPS if X86_64 && DYNAMIC_FTRACE_WITH_DIRECT_CALLS
You can remove the "&& DYNAMIC_FTRACE_WITH_DIRECT_CALLS" part by having the
config depend on it (see below).
>
> config INSTRUCTION_DECODER
> def_bool y
> diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c
> index 17af2aad8382..02371db3db3e 100644
> --- a/kernel/bpf/trampoline.c
> +++ b/kernel/bpf/trampoline.c
> @@ -33,12 +33,40 @@ static DEFINE_MUTEX(trampoline_mutex);
> #ifdef CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS
> static int bpf_trampoline_update(struct bpf_trampoline *tr, bool lock_direct_mutex);
>
> +#ifdef CONFIG_HAVE_SINGLE_FTRACE_DIRECT_OPS
Make this:
#ifdef CONFIG_SINGLE_FTRACE_DIRECT_OPS
for the suggested modification in the Kconfig below.
> +static struct bpf_trampoline *direct_ops_ip_lookup(struct ftrace_ops *ops, unsigned long ip)
> +{
> + struct hlist_head *head_ip;
> + struct bpf_trampoline *tr;
> +
> + mutex_lock(&trampoline_mutex);
guard(mutex)(&trampoline_mutex);
> + head_ip = &trampoline_ip_table[hash_64(ip, TRAMPOLINE_HASH_BITS)];
> + hlist_for_each_entry(tr, head_ip, hlist_ip) {
> + if (tr->ip == ip)
return NULL;
> + goto out;
> + }
> + tr = NULL;
> +out:
> + mutex_unlock(&trampoline_mutex);
No need for the above
> + return tr;
> +}
> +#else
> +static struct bpf_trampoline *direct_ops_ip_lookup(struct ftrace_ops *ops, unsigned long ip)
> +{
> + return ops->private;
> +}
> +#endif /* CONFIG_HAVE_SINGLE_FTRACE_DIRECT_OPS */
> +
> static int bpf_tramp_ftrace_ops_func(struct ftrace_ops *ops, unsigned long ip,
> enum ftrace_ops_cmd cmd)
> {
> - struct bpf_trampoline *tr = ops->private;
> + struct bpf_trampoline *tr;
> int ret = 0;
>
> + tr = direct_ops_ip_lookup(ops, ip);
> + if (!tr)
> + return -EINVAL;
> +
> if (cmd == FTRACE_OPS_CMD_ENABLE_SHARE_IPMODIFY_SELF) {
> /* This is called inside register_ftrace_direct_multi(), so
> * tr->mutex is already locked.
> @@ -137,6 +165,139 @@ void bpf_image_ksym_del(struct bpf_ksym *ksym)
> PAGE_SIZE, true, ksym->name);
> }
>
> +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS
> +#ifdef CONFIG_HAVE_SINGLE_FTRACE_DIRECT_OPS
Replace the above two with:
#ifdef CONFIG_SINGLE_FTRACE_DIRECT_OPS
> +/*
> + * We have only single direct_ops which contains all the direct call
> + * sites and is the only global ftrace_ops for all trampolines.
> + *
> + * We use 'update_ftrace_direct_*' api for attachment.
> + */
> +struct ftrace_ops direct_ops = {
> + .ops_func = bpf_tramp_ftrace_ops_func,
> +};
> +
> +static int direct_ops_alloc(struct bpf_trampoline *tr)
> +{
> + tr->fops = &direct_ops;
> + return 0;
> +}
> +
> +static void direct_ops_free(struct bpf_trampoline *tr) { }
> +
> +static struct ftrace_hash *hash_from_ip(struct bpf_trampoline *tr, void *ptr)
> +{
> + unsigned long ip, addr = (unsigned long) ptr;
> + struct ftrace_hash *hash;
> +
> + ip = ftrace_location(tr->ip);
> + if (!ip)
> + return NULL;
> + hash = alloc_ftrace_hash(FTRACE_HASH_DEFAULT_BITS);
> + if (!hash)
> + return NULL;
> + if (bpf_trampoline_use_jmp(tr->flags))
> + addr = ftrace_jmp_set(addr);
> + if (!add_hash_entry_direct(hash, ip, addr)) {
> + free_ftrace_hash(hash);
> + return NULL;
> + }
> + return hash;
> +}
> +
> +static int direct_ops_add(struct bpf_trampoline *tr, void *addr)
> +{
> + struct ftrace_hash *hash = hash_from_ip(tr, addr);
> + int err = -ENOMEM;
> +
> + if (hash)
> + err = update_ftrace_direct_add(tr->fops, hash);
> + free_ftrace_hash(hash);
> + return err;
> +}
I think these functions would be cleaner as:
{
struct ftrace_hash *hash = hash_from_ip(tr, addr);
int err;
if (!hash)
return -ENOMEM;
err = update_ftrace_direct_*(tr->fops, hash);
free_ftrace_hash(hash);
return err;
}
> +
> +static int direct_ops_del(struct bpf_trampoline *tr, void *addr)
> +{
> + struct ftrace_hash *hash = hash_from_ip(tr, addr);
> + int err = -ENOMEM;
> +
> + if (hash)
> + err = update_ftrace_direct_del(tr->fops, hash);
> + free_ftrace_hash(hash);
> + return err;
> +}
> +
> +static int direct_ops_mod(struct bpf_trampoline *tr, void *addr, bool lock_direct_mutex)
> +{
> + struct ftrace_hash *hash = hash_from_ip(tr, addr);
> + int err = -ENOMEM;
> +
> + if (hash)
> + err = update_ftrace_direct_mod(tr->fops, hash, lock_direct_mutex);
> + free_ftrace_hash(hash);
> + return err;
> +}
> +#else
> +/*
> + * We allocate ftrace_ops object for each trampoline and it contains
> + * call site specific for that trampoline.
> + *
> + * We use *_ftrace_direct api for attachment.
> + */
> +static int direct_ops_alloc(struct bpf_trampoline *tr)
> +{
> + tr->fops = kzalloc(sizeof(struct ftrace_ops), GFP_KERNEL);
> + if (!tr->fops)
> + return -ENOMEM;
> + tr->fops->private = tr;
> + tr->fops->ops_func = bpf_tramp_ftrace_ops_func;
> + return 0;
> +}
> +
> +static void direct_ops_free(struct bpf_trampoline *tr)
> +{
> + if (tr->fops) {
> + ftrace_free_filter(tr->fops);
> + kfree(tr->fops);
> + }
> +}
Why not:
static void direct_ops_free(struct bpf_trampoline *tr)
{
if (!tr->fops)
return;
ftrace_free_filter(tr->fops);
kfree(tr->fops);
}
?
> +
> +static int direct_ops_add(struct bpf_trampoline *tr, void *ptr)
> +{
> + unsigned long addr = (unsigned long) ptr;
> + struct ftrace_ops *ops = tr->fops;
> + int ret;
> +
> + if (bpf_trampoline_use_jmp(tr->flags))
> + addr = ftrace_jmp_set(addr);
> +
> + ret = ftrace_set_filter_ip(ops, tr->ip, 0, 1);
> + if (ret)
> + return ret;
> + return register_ftrace_direct(ops, addr);
> +}
> +
> +static int direct_ops_del(struct bpf_trampoline *tr, void *addr)
> +{
> + return unregister_ftrace_direct(tr->fops, (long)addr, false);
> +}
> +
> +static int direct_ops_mod(struct bpf_trampoline *tr, void *ptr, bool lock_direct_mutex)
> +{
> + unsigned long addr = (unsigned long) ptr;
> + struct ftrace_ops *ops = tr->fops;
> +
> + if (bpf_trampoline_use_jmp(tr->flags))
> + addr = ftrace_jmp_set(addr);
> + if (lock_direct_mutex)
> + return modify_ftrace_direct(ops, addr);
> + return modify_ftrace_direct_nolock(ops, addr);
> +}
> +#endif /* CONFIG_HAVE_SINGLE_FTRACE_DIRECT_OPS */
> +#else
> +static void direct_ops_free(struct bpf_trampoline *tr) { }
This is somewhat inconsistent with direct_ops_alloc() that has:
#ifdef CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS
if (direct_ops_alloc(tr)) {
kfree(tr);
tr = NULL;
goto out;
}
#endif
Now, if you wrap the direct_ops_free() too, we can remove the
#ifdef CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS
part with my kconfig suggestion. Otherwise keep the kconfig as is, but I
would add a stub function for direct_ops_alloc() too.
> +#endif /* CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS */
> +
> static struct bpf_trampoline *bpf_trampoline_lookup(u64 key, unsigned long ip)
> {
> struct bpf_trampoline *tr;
> @@ -155,14 +316,11 @@ static struct bpf_trampoline *bpf_trampoline_lookup(u64 key, unsigned long ip)
> if (!tr)
> goto out;
> #ifdef CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS
> - tr->fops = kzalloc(sizeof(struct ftrace_ops), GFP_KERNEL);
> - if (!tr->fops) {
> + if (direct_ops_alloc(tr)) {
> kfree(tr);
> tr = NULL;
> goto out;
> }
> - tr->fops->private = tr;
> - tr->fops->ops_func = bpf_tramp_ftrace_ops_func;
> #endif
>
> tr->key = key;
> @@ -206,7 +364,7 @@ static int unregister_fentry(struct bpf_trampoline *tr, u32 orig_flags,
> int ret;
>
> if (tr->func.ftrace_managed)
> - ret = unregister_ftrace_direct(tr->fops, (long)old_addr, false);
> + ret = direct_ops_del(tr, old_addr);
Doesn't this need a wrapper too?
> else
> ret = bpf_trampoline_update_fentry(tr, orig_flags, old_addr, NULL);
>
> @@ -220,15 +378,7 @@ static int modify_fentry(struct bpf_trampoline *tr, u32 orig_flags,
> int ret;
>
> if (tr->func.ftrace_managed) {
> - unsigned long addr = (unsigned long) new_addr;
> -
> - if (bpf_trampoline_use_jmp(tr->flags))
> - addr = ftrace_jmp_set(addr);
> -
> - if (lock_direct_mutex)
> - ret = modify_ftrace_direct(tr->fops, addr);
> - else
> - ret = modify_ftrace_direct_nolock(tr->fops, addr);
> + ret = direct_ops_mod(tr, new_addr, lock_direct_mutex);
and this.
> } else {
> ret = bpf_trampoline_update_fentry(tr, orig_flags, old_addr,
> new_addr);
> @@ -251,15 +401,7 @@ static int register_fentry(struct bpf_trampoline *tr, void *new_addr)
> }
>
> if (tr->func.ftrace_managed) {
> - unsigned long addr = (unsigned long) new_addr;
> -
> - if (bpf_trampoline_use_jmp(tr->flags))
> - addr = ftrace_jmp_set(addr);
> -
> - ret = ftrace_set_filter_ip(tr->fops, (unsigned long)ip, 0, 1);
> - if (ret)
> - return ret;
> - ret = register_ftrace_direct(tr->fops, addr);
> + ret = direct_ops_add(tr, new_addr);
Ditto.
> } else {
> ret = bpf_trampoline_update_fentry(tr, 0, NULL, new_addr);
> }
> @@ -910,10 +1052,7 @@ void bpf_trampoline_put(struct bpf_trampoline *tr)
> */
> hlist_del(&tr->hlist_key);
> hlist_del(&tr->hlist_ip);
> - if (tr->fops) {
> - ftrace_free_filter(tr->fops);
> - kfree(tr->fops);
> - }
> + direct_ops_free(tr);
> kfree(tr);
> out:
> mutex_unlock(&trampoline_mutex);
> diff --git a/kernel/trace/Kconfig b/kernel/trace/Kconfig
> index 4661b9e606e0..1ad2e307c834 100644
> --- a/kernel/trace/Kconfig
> +++ b/kernel/trace/Kconfig
> @@ -50,6 +50,9 @@ config HAVE_DYNAMIC_FTRACE_WITH_REGS
> config HAVE_DYNAMIC_FTRACE_WITH_DIRECT_CALLS
> bool
>
> +config HAVE_SINGLE_FTRACE_DIRECT_OPS
> + bool
> +
Now you could add:
config SINGLE_FTRACE_DIRECT_OPS
bool
default y
depends on HAVE_SINGLE_FTRACE_DIRECT_OPS && DYNAMIC_FTRACE_WITH_DIRECT_CALLS
-- Steve
> config HAVE_DYNAMIC_FTRACE_WITH_CALL_OPS
> bool
>
> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> index c2054fe80de7..a0789727b971 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -2605,8 +2605,13 @@ unsigned long ftrace_find_rec_direct(unsigned long ip)
> static void call_direct_funcs(unsigned long ip, unsigned long pip,
> struct ftrace_ops *ops, struct ftrace_regs *fregs)
> {
> - unsigned long addr = READ_ONCE(ops->direct_call);
> + unsigned long addr;
>
> +#ifdef CONFIG_HAVE_SINGLE_FTRACE_DIRECT_OPS
> + addr = ftrace_find_rec_direct(ip);
> +#else
> + addr = READ_ONCE(ops->direct_call);
> +#endif
> if (!addr)
> return;
>
^ permalink raw reply
* Re: [PATCHv5 bpf-next 8/9] ftrace: Factor ftrace_ops ops_func interface
From: Steven Rostedt @ 2025-12-18 16:06 UTC (permalink / raw)
To: Jiri Olsa
Cc: Steven Rostedt, Florent Revest, Mark Rutland, bpf, linux-kernel,
linux-trace-kernel, linux-arm-kernel, Alexei Starovoitov,
Daniel Borkmann, Andrii Nakryiko, Menglong Dong, Song Liu
In-Reply-To: <20251215211402.353056-9-jolsa@kernel.org>
On Mon, 15 Dec 2025 22:14:01 +0100
Jiri Olsa <jolsa@kernel.org> wrote:
> We are going to remove "ftrace_ops->private == bpf_trampoline" setup
> in following changes.
>
> Adding ip argument to ftrace_ops_func_t callback function, so we can
> use it to look up the trampoline.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
This mostly touches bpf code so:
Acked-by: Steven Rostedt (Google) <rostedt@goodmis.org>
-- Steve
^ permalink raw reply
* Re: [PATCH 2/3] mm/khugepaged: use enum scan_result for result variables
From: Zi Yan @ 2025-12-18 15:54 UTC (permalink / raw)
To: David Hildenbrand (Red Hat)
Cc: Garg, Shivank, Andrew Morton, Lorenzo Stoakes, Baolin Wang,
Liam R . Howlett, Nico Pache, Ryan Roberts, Dev Jain, Barry Song,
Lance Yang, linux-mm, linux-kernel, linux-trace-kernel
In-Reply-To: <b75674c4-2439-4515-9b4c-bc3d40f76fc9@kernel.org>
On 18 Dec 2025, at 4:17, David Hildenbrand (Red Hat) wrote:
> On 12/18/25 06:40, Garg, Shivank wrote:
>>
>>
>> On 12/16/2025 9:08 PM, Zi Yan wrote:
>>> On 16 Dec 2025, at 6:11, Shivank Garg wrote:
>>>
>>
>>>
>>>> int collapse_pte_mapped_thp(struct mm_struct *mm, unsigned long addr,
>>>> bool install_pmd)
>>>> {
>>>
>>> And here.
>> Since this function is declared in khugepaged.h, I need to
>> move the enum definition to that header. I see two options for handling
>> the CONFIG_TRANSPARENT_HUGEPAGE check:
>>
>> 1. Define enum OUTSIDE the ifdef: This allows the static inline stub
>> to also return enum scan_result, keeping the API consistent.
>> 2. Define enum INSIDE the ifdef: The enum is hidden when THP is disabled,
>> forcing the stub to return int 0 instead.
>>
>> The only external caller (uprobes.c) of collapse_pte_mapped_thp currently
>> ignores the return value.
>
> Probably best to not expose that enum (especially when nobody cares ...) and instead expose a new void function for uprobe purposes.
>
> Maybe
>
> void collapse_pte_mapped_thp(...)
> {
> try_collapse_pte_mapped_thp();
> }
>
> Maybe something like that?
Sounds good to me.
Best Regards,
Yan, Zi
^ permalink raw reply
* Re: [PATCHv5 bpf-next 6/9] ftrace: Add update_ftrace_direct_mod function
From: Steven Rostedt @ 2025-12-18 15:41 UTC (permalink / raw)
To: Jiri Olsa
Cc: Steven Rostedt, Florent Revest, Mark Rutland, bpf, linux-kernel,
linux-trace-kernel, linux-arm-kernel, Alexei Starovoitov,
Daniel Borkmann, Andrii Nakryiko, Menglong Dong, Song Liu
In-Reply-To: <20251218101942.0716efd6@gandalf.local.home>
On Thu, 18 Dec 2025 10:19:42 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:
> On Mon, 15 Dec 2025 22:13:59 +0100
> Jiri Olsa <jolsa@kernel.org> wrote:
>
> > diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> > index 48dc0de5f2ce..95a38fb18ed7 100644
> > --- a/kernel/trace/ftrace.c
> > +++ b/kernel/trace/ftrace.c
> > @@ -6489,6 +6489,78 @@ int update_ftrace_direct_del(struct ftrace_ops *ops, struct ftrace_hash *hash)
> > return err;
> > }
> >
>
> Kerneldoc needed.
>
> > +int update_ftrace_direct_mod(struct ftrace_ops *ops, struct ftrace_hash *hash, bool do_direct_lock)
> > +{
> > + struct ftrace_func_entry *entry, *tmp;
> > + static struct ftrace_ops tmp_ops = {
> > + .func = ftrace_stub,
> > + .flags = FTRACE_OPS_FL_STUB,
> > + };
> > + struct ftrace_hash *orig_hash;
> > + unsigned long size, i;
> > + int err = -EINVAL;
> > +
> > + if (!hash_count(hash))
> > + return -EINVAL;
> > + if (check_direct_multi(ops))
> > + return -EINVAL;
> > + if (!(ops->flags & FTRACE_OPS_FL_ENABLED))
> > + return -EINVAL;
> > + if (direct_functions == EMPTY_HASH)
> > + return -EINVAL;
> > +
> > + if (do_direct_lock)
> > + mutex_lock(&direct_mutex);
>
> This optional taking of the direct_mutex lock needs some serious rationale
> and documentation.
>
> > +
> > + orig_hash = ops->func_hash ? ops->func_hash->filter_hash : NULL;
> > + if (!orig_hash)
> > + goto unlock;
> > +
> > + /* Enable the tmp_ops to have the same functions as the direct ops */
Add to the comments here:
* In order to modify the direct callers, all the functions need to
* first be calling the ftrace_ops_list_func() and not be connected
* to any direct callers. To do that, create a temporary ops that
* attach to the same functions as the direct ops, and attach that
* first. Then when adding the direct ops, it will use the
* ftrace_ops_list_func(), and this can safely modify what the
* direct ops call.
Or something like that. I want this code to be as clear as day to what it
is doing. In a year or two, we will forget, and this will be very confusing
to newcomers.
> > + ftrace_ops_init(&tmp_ops);
> > + tmp_ops.func_hash = ops->func_hash;
> > +
> > + err = register_ftrace_function_nolock(&tmp_ops);
> > + if (err)
> > + goto unlock;
> > +
> > + /*
> > + * Call __ftrace_hash_update_ipmodify() here, so that we can call
> > + * ops->ops_func for the ops. This is needed because the above
> > + * register_ftrace_function_nolock() worked on tmp_ops.
> > + */
> > + err = __ftrace_hash_update_ipmodify(ops, orig_hash, orig_hash, true);
> > + if (err)
> > + goto out;
> > +
> > + /*
> > + * Now the ftrace_ops_list_func() is called to do the direct callers.
> > + * We can safely change the direct functions attached to each entry.
> > + */
> > + mutex_lock(&ftrace_lock);
>
> I'm going to need some time staring at this code. It looks like it may be
> relying on some internals here.
>
> -- Steve
>
>
> > +
I would add a comment here:
/* Now update the direct functions to point to the new callbacks */
-- Steve
> > + size = 1 << hash->size_bits;
> > + for (i = 0; i < size; i++) {
> > + hlist_for_each_entry(entry, &hash->buckets[i], hlist) {
> > + tmp = __ftrace_lookup_ip(direct_functions, entry->ip);
> > + if (!tmp)
> > + continue;
> > + tmp->direct = entry->direct;
> > + }
> > + }
> > +
> > + mutex_unlock(&ftrace_lock);
> > +
> > +out:
> > + /* Removing the tmp_ops will add the updated direct callers to the functions */
> > + unregister_ftrace_function(&tmp_ops);
> > +
> > +unlock:
> > + if (do_direct_lock)
> > + mutex_unlock(&direct_mutex);
> > + return err;
> > +}
> > +
> > #endif /* CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS */
> >
> > /**
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox