[PATCH] remoteproc: make rproc_class constant
Since commit 43a7206b0963 ("driver core: class: make class_register() take a const *"), the driver core allows for struct class to be in read-only memory, so move the rproc_class structure to be declared at build time placing it into read-only memory, instead of having to be dynamically allocated at boot time. Cc: Greg Kroah-Hartman Suggested-by: Greg Kroah-Hartman Signed-off-by: Ricardo B. Marliere --- drivers/remoteproc/remoteproc_internal.h | 2 +- drivers/remoteproc/remoteproc_sysfs.c| 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/remoteproc/remoteproc_internal.h b/drivers/remoteproc/remoteproc_internal.h index f62a82d71dfa..0cd09e67ac14 100644 --- a/drivers/remoteproc/remoteproc_internal.h +++ b/drivers/remoteproc/remoteproc_internal.h @@ -72,7 +72,7 @@ void rproc_init_debugfs(void); void rproc_exit_debugfs(void); /* from remoteproc_sysfs.c */ -extern struct class rproc_class; +extern const struct class rproc_class; int rproc_init_sysfs(void); void rproc_exit_sysfs(void); diff --git a/drivers/remoteproc/remoteproc_sysfs.c b/drivers/remoteproc/remoteproc_sysfs.c index 8c7ea8922638..138e752c5e4e 100644 --- a/drivers/remoteproc/remoteproc_sysfs.c +++ b/drivers/remoteproc/remoteproc_sysfs.c @@ -254,7 +254,7 @@ static const struct attribute_group *rproc_devgroups[] = { NULL }; -struct class rproc_class = { +const struct class rproc_class = { .name = "remoteproc", .dev_groups = rproc_devgroups, }; --- base-commit: 8b46dc5cfa5ffea279aed0fc05dc4b1c39a51517 change-id: 20240305-class_cleanup-remoteproc2-f1212934f990 Best regards, -- Ricardo B. Marliere
[PATCH] rpmsg: core: make rpmsg_class constant
Since commit 43a7206b0963 ("driver core: class: make class_register() take a const *"), the driver core allows for struct class to be in read-only memory, so move the rpmsg_class structure to be declared at build time placing it into read-only memory, instead of having to be dynamically allocated at boot time. Cc: Greg Kroah-Hartman Suggested-by: Greg Kroah-Hartman Signed-off-by: Ricardo B. Marliere --- drivers/rpmsg/rpmsg_char.c | 2 +- drivers/rpmsg/rpmsg_core.c | 16 +--- drivers/rpmsg/rpmsg_ctrl.c | 2 +- drivers/rpmsg/rpmsg_internal.h | 2 +- 4 files changed, 12 insertions(+), 10 deletions(-) diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c index 1cb8d7474428..d7a342510902 100644 --- a/drivers/rpmsg/rpmsg_char.c +++ b/drivers/rpmsg/rpmsg_char.c @@ -423,7 +423,7 @@ static struct rpmsg_eptdev *rpmsg_chrdev_eptdev_alloc(struct rpmsg_device *rpdev init_waitqueue_head(&eptdev->readq); device_initialize(dev); - dev->class = rpmsg_class; + dev->class = &rpmsg_class; dev->parent = parent; dev->groups = rpmsg_eptdev_groups; dev_set_drvdata(dev, eptdev); diff --git a/drivers/rpmsg/rpmsg_core.c b/drivers/rpmsg/rpmsg_core.c index 4295c01a2861..0fa08266404d 100644 --- a/drivers/rpmsg/rpmsg_core.c +++ b/drivers/rpmsg/rpmsg_core.c @@ -20,7 +20,9 @@ #include "rpmsg_internal.h" -struct class *rpmsg_class; +const struct class rpmsg_class = { + .name = "rpmsg", +}; EXPORT_SYMBOL(rpmsg_class); /** @@ -715,16 +717,16 @@ static int __init rpmsg_init(void) { int ret; - rpmsg_class = class_create("rpmsg"); - if (IS_ERR(rpmsg_class)) { - pr_err("failed to create rpmsg class\n"); - return PTR_ERR(rpmsg_class); + ret = class_register(&rpmsg_class); + if (ret) { + pr_err("failed to register rpmsg class\n"); + return ret; } ret = bus_register(&rpmsg_bus); if (ret) { pr_err("failed to register rpmsg bus: %d\n", ret); - class_destroy(rpmsg_class); + class_destroy(&rpmsg_class); } return ret; } @@ -733,7 +735,7 @@ postcore_initcall(rpmsg_init); static void __exit rpmsg_fini(void) { bus_unregister(&rpmsg_bus); - class_destroy(rpmsg_class); + class_destroy(&rpmsg_class); } module_exit(rpmsg_fini); diff --git a/drivers/rpmsg/rpmsg_ctrl.c b/drivers/rpmsg/rpmsg_ctrl.c index c312794ba4b3..28f57945ccd9 100644 --- a/drivers/rpmsg/rpmsg_ctrl.c +++ b/drivers/rpmsg/rpmsg_ctrl.c @@ -150,7 +150,7 @@ static int rpmsg_ctrldev_probe(struct rpmsg_device *rpdev) dev = &ctrldev->dev; device_initialize(dev); dev->parent = &rpdev->dev; - dev->class = rpmsg_class; + dev->class = &rpmsg_class; mutex_init(&ctrldev->ctrl_lock); cdev_init(&ctrldev->cdev, &rpmsg_ctrldev_fops); diff --git a/drivers/rpmsg/rpmsg_internal.h b/drivers/rpmsg/rpmsg_internal.h index b950d6f790a3..a3ba768138f1 100644 --- a/drivers/rpmsg/rpmsg_internal.h +++ b/drivers/rpmsg/rpmsg_internal.h @@ -18,7 +18,7 @@ #define to_rpmsg_device(d) container_of(d, struct rpmsg_device, dev) #define to_rpmsg_driver(d) container_of(d, struct rpmsg_driver, drv) -extern struct class *rpmsg_class; +extern const struct class rpmsg_class; /** * struct rpmsg_device_ops - indirection table for the rpmsg_device operations --- base-commit: b03aa6d4e9a74c4289929b6cf3c6bcc80270682d change-id: 20240305-class_cleanup-remoteproc-2b53e26b2817 Best regards, -- Ricardo B. Marliere
[PATCH] dax: constify the struct device_type usage
Since commit aed65af1cc2f ("drivers: make device_type const"), the driver core can properly handle constant struct device_type. Move the dax_mapping_type variable to be a constant structure as well, placing it into read-only memory which can not be modified at runtime. Cc: Greg Kroah-Hartman Signed-off-by: Ricardo B. Marliere --- drivers/dax/bus.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/dax/bus.c b/drivers/dax/bus.c index 1ff1ab5fa105..e265ba019785 100644 --- a/drivers/dax/bus.c +++ b/drivers/dax/bus.c @@ -763,7 +763,7 @@ static const struct attribute_group *dax_mapping_attribute_groups[] = { NULL, }; -static struct device_type dax_mapping_type = { +static const struct device_type dax_mapping_type = { .release = dax_mapping_release, .groups = dax_mapping_attribute_groups, }; --- base-commit: b401b621758e46812da61fa58a67c3fd8d91de0d change-id: 20240219-device_cleanup-dax-d82fd0c67ffd Best regards, -- Ricardo B. Marliere
[PATCH] virtio: make virtio_bus const
Now that the driver core can properly handle constant struct bus_type, move the virtio_bus variable to be a constant structure as well, placing it into read-only memory which can not be modified at runtime. Cc: Greg Kroah-Hartman Suggested-by: Greg Kroah-Hartman Signed-off-by: Ricardo B. Marliere --- drivers/virtio/virtio.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c index f4080692b351..a68e6556f71b 100644 --- a/drivers/virtio/virtio.c +++ b/drivers/virtio/virtio.c @@ -353,7 +353,7 @@ static void virtio_dev_remove(struct device *_d) of_node_put(dev->dev.of_node); } -static struct bus_type virtio_bus = { +static const struct bus_type virtio_bus = { .name = "virtio", .match = virtio_dev_match, .dev_groups = virtio_dev_groups, --- base-commit: 41b9fb381a486360b2daaec0c7480f8e3ff72bc7 change-id: 20240204-bus_cleanup-virtio-9f076c3cb068 Best regards, -- Ricardo B. Marliere
[PATCH] vdpa: make vdpa_bus const
Now that the driver core can properly handle constant struct bus_type, move the vdpa_bus variable to be a constant structure as well, placing it into read-only memory which can not be modified at runtime. Cc: Greg Kroah-Hartman Suggested-by: Greg Kroah-Hartman Signed-off-by: Ricardo B. Marliere --- drivers/vdpa/vdpa.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c index d0695680b282..c73c06102e7e 100644 --- a/drivers/vdpa/vdpa.c +++ b/drivers/vdpa/vdpa.c @@ -115,7 +115,7 @@ static const struct attribute_group vdpa_dev_group = { }; __ATTRIBUTE_GROUPS(vdpa_dev); -static struct bus_type vdpa_bus = { +static const struct bus_type vdpa_bus = { .name = "vdpa", .dev_groups = vdpa_dev_groups, .match = vdpa_dev_match, --- base-commit: 41b9fb381a486360b2daaec0c7480f8e3ff72bc7 change-id: 20240204-bus_cleanup-vdpa-8e19b86329f6 Best regards, -- Ricardo B. Marliere
[PATCH] rpmsg: core: make rpmsg_bus const
Now that the driver core can properly handle constant struct bus_type, move the rpmsg_bus variable to be a constant structure as well, placing it into read-only memory which can not be modified at runtime. Cc: Greg Kroah-Hartman Suggested-by: Greg Kroah-Hartman Signed-off-by: Ricardo B. Marliere --- drivers/rpmsg/rpmsg_core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/rpmsg/rpmsg_core.c b/drivers/rpmsg/rpmsg_core.c index 8abc7d022ff7..4295c01a2861 100644 --- a/drivers/rpmsg/rpmsg_core.c +++ b/drivers/rpmsg/rpmsg_core.c @@ -605,7 +605,7 @@ static void rpmsg_dev_remove(struct device *dev) rpmsg_destroy_ept(rpdev->ept); } -static struct bus_type rpmsg_bus = { +static const struct bus_type rpmsg_bus = { .name = "rpmsg", .match = rpmsg_dev_match, .dev_groups = rpmsg_dev_groups, --- base-commit: 80255b24efbe83a6a01600484b6959259a30ded5 change-id: 20240204-bus_cleanup-rpmsg-1a5f6ab69a24 Best regards, -- Ricardo B. Marliere
[PATCH] nvdimm: make nvdimm_bus_type const
Now that the driver core can properly handle constant struct bus_type, move the nvdimm_bus_type variable to be a constant structure as well, placing it into read-only memory which can not be modified at runtime. Cc: Greg Kroah-Hartman Suggested-by: Greg Kroah-Hartman Signed-off-by: Ricardo B. Marliere --- drivers/nvdimm/bus.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/nvdimm/bus.c b/drivers/nvdimm/bus.c index ef3d0f83318b..508aed017ddc 100644 --- a/drivers/nvdimm/bus.c +++ b/drivers/nvdimm/bus.c @@ -271,7 +271,7 @@ EXPORT_SYMBOL_GPL(nvdimm_clear_poison); static int nvdimm_bus_match(struct device *dev, struct device_driver *drv); -static struct bus_type nvdimm_bus_type = { +static const struct bus_type nvdimm_bus_type = { .name = "nd", .uevent = nvdimm_bus_uevent, .match = nvdimm_bus_match, --- base-commit: a085a5eb6594a3ebe5c275e9c2c2d341f686c23c change-id: 20240204-bus_cleanup-nvdimm-91771693bd4d Best regards, -- Ricardo B. Marliere
[PATCH] device-dax: make dax_bus_type const
Now that the driver core can properly handle constant struct bus_type, move the dax_bus_type variable to be a constant structure as well, placing it into read-only memory which can not be modified at runtime. Cc: Greg Kroah-Hartman Suggested-by: Greg Kroah-Hartman Signed-off-by: Ricardo B. Marliere --- drivers/dax/bus.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/dax/bus.c b/drivers/dax/bus.c index 1659b787b65f..fe0a415c854d 100644 --- a/drivers/dax/bus.c +++ b/drivers/dax/bus.c @@ -222,7 +222,7 @@ static void dax_bus_remove(struct device *dev) dax_drv->remove(dev_dax); } -static struct bus_type dax_bus_type = { +static const struct bus_type dax_bus_type = { .name = "dax", .uevent = dax_bus_uevent, .match = dax_bus_match, --- base-commit: 73bf93edeeea866b0b6efbc8d2595bdaaba7f1a5 change-id: 20240204-bus_cleanup-dax-52c34f72615f Best regards, -- Ricardo B. Marliere
[PATCH v3 3/3] tracing: convert __trace_seq_init to use WARN_ON_ONCE
The initialization of trace_seq buffers is done elsewhere and therefore __trace_seq_init should yield a warning if it has to actually initialize the buffer. Suggested-by: Steven Rostedt Signed-off-by: Ricardo B. Marliere --- kernel/trace/trace_seq.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/kernel/trace/trace_seq.c b/kernel/trace/trace_seq.c index 741b2f3d76c0..3c7a7d903b54 100644 --- a/kernel/trace/trace_seq.c +++ b/kernel/trace/trace_seq.c @@ -32,7 +32,7 @@ */ static inline void __trace_seq_init(struct trace_seq *s) { - if (unlikely(!s->seq.size)) + if (WARN_ON_ONCE(!s->seq.size)) trace_seq_init(s); } -- 2.43.0
[PATCH v3 2/3] tracing: add trace_seq_reset function
Currently, trace_seq_init may be called many times with the intent of resetting the buffer. Add a function trace_seq_reset that does that and replace the relevant occurrences to use it instead. Suggested-by: Steven Rostedt Signed-off-by: Ricardo B. Marliere --- include/linux/trace_seq.h| 11 +++ include/trace/trace_events.h | 2 +- kernel/trace/trace.c | 10 +- kernel/trace/trace_output.c | 2 +- kernel/trace/trace_seq.c | 2 +- 5 files changed, 19 insertions(+), 8 deletions(-) diff --git a/include/linux/trace_seq.h b/include/linux/trace_seq.h index 9ec229dfddaa..d3fa41001813 100644 --- a/include/linux/trace_seq.h +++ b/include/linux/trace_seq.h @@ -29,6 +29,17 @@ trace_seq_init(struct trace_seq *s) s->readpos = 0; } +static inline void +trace_seq_reset(struct trace_seq *s) +{ + if (WARN_ON_ONCE(!s->seq.size)) + seq_buf_init(&s->seq, s->buffer, TRACE_SEQ_BUFFER_SIZE); + else + seq_buf_clear(&s->seq); + s->full = 0; + s->readpos = 0; +} + /** * trace_seq_used - amount of actual data written to buffer * @s: trace sequence descriptor diff --git a/include/trace/trace_events.h b/include/trace/trace_events.h index c2f9cabf154d..2bc79998e5ab 100644 --- a/include/trace/trace_events.h +++ b/include/trace/trace_events.h @@ -227,7 +227,7 @@ trace_raw_output_##call(struct trace_iterator *iter, int flags, \ \ field = (typeof(field))entry; \ \ - trace_seq_init(p); \ + trace_seq_reset(p); \ return trace_output_call(iter, #call, print); \ } \ static struct trace_event_functions trace_event_type_funcs_##call = { \ diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c index 9eddf8168df2..9827700d0164 100644 --- a/kernel/trace/trace.c +++ b/kernel/trace/trace.c @@ -2928,7 +2928,7 @@ static void output_printk(struct trace_event_buffer *fbuffer) event = &fbuffer->trace_file->event_call->event; raw_spin_lock_irqsave(&tracepoint_iter_lock, flags); - trace_seq_init(&iter->seq); + trace_seq_reset(&iter->seq); iter->ent = fbuffer->entry; event_call->event.funcs->trace(iter, 0, event); trace_seq_putc(&iter->seq, 0); @@ -6925,7 +6925,7 @@ tracing_read_pipe(struct file *filp, char __user *ubuf, if (sret != -EBUSY) goto out; - trace_seq_init(&iter->seq); + trace_seq_reset(&iter->seq); if (iter->trace->read) { sret = iter->trace->read(iter, filp, ubuf, cnt, ppos); @@ -6998,7 +6998,7 @@ tracing_read_pipe(struct file *filp, char __user *ubuf, /* Now copy what we have to the user */ sret = trace_seq_to_user(&iter->seq, ubuf, cnt); if (iter->seq.readpos >= trace_seq_used(&iter->seq)) - trace_seq_init(&iter->seq); + trace_seq_reset(&iter->seq); /* * If there was nothing to send to user, in spite of consuming trace @@ -7130,7 +7130,7 @@ static ssize_t tracing_splice_read_pipe(struct file *filp, spd.partial[i].offset = 0; spd.partial[i].len = trace_seq_used(&iter->seq); - trace_seq_init(&iter->seq); + trace_seq_reset(&iter->seq); } trace_access_unlock(iter->cpu_file); @@ -10282,7 +10282,7 @@ trace_printk_seq(struct trace_seq *s) printk(KERN_TRACE "%s", s->buffer); - trace_seq_init(s); + trace_seq_reset(s); } void trace_init_global_iter(struct trace_iterator *iter) diff --git a/kernel/trace/trace_output.c b/kernel/trace/trace_output.c index 3e7fa44dc2b2..c949e7736618 100644 --- a/kernel/trace/trace_output.c +++ b/kernel/trace/trace_output.c @@ -308,7 +308,7 @@ int trace_raw_output_prep(struct trace_iterator *iter, return TRACE_TYPE_UNHANDLED; } - trace_seq_init(p); + trace_seq_reset(p); trace_seq_printf(s, "%s: ", trace_event_name(event)); return trace_handle_return(s); diff --git a/kernel/trace/trace_seq.c b/kernel/trace/trace_seq.c index c158d65a8a88..741b2f3d76c0 100644 --- a/kernel/trace/trace_seq.c +++ b/kernel/trace/trace_seq.c @@ -59,7 +59,7 @@ int trace_print_seq(struct seq_file *m, struct trace_seq *s) * do something else with the contents. */ if (!ret) - trace_seq_init(s); + trace_seq_reset(s); return ret; } -- 2.43.0
[PATCH v3 1/3] tracing: initialize trace_seq buffers
In order to extend trace_seq into being dynamic, the struct trace_seq will no longer be valid if simply set to zero. Call trace_seq_init() for all trace_seq when they are first created. Suggested-by: Steven Rostedt Signed-off-by: Ricardo B. Marliere --- kernel/trace/trace.c | 14 ++ 1 file changed, 14 insertions(+) diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c index 46dbe22121e9..9eddf8168df2 100644 --- a/kernel/trace/trace.c +++ b/kernel/trace/trace.c @@ -4889,6 +4889,9 @@ __tracing_open(struct inode *inode, struct file *file, bool snapshot) mutex_unlock(&trace_types_lock); + trace_seq_init(&iter->seq); + trace_seq_init(&iter->tmp_seq); + return iter; fail: @@ -6770,6 +6773,7 @@ static int tracing_open_pipe(struct inode *inode, struct file *filp) } trace_seq_init(&iter->seq); + trace_seq_init(&iter->tmp_seq); iter->trace = tr->current_trace; if (!alloc_cpumask_var(&iter->started, GFP_KERNEL)) { @@ -6947,6 +6951,7 @@ tracing_read_pipe(struct file *filp, char __user *ubuf, trace_iterator_reset(iter); cpumask_clear(iter->started); trace_seq_init(&iter->seq); + trace_seq_init(&iter->tmp_seq); trace_event_read_lock(); trace_access_lock(iter->cpu_file); @@ -8277,6 +8282,9 @@ static int tracing_buffers_open(struct inode *inode, struct file *filp) if (ret < 0) trace_array_put(tr); + trace_seq_init(&info->iter.seq); + trace_seq_init(&info->iter.tmp_seq); + return ret; } @@ -10300,6 +10308,9 @@ void trace_init_global_iter(struct trace_iterator *iter) iter->temp_size = STATIC_TEMP_BUF_SIZE; iter->fmt = static_fmt_buf; iter->fmt_size = STATIC_FMT_BUF_SIZE; + + trace_seq_init(&iter->seq); + trace_seq_init(&iter->tmp_seq); } void ftrace_dump(enum ftrace_dump_mode oops_dump_mode) @@ -10712,6 +10723,9 @@ void __init early_trace_init(void) tracepoint_printk = 0; else static_key_enable(&tracepoint_printk_key.key); + + trace_seq_init(&tracepoint_print_iter->seq); + trace_seq_init(&tracepoint_print_iter->tmp_seq); } tracer_alloc_buffers(); -- 2.43.0
[PATCH v3 0/3] tracing: add trace_seq_reset function
This series is a prerequisite for a later effort of making trace_seq more flexible about its buffer size. To achieve that, initializing and resetting the buffers need to be differentiated. Changes in v3: - Reordered commits so it doesn't produce a failing build in-between. - Improved changelogs. Changes in v2: - Added a WARN_ON_ONCE to __trace_seq_init to catch possible misuses. - Properly initialized trace_seq buffers. Ricardo B. Marliere (3): tracing: initialize trace_seq buffers tracing: add trace_seq_reset function tracing: convert __trace_seq_init to use WARN_ON_ONCE include/linux/trace_seq.h| 11 +++ include/trace/trace_events.h | 2 +- kernel/trace/trace.c | 24 +++- kernel/trace/trace_output.c | 2 +- kernel/trace/trace_seq.c | 4 ++-- 5 files changed, 34 insertions(+), 9 deletions(-) -- 2.43.0
Re: Re: [PATCH v2 2/3] tracing: initialize trace_seq buffers
Hi Steve, On 25 Jan 15:44, Steven Rostedt wrote: > On Thu, 25 Jan 2024 17:16:21 -0300 > "Ricardo B. Marliere" wrote: > > > Now that trace_seq_reset have been created, correct the places where the > > buffers need to be initialized. > > This patch would need to come first. You don't ever want to intentionally > create a broken kernel. Indeed, sorry for the lack of attention. > > Also, the change log should be: > > In order to extend trace_seq into being dynamic, the struct trace_seq > will no longer be valid if simply set to zero. Call trace_seq_init() for > all trace_seq when they are first created. Ack. > > > > > Suggested-by: Steven Rostedt > > Signed-off-by: Ricardo B. Marliere > > --- > > kernel/trace/trace.c | 14 ++ > > 1 file changed, 14 insertions(+) > > > > You also need to initialize iter.seq in ftrace_dump() Thanks a lot for reviewing, I will send a v3. - Ricardo > > -- Steve > > > diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c > > index d4c55d3e21c2..9827700d0164 100644 > > --- a/kernel/trace/trace.c > > +++ b/kernel/trace/trace.c > > @@ -4889,6 +4889,9 @@ __tracing_open(struct inode *inode, struct file > > *file, bool snapshot) > > > > mutex_unlock(&trace_types_lock); > > > > + trace_seq_init(&iter->seq); > > + trace_seq_init(&iter->tmp_seq); > > + > > return iter; > > > > fail: > > @@ -6770,6 +6773,7 @@ static int tracing_open_pipe(struct inode *inode, > > struct file *filp) > > } > > > > trace_seq_init(&iter->seq); > > + trace_seq_init(&iter->tmp_seq); > > iter->trace = tr->current_trace; > > > > if (!alloc_cpumask_var(&iter->started, GFP_KERNEL)) { > > @@ -6947,6 +6951,7 @@ tracing_read_pipe(struct file *filp, char __user > > *ubuf, > > trace_iterator_reset(iter); > > cpumask_clear(iter->started); > > trace_seq_init(&iter->seq); > > + trace_seq_init(&iter->tmp_seq); > > > > trace_event_read_lock(); > > trace_access_lock(iter->cpu_file); > > @@ -8277,6 +8282,9 @@ static int tracing_buffers_open(struct inode *inode, > > struct file *filp) > > if (ret < 0) > > trace_array_put(tr); > > > > + trace_seq_init(&info->iter.seq); > > + trace_seq_init(&info->iter.tmp_seq); > > + > > return ret; > > } > > > > @@ -10300,6 +10308,9 @@ void trace_init_global_iter(struct trace_iterator > > *iter) > > iter->temp_size = STATIC_TEMP_BUF_SIZE; > > iter->fmt = static_fmt_buf; > > iter->fmt_size = STATIC_FMT_BUF_SIZE; > > + > > + trace_seq_init(&iter->seq); > > + trace_seq_init(&iter->tmp_seq); > > } > > > > void ftrace_dump(enum ftrace_dump_mode oops_dump_mode) > > @@ -10712,6 +10723,9 @@ void __init early_trace_init(void) > > tracepoint_printk = 0; > > else > > static_key_enable(&tracepoint_printk_key.key); > > + > > + trace_seq_init(&tracepoint_print_iter->seq); > > + trace_seq_init(&tracepoint_print_iter->tmp_seq); > > } > > tracer_alloc_buffers(); > > >
[PATCH v2 3/3] tracing: convert __trace_seq_init to use WARN_ON_ONCE
The initialization of trace_seq buffers are done elsewhere and therefore __trace_seq_init should yield a warning if it has to actually initialize the buffer. Suggested-by: Steven Rostedt Signed-off-by: Ricardo B. Marliere --- kernel/trace/trace_seq.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/kernel/trace/trace_seq.c b/kernel/trace/trace_seq.c index 741b2f3d76c0..3c7a7d903b54 100644 --- a/kernel/trace/trace_seq.c +++ b/kernel/trace/trace_seq.c @@ -32,7 +32,7 @@ */ static inline void __trace_seq_init(struct trace_seq *s) { - if (unlikely(!s->seq.size)) + if (WARN_ON_ONCE(!s->seq.size)) trace_seq_init(s); } -- 2.43.0
[PATCH v2 2/3] tracing: initialize trace_seq buffers
Now that trace_seq_reset have been created, correct the places where the buffers need to be initialized. Suggested-by: Steven Rostedt Signed-off-by: Ricardo B. Marliere --- kernel/trace/trace.c | 14 ++ 1 file changed, 14 insertions(+) diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c index d4c55d3e21c2..9827700d0164 100644 --- a/kernel/trace/trace.c +++ b/kernel/trace/trace.c @@ -4889,6 +4889,9 @@ __tracing_open(struct inode *inode, struct file *file, bool snapshot) mutex_unlock(&trace_types_lock); + trace_seq_init(&iter->seq); + trace_seq_init(&iter->tmp_seq); + return iter; fail: @@ -6770,6 +6773,7 @@ static int tracing_open_pipe(struct inode *inode, struct file *filp) } trace_seq_init(&iter->seq); + trace_seq_init(&iter->tmp_seq); iter->trace = tr->current_trace; if (!alloc_cpumask_var(&iter->started, GFP_KERNEL)) { @@ -6947,6 +6951,7 @@ tracing_read_pipe(struct file *filp, char __user *ubuf, trace_iterator_reset(iter); cpumask_clear(iter->started); trace_seq_init(&iter->seq); + trace_seq_init(&iter->tmp_seq); trace_event_read_lock(); trace_access_lock(iter->cpu_file); @@ -8277,6 +8282,9 @@ static int tracing_buffers_open(struct inode *inode, struct file *filp) if (ret < 0) trace_array_put(tr); + trace_seq_init(&info->iter.seq); + trace_seq_init(&info->iter.tmp_seq); + return ret; } @@ -10300,6 +10308,9 @@ void trace_init_global_iter(struct trace_iterator *iter) iter->temp_size = STATIC_TEMP_BUF_SIZE; iter->fmt = static_fmt_buf; iter->fmt_size = STATIC_FMT_BUF_SIZE; + + trace_seq_init(&iter->seq); + trace_seq_init(&iter->tmp_seq); } void ftrace_dump(enum ftrace_dump_mode oops_dump_mode) @@ -10712,6 +10723,9 @@ void __init early_trace_init(void) tracepoint_printk = 0; else static_key_enable(&tracepoint_printk_key.key); + + trace_seq_init(&tracepoint_print_iter->seq); + trace_seq_init(&tracepoint_print_iter->tmp_seq); } tracer_alloc_buffers(); -- 2.43.0
[PATCH v2 1/3] tracing: add trace_seq_reset function
Currently, trace_seq_init may be called many times with the intent of resetting the buffer. Add a function trace_seq_reset that does that and replace the relevant occurrences to use it instead. Suggested-by: Steven Rostedt Signed-off-by: Ricardo B. Marliere --- include/linux/trace_seq.h| 11 +++ include/trace/trace_events.h | 2 +- kernel/trace/trace.c | 10 +- kernel/trace/trace_output.c | 2 +- kernel/trace/trace_seq.c | 2 +- 5 files changed, 19 insertions(+), 8 deletions(-) diff --git a/include/linux/trace_seq.h b/include/linux/trace_seq.h index 9ec229dfddaa..d3fa41001813 100644 --- a/include/linux/trace_seq.h +++ b/include/linux/trace_seq.h @@ -29,6 +29,17 @@ trace_seq_init(struct trace_seq *s) s->readpos = 0; } +static inline void +trace_seq_reset(struct trace_seq *s) +{ + if (WARN_ON_ONCE(!s->seq.size)) + seq_buf_init(&s->seq, s->buffer, TRACE_SEQ_BUFFER_SIZE); + else + seq_buf_clear(&s->seq); + s->full = 0; + s->readpos = 0; +} + /** * trace_seq_used - amount of actual data written to buffer * @s: trace sequence descriptor diff --git a/include/trace/trace_events.h b/include/trace/trace_events.h index c2f9cabf154d..2bc79998e5ab 100644 --- a/include/trace/trace_events.h +++ b/include/trace/trace_events.h @@ -227,7 +227,7 @@ trace_raw_output_##call(struct trace_iterator *iter, int flags, \ \ field = (typeof(field))entry; \ \ - trace_seq_init(p); \ + trace_seq_reset(p); \ return trace_output_call(iter, #call, print); \ } \ static struct trace_event_functions trace_event_type_funcs_##call = { \ diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c index 46dbe22121e9..d4c55d3e21c2 100644 --- a/kernel/trace/trace.c +++ b/kernel/trace/trace.c @@ -2928,7 +2928,7 @@ static void output_printk(struct trace_event_buffer *fbuffer) event = &fbuffer->trace_file->event_call->event; raw_spin_lock_irqsave(&tracepoint_iter_lock, flags); - trace_seq_init(&iter->seq); + trace_seq_reset(&iter->seq); iter->ent = fbuffer->entry; event_call->event.funcs->trace(iter, 0, event); trace_seq_putc(&iter->seq, 0); @@ -6921,7 +6921,7 @@ tracing_read_pipe(struct file *filp, char __user *ubuf, if (sret != -EBUSY) goto out; - trace_seq_init(&iter->seq); + trace_seq_reset(&iter->seq); if (iter->trace->read) { sret = iter->trace->read(iter, filp, ubuf, cnt, ppos); @@ -6993,7 +6993,7 @@ tracing_read_pipe(struct file *filp, char __user *ubuf, /* Now copy what we have to the user */ sret = trace_seq_to_user(&iter->seq, ubuf, cnt); if (iter->seq.readpos >= trace_seq_used(&iter->seq)) - trace_seq_init(&iter->seq); + trace_seq_reset(&iter->seq); /* * If there was nothing to send to user, in spite of consuming trace @@ -7125,7 +7125,7 @@ static ssize_t tracing_splice_read_pipe(struct file *filp, spd.partial[i].offset = 0; spd.partial[i].len = trace_seq_used(&iter->seq); - trace_seq_init(&iter->seq); + trace_seq_reset(&iter->seq); } trace_access_unlock(iter->cpu_file); @@ -10274,7 +10274,7 @@ trace_printk_seq(struct trace_seq *s) printk(KERN_TRACE "%s", s->buffer); - trace_seq_init(s); + trace_seq_reset(s); } void trace_init_global_iter(struct trace_iterator *iter) diff --git a/kernel/trace/trace_output.c b/kernel/trace/trace_output.c index 3e7fa44dc2b2..c949e7736618 100644 --- a/kernel/trace/trace_output.c +++ b/kernel/trace/trace_output.c @@ -308,7 +308,7 @@ int trace_raw_output_prep(struct trace_iterator *iter, return TRACE_TYPE_UNHANDLED; } - trace_seq_init(p); + trace_seq_reset(p); trace_seq_printf(s, "%s: ", trace_event_name(event)); return trace_handle_return(s); diff --git a/kernel/trace/trace_seq.c b/kernel/trace/trace_seq.c index c158d65a8a88..741b2f3d76c0 100644 --- a/kernel/trace/trace_seq.c +++ b/kernel/trace/trace_seq.c @@ -59,7 +59,7 @@ int trace_print_seq(struct seq_file *m, struct trace_seq *s) * do something else with the contents. */ if (!ret) - trace_seq_init(s); + trace_seq_reset(s); return ret; } -- 2.43.0
[PATCH v2 0/3] add trace_seq_reset function
This series is a prerequisite for a later effort of making trace_seq more flexible about its buffer size. To achieve that, initializing and resetting the buffers need to be differentiated. Ricardo B. Marliere (3): tracing: add trace_seq_reset function tracing: initialize trace_seq buffers tracing: convert __trace_seq_init to use WARN_ON_ONCE include/linux/trace_seq.h| 11 +++ include/trace/trace_events.h | 2 +- kernel/trace/trace.c | 24 +++- kernel/trace/trace_output.c | 2 +- kernel/trace/trace_seq.c | 4 ++-- 5 files changed, 34 insertions(+), 9 deletions(-) -- 2.43.0
Re: [PATCH] tracing: add trace_seq_reset function
On 22 Jan 17:10, Steven Rostedt wrote: > On Mon, 22 Jan 2024 15:22:25 -0300 > "Ricardo B. Marliere" wrote: > > > Currently, trace_seq_init may be called many times with the intent of > > resetting the buffer. Add a function trace_seq_reset that does that and > > replace the relevant occurrences to use it instead. > > > > Hi Ricardo! > > It's also OK to add to the commit log that the goal of this is to later > extend trace_seq to be more flexible in the size of the buffer it holds. To > do that, we need to differentiate between initializing a trace_seq and just > resetting it. > Hi, Steve Certainly. I also forgot to add your Suggested-by. > > > Signed-off-by: Ricardo B. Marliere > > --- > > include/linux/trace_seq.h| 8 > > include/trace/trace_events.h | 2 +- > > kernel/trace/trace.c | 8 > > kernel/trace/trace_seq.c | 2 +- > > 4 files changed, 14 insertions(+), 6 deletions(-) > > > > diff --git a/include/linux/trace_seq.h b/include/linux/trace_seq.h > > index 9ec229dfddaa..c28e0ccb50bd 100644 > > --- a/include/linux/trace_seq.h > > +++ b/include/linux/trace_seq.h > > @@ -29,6 +29,14 @@ trace_seq_init(struct trace_seq *s) > > s->readpos = 0; > > } > > > > +static inline void > > +trace_seq_reset(struct trace_seq *s) > > +{ > > + seq_buf_clear(&s->seq); > > + s->full = 0; > > + s->readpos = 0; > > +} > > + > > /** > > * trace_seq_used - amount of actual data written to buffer > > * @s: trace sequence descriptor > > diff --git a/include/trace/trace_events.h b/include/trace/trace_events.h > > index c2f9cabf154d..2bc79998e5ab 100644 > > --- a/include/trace/trace_events.h > > +++ b/include/trace/trace_events.h > > @@ -227,7 +227,7 @@ trace_raw_output_##call(struct trace_iterator *iter, > > int flags, \ > > \ > > field = (typeof(field))entry; \ > > \ > > - trace_seq_init(p); \ > > + trace_seq_reset(p); \ > > return trace_output_call(iter, #call, print); \ > > Note, p = &iter->tmp_seq > > where it has never been initialized. To do this, we need to add: > > trace_seq_init(&iter->tmp_seq); > > where ever iter is created. You need to look at all the locations where > iter is created ("iter =") and differentiate where it is first used from > just passing pointers around. > > The iter = kzalloc() will be easy, but for example, both seq and tmp_seq > need to be initialized in tracing_buffers_open(). That makes sense, I will work on a v2. > > Perhaps we need a: > > if (WARN_ON_ONCE(!s->seq.size)) > seq_buf_init(&s->seq, s->buffer, TRACE_SEQ_BUFFER_SIZE); > else > seq_buf_clear(&s->seq); > > > in the trace_seq_reset() to catch any place that doesn't have it > initialized yet. But that would be temporary, right? Kind of a "trace_seq_init_or_reset". If that's the case it would be best to simply work out all the places instead. Would the current tests [1] cover everything or should something else be made to make sure no place was missing from the patch? Thanks for reviewing! - Ricardo -- [1] https://github.com/rostedt/ftrace-ktests
[PATCH] tracing: add trace_seq_reset function
Currently, trace_seq_init may be called many times with the intent of resetting the buffer. Add a function trace_seq_reset that does that and replace the relevant occurrences to use it instead. Signed-off-by: Ricardo B. Marliere --- include/linux/trace_seq.h| 8 include/trace/trace_events.h | 2 +- kernel/trace/trace.c | 8 kernel/trace/trace_seq.c | 2 +- 4 files changed, 14 insertions(+), 6 deletions(-) diff --git a/include/linux/trace_seq.h b/include/linux/trace_seq.h index 9ec229dfddaa..c28e0ccb50bd 100644 --- a/include/linux/trace_seq.h +++ b/include/linux/trace_seq.h @@ -29,6 +29,14 @@ trace_seq_init(struct trace_seq *s) s->readpos = 0; } +static inline void +trace_seq_reset(struct trace_seq *s) +{ + seq_buf_clear(&s->seq); + s->full = 0; + s->readpos = 0; +} + /** * trace_seq_used - amount of actual data written to buffer * @s: trace sequence descriptor diff --git a/include/trace/trace_events.h b/include/trace/trace_events.h index c2f9cabf154d..2bc79998e5ab 100644 --- a/include/trace/trace_events.h +++ b/include/trace/trace_events.h @@ -227,7 +227,7 @@ trace_raw_output_##call(struct trace_iterator *iter, int flags, \ \ field = (typeof(field))entry; \ \ - trace_seq_init(p); \ + trace_seq_reset(p); \ return trace_output_call(iter, #call, print); \ } \ static struct trace_event_functions trace_event_type_funcs_##call = { \ diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c index 46dbe22121e9..9147f3717b9a 100644 --- a/kernel/trace/trace.c +++ b/kernel/trace/trace.c @@ -6946,7 +6946,7 @@ tracing_read_pipe(struct file *filp, char __user *ubuf, /* reset all but tr, trace, and overruns */ trace_iterator_reset(iter); cpumask_clear(iter->started); - trace_seq_init(&iter->seq); + trace_seq_reset(&iter->seq); trace_event_read_lock(); trace_access_lock(iter->cpu_file); @@ -6993,7 +6993,7 @@ tracing_read_pipe(struct file *filp, char __user *ubuf, /* Now copy what we have to the user */ sret = trace_seq_to_user(&iter->seq, ubuf, cnt); if (iter->seq.readpos >= trace_seq_used(&iter->seq)) - trace_seq_init(&iter->seq); + trace_seq_reset(&iter->seq); /* * If there was nothing to send to user, in spite of consuming trace @@ -7125,7 +7125,7 @@ static ssize_t tracing_splice_read_pipe(struct file *filp, spd.partial[i].offset = 0; spd.partial[i].len = trace_seq_used(&iter->seq); - trace_seq_init(&iter->seq); + trace_seq_reset(&iter->seq); } trace_access_unlock(iter->cpu_file); @@ -10274,7 +10274,7 @@ trace_printk_seq(struct trace_seq *s) printk(KERN_TRACE "%s", s->buffer); - trace_seq_init(s); + trace_seq_reset(s); } void trace_init_global_iter(struct trace_iterator *iter) diff --git a/kernel/trace/trace_seq.c b/kernel/trace/trace_seq.c index c158d65a8a88..741b2f3d76c0 100644 --- a/kernel/trace/trace_seq.c +++ b/kernel/trace/trace_seq.c @@ -59,7 +59,7 @@ int trace_print_seq(struct seq_file *m, struct trace_seq *s) * do something else with the contents. */ if (!ret) - trace_seq_init(s); + trace_seq_reset(s); return ret; } -- 2.43.0