Re: bug in thread_group_times (+possible patch)

[Fawzi Mohamed]

On Mar 11, 2012, at 5:09 AM, Hillf Danton wrote:

> Hello Fawzi,
> 
> On Sun, Mar 11, 2012 at 7:11 AM, Fawzi Mohamed <fawzi@xxxxxx> wrote:
>> Hi,
>> 
>> a friend of mine asked me about a kernel crash that he was having on a linux cluster (always after long compute intensive task).
>> I found the bug causing it, and patched it (in an imperfect way), and as I have seen that in the current git the bug is still there, I am writing it here.
>> I am not into kernel development, so I did not subscribe to the list, please CC me if you want me to answer.
>> Sorry if this is not the correct way, but I hope this might help to squash just another little bug.
>> 
>> Now the bug is in kernel/sched/core.c in thread_group_times which if CONFIG_VIRT_CPU_ACCOUNTING is not defined looks like this
>> 
>> void thread_group_times(struct task_struct *p, cputime_t *ut, cputime_t *st)
>> {
>>        struct signal_struct *sig = p->signal;
>>        struct task_cputime cputime;
>>        cputime_t rtime, utime, total;
>> 
>>        thread_group_cputime(p, &cputime);
>> 
>>        total = cputime.utime + cputime.stime;
>>        rtime = nsecs_to_cputime(cputime.sum_exec_runtime);
>> 
>>        if (total) {
>>                u64 temp = (__force u64) rtime;
>> 
>>                temp *= (__force u64) cputime.utime;
> 
> Would you please try the tiny change?
> 
> -                do_div(temp, (__force u32) total);
> +                temp = div64_u64(temp, (__force u64) total);

that should also work, and be better than my solution (keeps the precision by shifting just as much as  needed).
I guess it can be tested on the kernel 2.6.32 (where the error did happen, and that cannot really be upgraded, as it is a production system, and has hardware that requires special drivers).
I just tested and div64_u64 was already available.
The case is not required in this case (because an up cast if required will be implicit), and it does not make sense to cast one argument but not the other, as they require the same conversion.
So
> +                temp = div64_u64(temp, total);
should work.
Now it is up to Thomas (the one administering the machines, and that found the crash), but while this should fix the problem we saw, don't hold your breath for a quick confirmation, it did require ~19 days of computation to happen.
 
ciao
Fawzi
> 
>>                utime = (__force cputime_t) temp;
>>        } else
>>                utime = rtime;
>> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/