Received-SPF: pass (google.com: domain of linux-kernel+bounces-186922-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Precedence: bulk
MIME-Version: 1.0
References: <o89373n4-3oq5-25qr-op7n-55p9657r96o8@vanv.qr> <CAHk-=wjxdtkFMB8BPYpU3JedjAsva3XXuzwxtzKoMwQ2e8zRzw@mail.gmail.com>
 <ZkvO-h7AsWnj4gaZ@slm.duckdns.org> <CALOAHbCYpV1ubO3Z3hjMWCQnSmGd9-KYARY29p9OnZxMhXKs4g@mail.gmail.com>
 <CAHk-=wj9gFa31JiMhwN6aw7gtwpkbAJ76fYvT5wLL_tMfRF77g@mail.gmail.com>
In-Reply-To: <CAHk-=wj9gFa31JiMhwN6aw7gtwpkbAJ76fYvT5wLL_tMfRF77g@mail.gmail.com>
From: Yafang Shao <laoar.shao@gmail.com>
Date: Thu, 23 May 2024 10:38:17 +0800
Message-ID: <CALOAHbAmHTGxTLVuR5N+apSOA29k08hky5KH9zZDY8yg2SAG8Q@mail.gmail.com>
Subject: Re: [PATCH workqueue/for-6.10-fixes] workqueue: Refactor worker ID
 formatting and make wq_worker_comm() use full ID string
To: Linus Torvalds <torvalds@linux-foundation.org>, bpf <bpf@vger.kernel.org>
Cc: Tejun Heo <tj@kernel.org>, Jan Engelhardt <jengelh@inai.de>, Craig Small <csmall@enc.com.au>, 
	linux-kernel@vger.kernel.org, Lai Jiangshan <jiangshanlai@gmail.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Wed, May 22, 2024 at 2:06=E2=80=AFAM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> On Mon, 20 May 2024 at 19:34, Yafang Shao <laoar.shao@gmail.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] =3D =
{};
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] =3D "";

>
> 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] =3D p->comm[i];
tools//tcptracer.py:          evt6.comm[i] =3D 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)) !=3D 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=
);


--=20
Regards
Yafang