Bad patch to schedule()

Bad patch to schedule()

[Andi Kleen]

I just see that this patch went into mainline.

[PATCH] posix-timers: high-resolution CPU clocks for POSIX clock_* syscalls

This patch provides support for thread and process CPU time clocks in the
....

 /*
+ * This is called on clock ticks and on context switches.
+ * Bank in p->sched_time the ns elapsed since the last tick or switch.
+ */
+static inline void update_cpu_clock(task_t *p, runqueue_t *rq,
+				    unsigned long long now)
+{
+	unsigned long long last = max(p->timestamp, rq->timestamp_last_tick);
+	p->sched_time += now - last;
+}
+

called from schedule(). The problem with this is that it completely
messes up the register allocation for i386 schedule() because it 
does long long arithmetic. This causes gcc to spill everything
else because it needs four registers, and i386 only has 6 usable
ones.

I think a critical path like the scheduler needs a little bit more
care.  Also it is totally unclear if this obscure POSIX feature
is really worth making schedule() slower. I think not.

In case it is kept it should be done in a way that doesn't impact
i386 unduly e.g. by avoiding long long arithmetic here and 
making sure fast paths stay fast.

I would propose to back this patch out again until this is resolved.

-Andi

Re: Bad patch to schedule()

[Roland McGrath]

> called from schedule(). The problem with this is that it completely
> messes up the register allocation for i386 schedule() because it 
> does long long arithmetic. This causes gcc to spill everything
> else because it needs four registers, and i386 only has 6 usable
> ones.

The generated code I've seen does not bear out this claim.  If you are
using ancient compilers and they produce poor code, then it is time to move
on.  If you are using some recent gcc, then you'll have to show the
concrete details since they don't match what I see.


Thanks,
Roland