Re: [PATCH v5 bpf-next 06/10] tracepoint: compute num_args at build time

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 3/26/18 9:35 AM, Steven Rostedt wrote:
On Mon, 26 Mar 2018 09:25:07 -0700
Alexei Starovoitov <ast@xxxxxx> wrote:

commit log of patch 6 states:

"for_each_tracepoint_range() api has no users inside the kernel.
Make it more useful with ability to stop for_each() loop depending
via callback return value.
In such form it's used in subsequent patch."

and in patch 7:

+static void *__find_tp(struct tracepoint *tp, void *priv)
+{
+       char *name = priv;
+
+       if (!strcmp(tp->name, name))
+               return tp;
+       return NULL;
+}
...
+       struct tracepoint *tp;
...
+       tp = for_each_kernel_tracepoint(__find_tp, tp_name);
+       if (!tp)
+               return -ENOENT;

still not obvious?

Please just create a new function called tracepoint_find_by_name(), and
use that. I don't see any benefit in using a for_each* function for
such a simple routine. Not to mention, you then don't need to know the
internals of a tracepoint in kernel/bpf/syscall.c.

It's a standard pattern in the kernel to stop for_each*() iterator
when callback function returns non-null.
Few examples:
idr_for_each
cfs_hash_for_each
and there are plenty more.

I don't mind to _rename_ for_each_kernel_tracepoint() into
tracepoint_find_by_name(), but keeping exported function
just to be used by out-of-tree modules would be wrong message for
the kernel community in general.
With my patch the for_each_kernel_tracepoint() will be used by bpf side
and out-of-tree can trivially hack their callbacks to keep working.
imo that's a better approach then renaming it.

--
To unsubscribe from this list: send the line "unsubscribe linux-api" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux