[PATCH][2.6.7-mm4] perfctr update 5/6: reduce stack usage

From: Mikael Pettersson (mikpe_at_csd.uu.se)
Date: 06/30/04

  • Next message: Vojtech Pavlik: "Experimental PS/2 driver with heuristic synchronization"
    Date:	Wed, 30 Jun 2004 14:21:52 +0200 (MEST)
    To: akpm@osdl.org
    
    

    - Reduce stack usage by using kmalloc() instead of the stack
      for temporary state and control copies.
    - Eliminate some unnecessary cpumask_t copies. Use newish
      cpus_intersects() instead of cpus_and(); !cpus_empty().

    Signed-off-by: Mikael Pettersson <mikpe@csd.uu.se>

    --- linux-2.6.7-mm4/drivers/perfctr/init.c.~1~ 2004-06-29 19:01:56.000000000 +0200
    +++ linux-2.6.7-mm4/drivers/perfctr/init.c 2004-06-30 01:14:20.000000000 +0200
    @@ -54,14 +54,12 @@
             if (infop && copy_to_user(infop, &perfctr_info, sizeof perfctr_info))
                     return -EFAULT;
             if (cpusp) {
    - cpumask_t cpus = cpu_online_map;
    - int err = cpus_copy_to_user(cpusp, &cpus);
    + int err = cpus_copy_to_user(cpusp, &cpu_online_map);
                     if (err)
                             return err;
             }
             if (forbiddenp) {
    - cpumask_t cpus = perfctr_cpus_forbidden_mask;
    - int err = cpus_copy_to_user(forbiddenp, &cpus);
    + int err = cpus_copy_to_user(forbiddenp, &perfctr_cpus_forbidden_mask);
                     if (err)
                             return err;
             }
    --- linux-2.6.7-mm4/drivers/perfctr/virtual.c.~1~ 2004-06-29 19:11:30.000000000 +0200
    +++ linux-2.6.7-mm4/drivers/perfctr/virtual.c 2004-06-30 02:29:12.000000000 +0200
    @@ -364,10 +364,7 @@
                                      struct vperfctr *perfctr,
                                      cpumask_t new_mask)
     {
    - cpumask_t tmp;
    -
    - cpus_and(tmp, new_mask, perfctr_cpus_forbidden_mask);
    - if (!cpus_empty(tmp)) {
    + if (cpus_intersects(new_mask, perfctr_cpus_forbidden_mask)) {
                     atomic_set(&perfctr->bad_cpus_allowed, 1);
                     printk(KERN_WARNING "perfctr: process %d (comm %s) issued unsafe"
                            " set_cpus_allowed() on process %d (comm %s)\n",
    @@ -391,7 +388,7 @@
                                    const struct vperfctr_control __user *argp,
                                    struct task_struct *tsk)
     {
    - struct vperfctr_control control;
    + struct vperfctr_control *control;
             int err;
             unsigned int next_cstatus;
             unsigned int nrctrs, i;
    @@ -399,18 +396,29 @@
             if (!tsk)
                     return -ESRCH; /* attempt to update unlinked perfctr */
     
    - if (copy_from_user(&control, argp, sizeof control))
    - return -EFAULT;
    + /* The control object can be large (over 300 bytes on i386),
    + so kmalloc() it instead of storing it on the stack.
    + We must use task-private storage to prevent racing with a
    + monitor process attaching to us before the non-preemptible
    + perfctr update step. Therefore we cannot store the copy
    + in the perfctr object itself. */
    + control = kmalloc(sizeof(*control), GFP_USER);
    + if (!control)
    + return -ENOMEM;
    +
    + err = -EFAULT;
    + if (copy_from_user(control, argp, sizeof *control))
    + goto out_kfree;
     
    - if (control.cpu_control.nractrs || control.cpu_control.nrictrs) {
    - cpumask_t tmp, old_mask, new_mask;
    + if (control->cpu_control.nractrs || control->cpu_control.nrictrs) {
    + cpumask_t old_mask, new_mask;
     
    - tmp = perfctr_cpus_forbidden_mask;
                     old_mask = tsk->cpus_allowed;
    - cpus_andnot(new_mask, old_mask, tmp);
    + cpus_andnot(new_mask, old_mask, perfctr_cpus_forbidden_mask);
     
    + err = -EINVAL;
                     if (cpus_empty(new_mask))
    - return -EINVAL;
    + goto out_kfree;
                     if (!cpus_equal(new_mask, old_mask))
                             set_cpus_allowed(tsk, new_mask);
             }
    @@ -424,7 +432,7 @@
                     perfctr->cpu_state.cstatus = 0;
                     vperfctr_clear_iresume_cstatus(perfctr);
             }
    - perfctr->cpu_state.control = control.cpu_control;
    + perfctr->cpu_state.control = control->cpu_control;
             /* remote access note: perfctr_cpu_update_control() is ok */
             err = perfctr_cpu_update_control(&perfctr->cpu_state, 0);
             if (err < 0)
    @@ -434,14 +442,14 @@
                     goto out;
     
             /* XXX: validate si_signo? */
    - perfctr->si_signo = control.si_signo;
    + perfctr->si_signo = control->si_signo;
     
             if (!perfctr_cstatus_has_tsc(next_cstatus))
                     perfctr->cpu_state.tsc_sum = 0;
     
             nrctrs = perfctr_cstatus_nrctrs(next_cstatus);
             for(i = 0; i < nrctrs; ++i)
    - if (!(control.preserve & (1<<i)))
    + if (!(control->preserve & (1<<i)))
                             perfctr->cpu_state.pmc[i].sum = 0;
     
             if (tsk == current)
    @@ -449,6 +457,8 @@
     
      out:
             preempt_enable();
    + out_kfree:
    + kfree(control);
             return err;
     }
     
    @@ -500,8 +510,21 @@
                                 struct vperfctr_control __user *controlp,
                                 const struct task_struct *tsk)
     {
    - struct perfctr_sum_ctrs sum;
    - struct vperfctr_control control;
    + struct {
    + struct perfctr_sum_ctrs sum;
    + struct vperfctr_control control;
    + } *tmp;
    + int ret;
    +
    + /* The state snapshot can be large (almost 500 bytes on i386),
    + so kmalloc() it instead of storing it on the stack.
    + We must use task-private storage to prevent racing with a
    + monitor process attaching to us during the preemptible
    + copy_to_user() step. Therefore we cannot store the snapshot
    + in the perfctr object itself. */
    + tmp = kmalloc(sizeof(*tmp), GFP_USER);
    + if (!tmp)
    + return -ENOMEM;
     
             /* PREEMPT note: While we're reading our own control, another
                process may ptrace ATTACH to us and update our control.
    @@ -515,22 +538,26 @@
             }
             if (sump) { //sum = perfctr->cpu_state.sum;
                     int j;
    - sum.tsc = perfctr->cpu_state.tsc_sum;
    - for(j = 0; j < ARRAY_SIZE(sum.pmc); ++j)
    - sum.pmc[j] = perfctr->cpu_state.pmc[j].sum;
    + tmp->sum.tsc = perfctr->cpu_state.tsc_sum;
    + for(j = 0; j < ARRAY_SIZE(tmp->sum.pmc); ++j)
    + tmp->sum.pmc[j] = perfctr->cpu_state.pmc[j].sum;
             }
             if (controlp) {
    - control.si_signo = perfctr->si_signo;
    - control.cpu_control = perfctr->cpu_state.control;
    - control.preserve = 0;
    + tmp->control.si_signo = perfctr->si_signo;
    + tmp->control.cpu_control = perfctr->cpu_state.control;
    + tmp->control.preserve = 0;
             }
             if (tsk == current)
                     preempt_enable();
    - if (sump && copy_to_user(sump, &sum, sizeof sum))
    - return -EFAULT;
    - if (controlp && copy_to_user(controlp, &control, sizeof control))
    - return -EFAULT;
    - return 0;
    + ret = -EFAULT;
    + if (sump && copy_to_user(sump, &tmp->sum, sizeof tmp->sum))
    + goto out;
    + if (controlp && copy_to_user(controlp, &tmp->control, sizeof tmp->control))
    + goto out;
    + ret = 0;
    + out:
    + kfree(tmp);
    + return ret;
     }
     
     /****************************************************************
    -
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/


  • Next message: Vojtech Pavlik: "Experimental PS/2 driver with heuristic synchronization"

    Relevant Pages

    • Re: [PATCH][1/6] perfctr-2.7.3 for 2.6.7-rc1-mm1: core
      ... The term "remote control" has not been defined by you, ... assume that it refers to one process initiating perfctr instrumentation ... That should reduce stack usage. ... send the line "unsubscribe linux-kernel" in ...
      (Linux-Kernel)
    • Re: I request inclusion of reiser4 in the mainline kernel
      ... > dependency on!4k stacks was removed and stack usage was fixed. ... errors sticking out from an half an error look. ... I'll do a deeper review, ... send the line "unsubscribe linux-kernel" in ...
      (Linux-Kernel)
    • Re: GCC 3.4 and broken inlining.
      ... > inlining somewhat blows up stack usage so do monitor it... ... There had been need of rain for many days. ... send the line "unsubscribe linux-kernel" in ...
      (Linux-Kernel)
    • Re: unit-at-a-time...
      ... > It happened when I added perfctr to a 2.6.5-based SuSE kernel, ... Perfctr normally adds a little stack usage to the ... Better fix would have been a few strategic "noinline"s. ... send the line "unsubscribe linux-kernel" in ...
      (Linux-Kernel)
    • Re: [patch 2/2] kernel/power/disk.c string fix and if-less iterator
      ... Note also that this increases the stack usage of the function and slows it ... as this means allocating an array and filling it in at call time. ... send the line "unsubscribe linux-kernel" in ...
      (Linux-Kernel)