Date: Thu, 23 May 2024 10:38:17 +0800
From: Yafang Shao <laoar.shao@...il.com>
To: Linus Torvalds <torvalds@...ux-foundation.org>, bpf <bpf@...r.kernel.org>
Cc: Tejun Heo <tj@...nel.org>, Jan Engelhardt <jengelh@...i.de>, Craig Small <csmall@....com.au>, 
	linux-kernel@...r.kernel.org, Lai Jiangshan <jiangshanlai@...il.com>
Subject: Re: [PATCH workqueue/for-6.10-fixes] workqueue: Refactor worker ID
 formatting and make wq_worker_comm() use full ID string

On Wed, May 22, 2024 at 2:06 AM Linus Torvalds
<torvalds@...ux-foundation.org> wrote:
>
> On Mon, 20 May 2024 at 19:34, Yafang Shao <laoar.shao@...il.com> wrote:
> >
> > We discussed extending it to 24 characters several years ago [0], but
> > some userspace tools might break.
>
> Well, the fact that we already expose names longer than 16 bytes in
> /proc means that at least *that* side of it could use an extended
> comm[] array.
>
> Yes, some other interfaces might want to still use a 16-byte limit as
> the length for the buffers they use (tracing?) but I suspect we could
> make the comm[] array easily bigger.

Indeed, the 16-byte limit is hard-coded in certain BPF code:

$ grep -r "comm\["  tools/testing/selftests/bpf/
tools/testing/selftests/bpf//prog_tests/ringbuf_multi.c: char comm[16];
tools/testing/selftests/bpf//prog_tests/sk_storage_tracing.c: char comm[16];
tools/testing/selftests/bpf//prog_tests/test_overhead.c: char comm[16] = {};
tools/testing/selftests/bpf//prog_tests/ringbuf.c: char comm[16];
tools/testing/selftests/bpf//progs/pyperf.h: char comm[TASK_COMM_LEN];
tools/testing/selftests/bpf//progs/dynptr_success.c: char comm[16];
tools/testing/selftests/bpf//progs/test_ringbuf.c: char comm[16];
tools/testing/selftests/bpf//progs/test_ringbuf_n.c: char comm[16];
tools/testing/selftests/bpf//progs/task_kfunc_success.c:
bpf_strncmp(&task->comm[8], 4, "foo");
tools/testing/selftests/bpf//progs/user_ringbuf_fail.c: char comm[16];
tools/testing/selftests/bpf//progs/test_ringbuf_map_key.c: char comm[16];
tools/testing/selftests/bpf//progs/test_core_reloc_kernel.c: char
comm[sizeof("test_progs")];
tools/testing/selftests/bpf//progs/test_core_reloc_kernel.c: char comm[16];
tools/testing/selftests/bpf//progs/dynptr_fail.c: char comm[16];
tools/testing/selftests/bpf//progs/strobemeta.h: char comm[TASK_COMM_LEN];
tools/testing/selftests/bpf//progs/core_reloc_types.h: char
comm[sizeof("test_progs")];
tools/testing/selftests/bpf//progs/core_reloc_types.h: char
comm[sizeof("test_progs")];
tools/testing/selftests/bpf//progs/test_skb_helpers.c: char comm[TEST_COMM_LEN];
tools/testing/selftests/bpf//progs/test_tracepoint.c: char
prev_comm[TASK_COMM_LEN];
tools/testing/selftests/bpf//progs/test_tracepoint.c: char
next_comm[TASK_COMM_LEN];
tools/testing/selftests/bpf//progs/test_ringbuf_multi.c: char comm[16];
tools/testing/selftests/bpf//progs/test_user_ringbuf.h: char comm[16];
tools/testing/selftests/bpf//progs/test_core_reloc_module.c: char
comm[sizeof("test_progs")];
tools/testing/selftests/bpf//progs/test_stacktrace_map.c: char
prev_comm[TASK_COMM_LEN];
tools/testing/selftests/bpf//progs/test_stacktrace_map.c: char
next_comm[TASK_COMM_LEN];
tools/testing/selftests/bpf//progs/test_sk_storage_tracing.c: char comm[16];
tools/testing/selftests/bpf//progs/test_sk_storage_tracing.c:char
task_comm[16] = "";

>
> But what I suspect we should do *first* is to try to get rid of a lot
> of the "current->comm" users. One of the most common uses is purely
> for printing, and we could actually just add a new '%p' pointer for
> printing the current name. That would allow our vsprintf() code to not
> just use tsk->comm, but to use the full_name for threads etc.
>
> So instead of
>
>    printf("%s ..", tsk->comm..);
>
> we could have something like
>
>    printf("%pc ..", tsk);
>
> to print the name of the task.

I believe it's a good start.

>
> That would get rid of a lot of the bare ->comm[] uses, and then the
> rest should probably use proper wrappers for copying the data (ie
> using 'get_task_comm()' etc).
>
> That would not only pick up the better names for printk and oopses, it
> would also make future cleanups simpler (for example, I'd love to get
> rid of the 'comm' name entirely, and replace it with 'exe_name[24]'
> and have the compiler just notice when somebody is trying to access
> 'comm' directly).

Some tools may flag the naming change. Below is a simple grep from
bcc-tools and bpftrace.

bcc $ grep -r "\->comm" tools/
tools//wakeuptime.py:    bpf_probe_read_kernel(&key.target,
sizeof(key.target), p->comm);
tools//bitesize.py:    bpf_probe_read_kernel(&key.name,
sizeof(key.name), args->comm);
tools//tcptracer.py:          evt4.comm[i] = p->comm[i];
tools//tcptracer.py:          evt6.comm[i] = p->comm[i];
tools//old/wakeuptime.py:    bpf_probe_read(&key.target,
sizeof(key.target), p->comm);
tools//old/oomkill.py:    bpf_probe_read(&data.tcomm,
sizeof(data.tcomm), p->comm);
tools//oomkill.py:    bpf_probe_read_kernel(&data.tcomm,
sizeof(data.tcomm), p->comm);
tools//runqslower.py:    bpf_probe_read_kernel_str(&data.prev_task,
sizeof(data.prev_task), prev->comm);
tools//runqslower.py:    bpf_probe_read_kernel_str(&data.task,
sizeof(data.task), next->comm);
tools//runqslower.py:    bpf_probe_read_kernel_str(&data.prev_task,
sizeof(data.prev_task), prev->comm);
tools//shmsnoop.py:    if (bpf_get_current_comm(&val->comm,
sizeof(val->comm)) != 0)
tools//sslsniff.py:        bpf_get_current_comm(&data->comm,
sizeof(data->comm));
tools//sslsniff.py:        bpf_get_current_comm(&data->comm,
sizeof(data->comm));
tools//fileslower.py:    bpf_probe_read_kernel(&data.comm,
sizeof(data.comm), valp->comm);
tools//mountsnoop.py:    bpf_probe_read_kernel_str(&event.enter.pcomm,
TASK_COMM_LEN, task->real_parent->comm);
tools//mountsnoop.py:    bpf_probe_read_kernel_str(&event.enter.pcomm,
TASK_COMM_LEN, task->real_parent->comm);
tools//gethostlatency.py:    bpf_probe_read_kernel(&data.comm,
sizeof(data.comm), valp->comm);
tools//opensnoop.py:    bpf_probe_read_kernel(&data.comm,
sizeof(data.comm), valp->comm);
tools//killsnoop.py:    bpf_probe_read_kernel(&data.comm,
sizeof(data.comm), valp->comm);

bpftrace $ grep -r "\->comm" tools/
tools//naptime.bt:     $task->real_parent->comm, pid, comm,
tools//oomkill.bt:     $oc->chosen->pid, $oc->chosen->comm, $oc->totalpages);


-- 
Regards
Yafang

Powered by blists - more mailing lists