[RFC] refactor tasklets to avoid unsigned long argument

Commit Message

Allen May 6, 2019, 9:32 a.m. UTC

All,

  I have been toying with the idea of "refactor tasklets to avoid
unsigned long argument" since Kees listed on KSPP wiki. I wanted to
and have kept the implementation very simple. Let me know what you
guys think.

Note: I haven't really done much of testing besides boot testing with small
set of files moved to the new api.

  My only concern with the implementation is, in the kernel the combination
of tasklet_init/DECLARE_TAKSLET is seen in over ~400 plus files.
With the change(dropping unsigned long argument) there will be huge list
of patches migrating to the new api.

Below is the diff of the change:

Signed-off-by: Allen Pais <[e-mail]>

             eni_dev->vci,eni_dev->rx_dma,eni_dev->tx_dma,
             eni_dev->service,buf);
        spin_lock_init(&eni_dev->lock);
-       tasklet_init(&eni_dev->task,eni_tasklet,(unsigned long) dev);
+       tasklet_init(&eni_dev->task,eni_tasklet);
        eni_dev->events = 0;
        /* initialize memory management */
        buffer_mem = eni_dev->mem - (buf - eni_dev->ram);


 - Allen

Comments

Kees Cook May 6, 2019, 4:11 p.m. UTC | #1

On Mon, May 6, 2019 at 2:32 AM Allen <[e-mail]> wrote:
>   I have been toying with the idea of "refactor tasklets to avoid
> unsigned long argument" since Kees listed on KSPP wiki. I wanted to
> and have kept the implementation very simple. Let me know what you
> guys think.
>
> Note: I haven't really done much of testing besides boot testing with small
> set of files moved to the new api.
>
>   My only concern with the implementation is, in the kernel the combination
> of tasklet_init/DECLARE_TAKSLET is seen in over ~400 plus files.
> With the change(dropping unsigned long argument) there will be huge list
> of patches migrating to the new api.

Yeah, this is the main part of the work for making this change. When
the timer API got changed, I had to do a two-stage change so we could
convert the users incrementally, and then finalize the API change to
drop the old style.

Beyond that, yeah, everything you sent looks good. It's just the
matter of building a series of patches to do it without breaking the
world. (Though if it's small enough, maybe it could be a single patch?
But I doubt that would be doable...)

Allen May 7, 2019, 5:55 a.m. UTC | #2

> >   My only concern with the implementation is, in the kernel the combination
> > of tasklet_init/DECLARE_TAKSLET is seen in over ~400 plus files.
> > With the change(dropping unsigned long argument) there will be huge list
> > of patches migrating to the new api.
>
> Yeah, this is the main part of the work for making this change. When
> the timer API got changed, I had to do a two-stage change so we could
> convert the users incrementally, and then finalize the API change to
> drop the old style.
>
> Beyond that, yeah, everything you sent looks good. It's just the
> matter of building a series of patches to do it without breaking the
> world. (Though if it's small enough, maybe it could be a single patch?
> But I doubt that would be doable...)

 Thank you.

 I like the idea of two-stage approach. For first stage, I was thinking of all
the architectures with their default configs(Nothing gets broken when
defconf's are tried) and for second stage the rest of drivers etc..

Thoughts?

Either ways, I will convert and get them pushed to github, we could see how
best it could be taken to upstream once it's done.

       - Allen

Patch

diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h
index 690b238a44d5..5e58df52970f 100644
--- a/include/linux/interrupt.h
+++ b/include/linux/interrupt.h
@@ -589,16 +589,17 @@  struct tasklet_struct
        struct tasklet_struct *next;
        unsigned long state;
        atomic_t count;
-       void (*func)(unsigned long);
-       unsigned long data;
+       void (*func)(struct tasklet_struct *);
 };

-#define DECLARE_TASKLET(name, func, data) \
-struct tasklet_struct name = { NULL, 0, ATOMIC_INIT(0), func, data }
+#define DECLARE_TASKLET(name, func) \
+struct tasklet_struct name = { NULL, 0, ATOMIC_INIT(0), func }

-#define DECLARE_TASKLET_DISABLED(name, func, data) \
-struct tasklet_struct name = { NULL, 0, ATOMIC_INIT(1), func, data }
+#define DECLARE_TASKLET_DISABLED(name, func) \
+struct tasklet_struct name = { NULL, 0, ATOMIC_INIT(1), func }

+#define from_tasklet(var, callback_tasklet, tasklet_fieldname) \
+       container_of(callback_tasklet, typeof(*var), tasklet_fieldname)

 enum
 {
@@ -666,7 +667,7 @@  static inline void tasklet_enable(struct tasklet_struct *t)
 extern void tasklet_kill(struct tasklet_struct *t);
 extern void tasklet_kill_immediate(struct tasklet_struct *t, unsigned int cpu);
 extern void tasklet_init(struct tasklet_struct *t,
-                        void (*func)(unsigned long), unsigned long data);
+                        void (*func)(struct tasklet_struct *));

 struct tasklet_hrtimer {
        struct hrtimer          timer;
diff --git a/kernel/softirq.c b/kernel/softirq.c
index 10277429ed84..923a76be6038 100644
--- a/kernel/softirq.c
+++ b/kernel/softirq.c
@@ -521,7 +521,7 @@  static void tasklet_action_common(struct softirq_action *a,
                                if (!test_and_clear_bit(TASKLET_STATE_SCHED,
                                                        &t->state))
                                        BUG();
-                               t->func(t->data);
+                               t->func(t);
                                tasklet_unlock(t);
                                continue;
                        }
@@ -548,13 +548,12 @@  static __latent_entropy void
tasklet_hi_action(struct softirq_action *a)
 }

 void tasklet_init(struct tasklet_struct *t,
-                 void (*func)(unsigned long), unsigned long data)
+                 void (*func)(struct tasklet_struct *))
 {
        t->next = NULL;
        t->state = 0;
        atomic_set(&t->count, 0);
        t->func = func;
-       t->data = data;
 }
 EXPORT_SYMBOL(tasklet_init);

@@ -595,9 +594,9 @@  static enum hrtimer_restart
__hrtimer_tasklet_trampoline(struct hrtimer *timer)
  * Helper function which calls the hrtimer callback from
  * tasklet/softirq context
  */
-static void __tasklet_hrtimer_trampoline(unsigned long data)
+static void __tasklet_hrtimer_trampoline(struct tasklet_struct *t)
 {
-       struct tasklet_hrtimer *ttimer = (void *)data;
+       struct tasklet_hrtimer *ttimer = from_tasklet(ttimer, t, tasklet);
        enum hrtimer_restart restart;

        restart = ttimer->function(&ttimer->timer);
@@ -618,8 +617,7 @@  void tasklet_hrtimer_init(struct tasklet_hrtimer *ttimer,
 {
        hrtimer_init(&ttimer->timer, which_clock, mode);
        ttimer->timer.function = __hrtimer_tasklet_trampoline;
-       tasklet_init(&ttimer->tasklet, __tasklet_hrtimer_trampoline,
-                    (unsigned long)ttimer);
+       tasklet_init(&ttimer->tasklet, __tasklet_hrtimer_trampoline);
        ttimer->function = function;
 }
 EXPORT_SYMBOL_GPL(tasklet_hrtimer_init);


Couple of diffs where the files have been moved to the new api:

1.
diff --git a/kernel/debug/debug_core.c b/kernel/debug/debug_core.c
index 5cc608de6883..64b8fbff3c1a 100644
--- a/kernel/debug/debug_core.c
+++ b/kernel/debug/debug_core.c
@@ -1010,13 +1010,13 @@  static void kgdb_unregister_callbacks(void)
  * such as is the case with kgdboe, where calling a breakpoint in the
  * I/O driver itself would be fatal.
  */
-static void kgdb_tasklet_bpt(unsigned long ing)
+static void kgdb_tasklet_bpt(struct tasklet_struct *t)
 {
        kgdb_breakpoint();
        atomic_set(&kgdb_break_tasklet_var, 0);
 }

-static DECLARE_TASKLET(kgdb_tasklet_breakpoint, kgdb_tasklet_bpt, 0);
+static DECLARE_TASKLET(kgdb_tasklet_breakpoint, kgdb_tasklet_bpt);

 void kgdb_schedule_breakpoint(void)
 {

2.
diff --git a/drivers/atm/eni.c b/drivers/atm/eni.c
index f8c703426c90..0c3e924c0a48 100644
--- a/drivers/atm/eni.c
+++ b/drivers/atm/eni.c
@@ -1524,9 +1524,9 @@  static irqreturn_t eni_int(int irq,void *dev_id)
 }

-static void eni_tasklet(unsigned long data)
+static void eni_tasklet(struct tasklet_struct *t)
 {
-       struct atm_dev *dev = (struct atm_dev *) data;
+       struct atm_dev *dev = from_tasklet(dev, t, tasklet);
        struct eni_dev *eni_dev = ENI_DEV(dev);
        unsigned long flags;
        u32 events;
@@ -1841,7 +1841,7 @@  static int eni_start(struct atm_dev *dev)