Re: [patch 11/23] hrtimers: state tracking

On Fri, 29 Sep 2006 23:58:30 -0000
Thomas Gleixner <tglx@xxxxxxxxxxxxx> wrote:

From: Thomas Gleixner <tglx@xxxxxxxxxxxxx>

reintroduce ktimers feature "optimized away" by the ktimers
review process: multiple hrtimer states to enable the running
of hrtimers without holding the cpu-base-lock.

(the "optimized" rbtree hack carried only 2 states worth of
information and we need 3.)


uh, I'll believe you ;)

-#define HRTIMER_INACTIVE ((void *)1UL)
+#define HRTIMER_INACTIVE 0x00
+#define HRTIMER_ACTIVE 0x01
+#define HRTIMER_CALLBACK 0x02

struct hrtimer_clock_base;

@@ -54,6 +56,7 @@ struct hrtimer {
ktime_t expires;
int (*function)(struct hrtimer *);
struct hrtimer_clock_base *base;
+ unsigned long state;

I assume that `state' here takes the above enumerated values HRTIMER_*?

Using an enum would make that explicit, and more understandable.

Does it really need to be a long type?

static inline int hrtimer_active(const struct hrtimer *timer)
{
- return rb_parent(&timer->node) != &timer->node;
+ return timer->state != HRTIMER_INACTIVE;
}

This implies that HRTIMER_CALLBACK is an "active" state, yes? If so, how
come? Perhaps a comment here would aid understandability.

+ timer->state |= HRTIMER_ACTIVE;

No! It's a bitfield! The plot thickens.

How come hrtimer_active() tests for equality of all bits if it's a bitfield?

+ timer->state = newstate;

No, it's not a bitfield. It's a scalar.

+ if (!(timer->state & HRTIMER_CALLBACK))

whoop, it's a bitfield again.

ret = remove_hrtimer(timer, base);

unlock_hrtimer_base(timer, &flags);
@@ -592,7 +594,6 @@ void hrtimer_init(struct hrtimer *timer,
clock_id = CLOCK_MONOTONIC;

timer->base = &cpu_base->clock_base[clock_id];
- rb_set_parent(&timer->node, &timer->node);
}
EXPORT_SYMBOL_GPL(hrtimer_init);

@@ -643,13 +644,14 @@ static inline void run_hrtimer_queue(str

fn = timer->function;
set_curr_timer(cpu_base, timer);
- __remove_hrtimer(timer, base);
+ __remove_hrtimer(timer, base, HRTIMER_CALLBACK);

How come this was assigned to state, and not or-ed into it?

+ timer->state &= ~HRTIMER_CALLBACK;

Please document the locking for timer->state.

Please also document its various states.
-
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/