On Sat, May 19, 2018 at 12:09:46AM +0300, Andy Shevchenko wrote:
> On Fri, May 18, 2018 at 4:59 AM, Joel Fernandes <joe...@google.com> wrote:
> > From: "Joel Fernandes (Google)" <j...@joelfernandes.org>
> >
> > In this patch we introduce a test module for simulating a long atomic
> > section in the kernel which the preemptoff or irqsoff tracers can
> > detect. This module is to be used only for test purposes and is default
> > disabled.
> >
> > Following is the expected output (only briefly shown) that can be parsed
> > to verify that the tracers are working correctly. We will use this from
> > the kselftests in future patches.
>
> > +config TEST_ATOMIC_SECTIONS
> > + tristate "Simulate atomic sections for tracers to detect"
>
> > + default n
>
> n _is_ default default.
I would rather be explicit, several other TEST configs also mention it. Is it
a strong desire to drop off default n?
> > +/*
>
> SPDX?
Ok, will add.
>
> > + */
>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/ktime.h>
> > +#include <linux/irq.h>
> > +#include <linux/printk.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/delay.h>
> > +#include <linux/string.h>
> > +#include <linux/kthread.h>
>
> Perhaps keep in order?
Sure.
> > +
> > +static int atomic_time = 100;
> > +static char atomic_mode[10] = "irq";
> > +
> > +module_param_named(atomic_time, atomic_time, int, S_IRUGO);
> > +module_param_string(atomic_mode, atomic_mode, 10, S_IRUGO);
> > +MODULE_PARM_DESC(atomic_time, "Period in microseconds (100 uS default)");
> > +MODULE_PARM_DESC(atomic_mode, "Mode of the test such as preempt or irq
> > (default irq)");
>
> > +
> > +
>
> Extra blank line.
Fixed
> > +static int __init atomic_sect_init(void)
> > +{
> > + char task_name[50];
> > + struct task_struct *test_task;
> > +
>
> > + sprintf(task_name, "%s dis test", atomic_mode);
>
> Just to be protective from dumb user.
>
> snprintf();
>
Done.
> > +
> > + test_task = kthread_run((void*)atomic_sect_run, NULL, task_name);
> > + if (IS_ERR(test_task)) {
>
> > + return -1;
>
> return PTR_ERR() ?
Sure, will do. Thanks for the review!
- Joel
