3.14-rt1 fixes

PATCH In review Mike Galbraith 4 participants 12 messages May 2, 2014 – May 3, 2014

Share & tools

Mike Galbraith

author

2014-05-02 11:12 UTC #0 lore raw

cover 3.14-rt1 fixes

The following patches are fixes that fell out of testing rt1. Patches 1-4 are intended to be folded into existing patches, 5-6 are replacements.

fold:
1/6 - preempt-lazy-support.patch
2/6 - x86-preempt-lazy.patch
3/6 - hotplug-light-get-online-cpus.patch
4/6 - stomp-machine-raw-lock.patch

replace:
5/6 - (prep)
6/6 - stomp-machine-deal-clever-with-stopper-lock.patch

drop: (buggy - calls migrate_disable() _after_ maybe blocking) migrate_disable-pushd-down-in-atomic_dec_and_spin_lo.patch

-Mike

Pavel Vasilyev

2014-05-02 11:39 UTC #1 lore raw

02.05.2014 15:12, Mike Galbraith пишет:

Quoted 10 lines

The following patches are fixes that fell out of testing rt1. Patches 1-4 are intended to be folded into existing patches, 5-6 are replacements.
fold:
1/6 - preempt-lazy-support.patch
2/6 - x86-preempt-lazy.patch
3/6 - hotplug-light-get-online-cpus.patch
4/6 - stomp-machine-raw-lock.patch

Quoted 6 lines

replace:
5/6 - (prep)
6/6 - stomp-machine-deal-clever-with-stopper-lock.patch
drop: (buggy - calls migrate_disable() _after_ maybe blocking) migrate_disable-pushd-down-in-atomic_dec_and_spin_lo.patch

Cool, but you can the same, but for 3.14.2 ?

Signature · 5 lines

                                                         Pavel.
--
To unsubscribe from this list: send the line "unsubscribe linux-rt-users" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Mike Galbraith

author

2014-05-02 11:43 UTC #2 lore raw

Quoted 15 lines · Pavel Vasilyev

02.05.2014 15:12, Mike Galbraith пишет:
Quoted 14 lines
The following patches are fixes that fell out of testing rt1. Patches 1-4 are intended to be folded into existing patches, 5-6 are replacements.
fold:
1/6 - preempt-lazy-support.patch
2/6 - x86-preempt-lazy.patch
3/6 - hotplug-light-get-online-cpus.patch
4/6 - stomp-machine-raw-lock.patch
replace:
5/6 - (prep)
6/6 - stomp-machine-deal-clever-with-stopper-lock.patch

Quoted 5 lines

Quoted 2 lines
drop: (buggy - calls migrate_disable() _after_ maybe blocking) migrate_disable-pushd-down-in-atomic_dec_and_spin_lo.patch
Cool, but you can the same, but for 3.14.2 ?

They'll apply fine. You'll just have to comment out the two patches in the rt1 tree which are no longer needed. I added those on top of 14.2-rt1 with those two commented out.

-Mike

--
To unsubscribe from this list: send the line "unsubscribe linux-rt-users" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Nicholas Mc Guire

2014-05-03 08:41 UTC #3 lore raw

Quoted 12 lines · Mike Galbraith

Quoted 11 lines · Pavel Vasilyev
02.05.2014 15:12, Mike Galbraith ??????????:
Quoted 10 lines
The following patches are fixes that fell out of testing rt1. Patches 1-4 are intended to be folded into existing patches, 5-6 are replacements.
fold:
1/6 - preempt-lazy-support.patch
2/6 - x86-preempt-lazy.patch
3/6 - hotplug-light-get-online-cpus.patch
4/6 - stomp-machine-raw-lock.patch

Quoted 8 lines

Quoted 8 lines
Quoted 6 lines
replace:
5/6 - (prep)
6/6 - stomp-machine-deal-clever-with-stopper-lock.patch
drop: (buggy - calls migrate_disable() _after_ maybe blocking) migrate_disable-pushd-down-in-atomic_dec_and_spin_lo.patch

Why would calling migrate_disable be buggy after blocking ?
at that point any per cpu data is not yet being accessed so its perfectly fine to

lock->block
-> migrate

    -> unblock->lock 
     -> migrate_disable on a new CPU
      -> access per_cpu object
     -> migrate_enable

-> unlock

what problems would there be ?

thx!
hofrat

You tell me. Why do we bother at all if it's ok to migrate with a lock
in your pocket, or the CPU can go away on you while you're acquiring? If this is in fact safe, you should be able to move each and every migrate_disable() to post acquisition. I have a virtual nickle that says your box will have a severe allergic reaction to such a patch.

-Mike

Nicholas Mc Guire

2014-05-03 12:31 UTC #5 lore raw

Quoted 35 lines · Mike Galbraith

Quoted 34 lines · Nicholas Mc Guire
Quoted 12 lines · Mike Galbraith
Quoted 11 lines · Pavel Vasilyev
02.05.2014 15:12, Mike Galbraith ??????????:
Quoted 10 lines
The following patches are fixes that fell out of testing rt1. Patches 1-4 are intended to be folded into existing patches, 5-6 are replacements.
fold:
1/6 - preempt-lazy-support.patch
2/6 - x86-preempt-lazy.patch
3/6 - hotplug-light-get-online-cpus.patch
4/6 - stomp-machine-raw-lock.patch
Quoted 8 lines
Quoted 8 lines
Quoted 6 lines
replace:
5/6 - (prep)
6/6 - stomp-machine-deal-clever-with-stopper-lock.patch
drop: (buggy - calls migrate_disable() _after_ maybe blocking) migrate_disable-pushd-down-in-atomic_dec_and_spin_lo.patch
Why would calling migrate_disable be buggy after blocking ?
at that point any per cpu data is not yet being accessed so its perfectly fine to
lock->block
-> migrate
    -> unblock->lock 
     -> migrate_disable on a new CPU
      -> access per_cpu object
     -> migrate_enable
-> unlock

Quoted 4 lines

Quoted 1 line
what problems would there be ?
You tell me. Why do we bother at all if it's ok to migrate with a lock
in your pocket, or the CPU can go away on you while you're acquiring?

The lock in your pocket is not the issue (unless its a per_cpu lock in which case we are pinned already anyway) and the cpu can go away with a global lock held in my opinion - why not ?

Quoted 2 lines

If this is in fact safe, you should be able to move each and every migrate_disable() to post acquisition.

yup

Quoted 3 lines

I have a virtual nickle that
says your box will have a severe allergic reaction to such a patch.

Actually that is what the pushdowns in the read_lock/write_lock api did and I did not notice any of the systems having problems with that.

Not saying that there is no case in the pushdown that is wrongly handled but before looking at that lets clarify if the assumptions made for the migrate pushdowns was right in the first place.

migrate_disable() provides referencial consistency on per_cpu objects so that preemption is safe with a per_cpu ref held - nothing else.

So:
if the lock is global then it is safe to migrate while blocked on a lock as
the referencial consistency of the lock is not affected the reference to a protected per_cpu object not yet set (if it were that is a bug if the method used to access the per_cpu objects ref did not migrate_disable already).
if it is a per-cpu lock then migrate_disable() would need to be called before
accessing the lock object any way and the problem is not related to blocking on a lock - the get_ primitives would have disable migration before you got access to the lock object and thus you are safe.

Regarding the CPU going away while blocked on a lock we have:
if the lock is global then the CPU going away would push you off first without
that the reference to the lock object is invalidated - so that is safe.
if the lock is per_cpu then the get_cpu_var or what ever you use to get that
per_cpu lock object would have disable migration BEFORE you got access to the per_cpu lock object, so once you have access to the per_cpu lock you would again be safe against the cpu going away as well via pin_current_cpu

Quoted 1 line

From my understanding for a globally visible lock:

per_cpu

 lock pin   access   unpin unlock
 pin lock   access   unlock unpin

are semantically equivalent and safe
even
per_cpu

 lock pin   access   unlock unpin
 pin lock   access   unpin unlock

is safe (while strange) - in no constelation would you lose referencial consistency of the per_cpu object within the crticial section (access).

If code relies on these two (semantically orthogonal functions) to be in a particuular order I would suspect that the code has a bug.

If one now looks at the cases of
per_cpu
lock pin access ...

and assume that you got migrated away while aquireing the lock, or actually simply before you got temporarily pinned to the cpu, then this is safe for global locks - if the CPU went away and we got pushed to another CPU then the per_cpu object access is on the cpu we got pushed to but stays consistent and safe.

for a per_cpu lock the pin/unpin is implied in access to the per_cpu lock object it self so the migrate_disable done in the locking api is not needed but no harme either and the position of the same (before or after aquisition of the lock) is irrelevant.

So what case did I miss ?

thx!
hofrat

Mike Galbraith

author

2014-05-03 14:03 UTC #6 lore raw

Quoted 6 lines · Nicholas Mc Guire

Quoted 2 lines · Mike Galbraith
If this is in fact safe, you should be able to move each and every migrate_disable() to post acquisition.
yup

Having just seen working -> brick transition, color me skeptical.

Quoted 5 lines

Quoted 3 lines
I have a virtual nickle that
says your box will have a severe allergic reaction to such a patch.
Actually that is what the pushdowns in the read_lock/write_lock api did and I did not notice any of the systems having problems with that.

If you had tested hotplug, you would have met the deadlock, and would have verified that the change to read_lock() was the culprit instead of me doing that. Steven also verified that. You too can flip back and forth, drive boxen into the wall as many times as it take to convince yourself that that change really really did induce the breakage.

I'll read the rest later, I'm getting all grumpy.

-Mike

Nicholas Mc Guire

2014-05-03 14:19 UTC #7 lore raw

Quoted 13 lines · Mike Galbraith

Quoted 6 lines · Nicholas Mc Guire
Quoted 2 lines · Mike Galbraith
If this is in fact safe, you should be able to move each and every migrate_disable() to post acquisition.
yup
Having just seen working -> brick transition, color me skeptical.
Quoted 3 lines
Quoted 3 lines
I have a virtual nickle that
says your box will have a severe allergic reaction to such a patch.

Quoted 9 lines

Quoted 2 lines
Actually that is what the pushdowns in the read_lock/write_lock api did and I did not notice any of the systems having problems with that.
If you had tested hotplug, you would have met the deadlock, and would have verified that the change to read_lock() was the culprit instead of me doing that. Steven also verified that. You too can flip back and forth, drive boxen into the wall as many times as it take to convince yourself that that change really really did induce the breakage.

I did not test hotplug - I did try and understand the code to verify the assumptions - but before looking at details of some code path (which in my opinion has a few problems of its own) I think it would be best to clarify first if the assumptions made for the migrate pushdown patches is right or not - if it is not there is no point in discussing individual code paths but then that patch set simply needs to go out, if the assumptions are right then we can discuss where the fix is needed.

thx!
hofrat

Mike Galbraith

author

2014-05-03 15:24 UTC #8 lore raw

Quoted 18 lines · Nicholas Mc Guire

Quoted 16 lines · Mike Galbraith
Quoted 6 lines · Nicholas Mc Guire
Quoted 2 lines · Mike Galbraith
If this is in fact safe, you should be able to move each and every migrate_disable() to post acquisition.
yup
Having just seen working -> brick transition, color me skeptical.
Quoted 5 lines
Quoted 3 lines
I have a virtual nickle that
says your box will have a severe allergic reaction to such a patch.
Actually that is what the pushdowns in the read_lock/write_lock api did and I did not notice any of the systems having problems with that.

Quoted 8 lines

Quoted 6 lines
If you had tested hotplug, you would have met the deadlock, and would have verified that the change to read_lock() was the culprit instead of me doing that. Steven also verified that. You too can flip back and forth, drive boxen into the wall as many times as it take to convince yourself that that change really really did induce the breakage.
I did not test hotplug - I did try and understand the code to

Not testing hotplug was obviously a mistake given what you were changing. Nor is that a tiny change, it's a glaringly huge order change... for read_lock(), with no order change to write_lock().

Things that make ya go hmmm.

Quoted 7 lines

verify the assumptions - but before looking at details of some code path (which in my opinion has a few problems of its own) I think it would be best to clarify first if the assumptions made for the migrate pushdown patches is right or not - if it is not there is no point in discussing individual code paths but then that patch set simply needs to go out, if the assumptions are right then we can discuss where the fix is needed.

If your assumptions were correct, there would have been no breakage.

-Mike

Sebastian Andrzej Siewior

2014-05-02 13:44 UTC #9 lore raw

In reply to Mike Galbraith

Quoted 2 lines · * Mike Galbraith | 2014-05-02 13:12:39 [+0200]:

drop: (buggy - calls migrate_disable() _after_ maybe blocking) migrate_disable-pushd-down-in-atomic_dec_and_spin_lo.patch

In the thread you quoted also the delayed migrated_disable in rt_read_lock(). Is this one no longer required?

Quoted 1 line

-Mike

Sebastian

Mike Galbraith

author

2014-05-02 13:45 UTC #10 lore raw

Quoted 7 lines · Sebastian Andrzej Siewior

Quoted 2 lines · * Mike Galbraith | 2014-05-02 13:12:39 [+0200]:
drop: (buggy - calls migrate_disable() _after_ maybe blocking) migrate_disable-pushd-down-in-atomic_dec_and_spin_lo.patch
In the thread you quoted also the delayed migrated_disable in rt_read_lock(). Is this one no longer required?

Steven submitted a patch for that.

-Mike

Sebastian Andrzej Siewior

2014-05-02 13:49 UTC #11 lore raw

Quoted 10 lines · Mike Galbraith

Quoted 7 lines · Sebastian Andrzej Siewior
Quoted 2 lines · * Mike Galbraith | 2014-05-02 13:12:39 [+0200]:
drop: (buggy - calls migrate_disable() _after_ maybe blocking) migrate_disable-pushd-down-in-atomic_dec_and_spin_lo.patch
In the thread you quoted also the delayed migrated_disable in rt_read_lock(). Is this one no longer required?
Steven submitted a patch for that.

Ah I see. Let me look at this. What it looks is that we revert the 3 three push down patches since they break hotplug. Thanks.

Quoted 3 lines

-Mike

Sebastian