preempt abstraction

The following patch abstracts access to need_resched:

	ftp://infradead.org/pub/people/dwh/preempt-252p10.diff.bz2

It replaces most C-source read accesses to it with need_preempt() which returns true if rescheduling is necessary.

It also replaces instances of:

	if (current->need_resched())
		schedule();

With:

	preempt();

It doesn't (or at least shouldn't) do anything else.

Cheers,
David

Regardless of the benefit of abstracting access to current->need_resched, I've always thought something like this was needed for code cleanliness as well. Since yield is now in 2.5.2-preXX, why not add your patch too. Nice...

Jeff

Jeff Garzik      | Alternate titles for LOTR:
Building 1024    | Fast Times at Uruk-Hai
MandrakeSoft     | The Took, the Elf, His Daughter and Her Lover
                 | Samwise Gamgee: International Hobbit of Mystery

Nice patch!

A couple of points:

Why not use the more commonly named conditional_schedule instead of preempt() ? In addition to being more in-use (low-latency, lock-break, and Andrea's aa patch all use it) I think it better conveys its meaning, which is a schedule() but only conditionally.

I'm sure it is just being pedantic, but why not just make need_preempt and preempt (which I would rename need_schedule and conditional_schedule, personally) defines? Example:

	#define need_schedule() (unlikely(current->need_resched))
	#define conditional_schedule() do { \
		if (need_schedule()) \
			schedule(); \
	} while(0);

Next, in kernel/sched.c you wrap need_preempt in an unlikey() but note it is unlikely by design ... Same in mm/vmscan.c a couple times.

Oh, and the patch is confusingly similar to preempt-kernel in name, but I guess that is my problem. :-)

Anyhow, I like. 2.5 _and_ 2.4?

Robert Love

I think the choice is very subjective, but I prefer preempt(). It's nicely short to type (!) and similar in spirit to Ingo's yield()..

Christoph

naah. preempt() means preempt. But the implementation is in fact maybe_preempt(), or preempt_if_needed().

I use (verbosely) (simplified):

#define conditional_schedule_needed() unlikely(current->need_resched)

#define unconditional_schedule()	do {
						__set_current_state(TASK_RUNNING)
						schedule();
					} while(0);
#define conditional_schedule()		if (conditional_schedule_needed())
						unconditional_schedule();

...

foo()

{
	...
	conditional_schedule();
	...
}

bar()

{
	...
	if (conditional_schedule_needed()) {
		spin_unlock(&piggy_lock);
		unconditional_schedule();
		spin_lock(&piggy_lock);
		goto clean_up_mess;
	}
	...
}

Agreed. preempt has me envision various things, none of which are what we want. What is the difference between schedule vs preempt? Confusing.

What we are calling preempt here is the same as schedule, but we check if it is needed. So I suggest conditional_schedule, which has the benefit of being widely used in at least three patches. schedule_if_needed, sched_if_needed, etc. both fit. Why introduce the namespace preempt when we already have sched?

sched_conditional() and sched_needed() ?

Robert Love

how about

  preemption_point();

A point of (possible) preemption...
It might be nice to add the orthogonal

preempt_disable()
preemtion_enable()

At the same time - see Robert Loves patch for places. (mostly around CPU specific data)
But they should be null statements for now...

/RogerL

Roger Larsson
Skellefteå
Sweden

It's not, it's a point of possible rescheduling. With that in mind I'd suggest... [drum roll]... [drum roll]...

    schedule_point();

--
Daniel

The choice of need_preempt() and preempt() came out of discussion with a number of people. However, I can see the points being made. I have to admit, I've not come across conditional_schedule in the main kernel, so I guess this is an Andrea specific.

I think, actually, I prefer yield/need_yield or maybe yield and yield_requested as this is consistent with things like sched_yield.

David

Linus,

ftp://infradead.org/pub/people/dwh/yield-252p10.diff.bz2

There's now a version of the patch with preempt/need_preempt changed to yield/need_yield, and with the unlikely() claused moved into need_yield(), should that be more to your preference.

David

Still not the name I would pick, but better :)

Oh, and need_yield is still marked unlikely in sched.c

Good patch,

Robert Love