[PATCH] preempt abstraction

[PATCH] preempt abstraction

[David Howells]

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

Re: [PATCH] preempt abstraction

[Jeff Garzik]

David Howells wrote:
>         ftp://infradead.org/pub/people/dwh/preempt-252p10.diff.bz2

> It also replaces instances of:
> 
>         if (current->need_resched())
>                 schedule();
> 
> With:
> 
>         preempt();


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

Re: [PATCH] preempt abstraction

[Robert Love]

On Tue, 2002-01-08 at 12:40, David Howells wrote:
> 
> 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.

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

Re: [PATCH] preempt abstraction

[Christoph Hellwig]

On Tue, Jan 08, 2002 at 01:57:28PM -0500, Robert Love wrote:
> 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 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

Re: [PATCH] preempt abstraction

[Andrew Morton]

Christoph Hellwig wrote:
> 
> On Tue, Jan 08, 2002 at 01:57:28PM -0500, Robert Love wrote:
> > 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 think the choice is very subjective, but I prefer preempt().
> It's nicely short to type (!) and similar in spirit to Ingo's yield()..
> 

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;
	}
	...
}

Re: [PATCH] preempt abstraction

[Roger Larsson]

On Tuesday den 8 January 2002 21.52, Andrew Morton wrote:
> Christoph Hellwig wrote:
> > On Tue, Jan 08, 2002 at 01:57:28PM -0500, Robert Love wrote:
> > > 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 think the choice is very subjective, but I prefer preempt().
> > It's nicely short to type (!) and similar in spirit to Ingo's yield()..
>
> naah.  preempt() means preempt.  But the implementation
> is in fact maybe_preempt(), or preempt_if_needed().
>

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

Re: [PATCH] preempt abstraction

[Robert Love]

On Tue, 2002-01-08 at 15:52, Andrew Morton wrote:

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

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

Re: [PATCH] preempt abstraction

[Daniel Phillips]

On January 8, 2002 10:25 pm, Roger Larsson wrote:
> On Tuesday den 8 January 2002 21.52, Andrew Morton wrote:
> > Christoph Hellwig wrote:
> > > On Tue, Jan 08, 2002 at 01:57:28PM -0500, Robert Love wrote:
> > > > 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 think the choice is very subjective, but I prefer preempt().
> > > It's nicely short to type (!) and similar in spirit to Ingo's yield()..
> >
> > naah.  preempt() means preempt.  But the implementation
> > is in fact maybe_preempt(), or preempt_if_needed().
> >
> 
> how about
> 
>   preemption_point();
> 
> A point of (possible) preemption...

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

    schedule_point();

--
Daniel

Re: [PATCH] preempt abstraction

[David Howells]

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

Re: [PATCH] preempt abstraction

[David Howells]

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

Re: [PATCH] preempt abstraction

[Robert Love]

On Tue, 2002-01-08 at 17:46, David Howells wrote:

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

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

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

Good patch,

	Robert Love