Messages in this thread | | | Date | Fri, 27 Mar 2015 17:15:36 +0100 | From | Peter Zijlstra <> | Subject | Re: [PATCH v2 0/5] Reenable might_sleep() checks for might_fault() |
| |
On Fri, Mar 27, 2015 at 04:40:50PM +0100, David Hildenbrand wrote: > e.g. futex_atomic_op_inuser(): easy to fix, add preempt_enable/disable > respectively. > > e.g. futex_atomic_cmpxchg_inatomic(): not so easy / nice to fix. > > The "inatomic" variants rely on the caller to make sure that preemption is > disabled. > > pagefault_disable(); > ret = futex_atomic_cmpxchg_inatomic(curval, uaddr, uval, newval); > pagefault_enable();
Typically the _inatomic() variants of functions have the exception tables required for fixups and can return -EFAULT. In that regard the futex_atomic_cmpxchg_inatomic() is consistently named.
In specific the above is taken from cmpxchg_futex_value_locked(), which is private to futex.c, so we don't really need to worry about it.
Furthermore, the futex.c helpers that wrap them in pagefault_disable() do so because they want to handle the fault themselves. I don't think we need to worry about that.
> 1. We could simply add preempt_disable/enable to the calling code. However that > results in _all_ futex_atomic_cmpxchg_inatomic() running with disabled > preemption, although the implementation doesn't really need it. So there is not > really a "decoupling", but to counters to set.
Not really needed, the few callsites where they are not already under a lock is where we want to explicitly handle the -EFAULT case ourselves.
> 2. We could add the preempt_disable/enable to the implementations that only > need it, leaving calling code as is. However, then the name > "futex_atomic_cmpxchg_inatomic" is misleading, because it has nothing to do > with "inatomic" anymore.
The _inatomic() naming is because it _can_ be called from atomic context, like __copy_to_user_inatomic(). It doesn't mean it has to. These functions work just fine outside of atomic regions.
And they still can be used in atomic regions, but now pagefault_disable() will also trigger the exception fixup.
I don't think we should worry too much about this.
> The same applies to other "inatomic" functions. I think most of these functions > rely on pagefaults to be disabled in order to work correctly, not disabled > preemption. > > Any idea how to fix this or what would be the way to go?
I have the feeling you're over thinking this. _inatomic() has exception fixups and will return -EFAULT when it cannot do the pagefault in place, for whatever reason -- traditionally because of atomic context, now also pagefault_disable().
And esp. things like futexes have been extensively used under -rt and are known good.
| |