Re: [PATCH 2/2] del_timer_sync: proof of concept

[Oleg Nesterov]

Ingo Molnar wrote:
>
> * Oleg Nesterov <oleg@xxxxxxxxxx> wrote:
>
> > New rules:
> > 	->_base &  1	: is timer pending
> > 	->_base & ~1	: timer's base
>
> how would it look like if we had a separate timer->pending field after
> all? Would it be faster/cleaner?

The only change visible outside kernel/timer.c is:

 static inline int timer_pending(const struct timer_list * timer)
 {
-	return timer->base != NULL;
+	return timer->base & 1;
 }

Currently __get_base() usage in the kernel/time.c suboptimal and
should be cleanuped, I see no other problems with performance.

> (we dont need to keep them small _that_ bad - if there's a good reason
> we should rather add a clean new field than to encode two fields into
> one field and complicate the code.)

I think that separate timer->pending field will require more changes,
because we can't read/write base+pending atomically.

int del_timer()
{
again:
	if (!timer->pending)	// not strictly necessary, but it is
		return 0;	// nice to avoid locking
	base = timer->base;
	if (!base)
		return 0;

	spin_lock(base->lock);

	if (!timer->pending) {
		spin_unlock();
		goto again;
	}
	if (timer->base != base) {
		spin_unlock();
		goto again;
	}
	....	
}

Note also, that we have to audit every timer->base usage anyway,
because currently it mix base and pending.

But may be you are right, the encoding of a bit in a pointer is
indeed weird.

Oleg.
-
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/