lkml.org 
[lkml]   [2010]   [Jan]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH] sched: cpuacct: Use bigger percpu counter batch values for stats counters
    On Tue, 26 Jan 2010 11:47:15 +0530 Balbir Singh <balbir@linux.vnet.ibm.com> wrote:

    > On Tuesday 26 January 2010 04:44 AM, Andrew Morton wrote:
    > > On Mon, 18 Jan 2010 15:41:42 +1100
    > > Anton Blanchard <anton@samba.org> wrote:
    > >
    > >> When CONFIG_VIRT_CPU_ACCOUNTING and CONFIG_CGROUP_CPUACCT are enabled we can
    > >> call cpuacct_update_stats with values much larger than percpu_counter_batch.
    > >> This means the call to percpu_counter_add will always add to the global count
    > >> which is protected by a spinlock and we end up with a global spinlock in
    > >> the scheduler.
    > >
    > > When one looks at the end result:
    > >
    > > : static void cpuacct_update_stats(struct task_struct *tsk,
    > > : enum cpuacct_stat_index idx, cputime_t val)
    > > : {
    > > : struct cpuacct *ca;
    > > : int batch;
    > > :
    > > : if (unlikely(!cpuacct_subsys.active))
    > > : return;
    > > :
    > > : rcu_read_lock();
    > > : ca = task_ca(tsk);
    > > :
    > > : batch = min_t(long, percpu_counter_batch * cputime_one_jiffy, INT_MAX);
    > > : do {
    > > : __percpu_counter_add(&ca->cpustat[idx], val, batch);
    > > : ca = ca->parent;
    > > : } while (ca);
    > > : rcu_read_unlock();
    > > : }
    > >
    > > the code (which used to be quite obvious) becomes pretty unobvious. In
    > > fact it looks quite wrong.
    > >
    > > Shouldn't there be a comment there explaining wtf is going on?
    >
    > Andrew,
    >
    > I guess a lot of the changelog and comments are in the email history,

    Not a very useful location for it!

    > Why does it look quite wrong to you?

    Because it computes the correct value and then if it's larger than
    INT_MAX, it inexplicably assigns INT_MAX to it, giving a wrong result!


    Does that code actually work, btw? percpu_counter_batch has type `int'
    and cputime_one_jiffy has type `int' so their product has type `int'.
    So by the time min_t performs its comparison, the upper 32 bits of the
    product are already lost.



    \
     
     \ /
      Last update: 2010-01-26 07:39    [W:0.023 / U:66.836 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site