2002-12-19 02:32:08

by Perez-Gonzalez, Inaky

[permalink] [raw]
Subject: RE: [PATCH 2.5.52] Use __set_current_state() instead of current-> state = (take 1)


> > - any setting before a return should be barriered unless we
> > return to a place[s] known to be harmless
>
> Not sure.

Well, I think it makes kind of sense. If we know we are
returning to some place where nothing bad could happen
with reordering ... well, so be it, don't use __set_...()

> > - any setting to TASK_RUNNING should be kind of safe
>
> Yes, I agree. It may race, but with what?

Hmmm ... I guess it could race, but it would not really
be that important [unless maybe certain constructs], as
the only setting it will go to would be [UN]INTERRUPTIBLE,
where it would be waken up afterwards, as it'd be out of the
rq, STOPPED, same thing, out of the rq, ZOMBIE or DEAD [gone].

I guess in this case, the only one I can come up with,
there would be an inconsistency between the actual state
of the task in task->state and the real one [or its position
in whatever rq or wq].

Could that have ugly consequences? I dunno ...

> > - current->state = TASK_UNINTERRUPTIBLE;
> > + __set_current_state(TASK_UNINTERRUPTIBLE);
> > spin_unlock_irq(&oldsig->siglock);
> >
> > Should be safe, as spin_unlock_irq() will do memory clobber
> > on sti() [undependant from UP/SMP].
>
> The memory clobber only acts as a compiler barrier and insures the
> compiler does not reorder the statements from the order in the C code.

while (1)
printk ("Inaky, remember, clobber is a hint to gcc\n");

And that would now really work when CONFIG_X86_OOSTORE=1 is required
[after all, it is a write, so it'd need the equivalent of a wmb() or
xchg()].
Okay, changing that one too, just in case.


Inaky Perez-Gonzalez -- Not speaking for Intel - opinions are my own [or my
fault]



2002-12-19 03:11:41

by Robert Love

[permalink] [raw]
Subject: RE: [PATCH 2.5.52] Use __set_current_state() instead of current-> state = (take 1)

On Wed, 2002-12-18 at 21:40, Perez-Gonzalez, Inaky wrote:

> Well, I think it makes kind of sense. If we know we are
> returning to some place where nothing bad could happen
> with reordering ... well, so be it, don't use __set_...()

Oh, I see. If it returns to somewhere that immediately e.g. puts it on
a wait queue. In that case, yep: need the barrier version.

> And that would now really work when CONFIG_X86_OOSTORE=1 is required
> [after all, it is a write, so it'd need the equivalent of a wmb() or
> xchg()].

Is this a hint that your employer may have an x86 chip in the future
with weak ordering? :)

> Okay, changing that one too, just in case.

Good - better safe than sorry.

Robert Love