* [PATCH 1/3] futex: remove duplicated code [not found] <20170303122712.13353-1-jslaby@suse.cz> @ 2017-03-03 14:08 ` Heiko Carstens 2017-03-03 14:48 ` Chris Metcalf ` (2 subsequent siblings) 3 siblings, 0 replies; 6+ messages in thread From: Heiko Carstens @ 2017-03-03 14:08 UTC (permalink / raw) To: linux-arm-kernel On Fri, Mar 03, 2017 at 01:27:10PM +0100, Jiri Slaby wrote: > There is code duplicated over all architecture's headers for > futex_atomic_op_inuser. Namely op decoding, access_ok check for uaddr, > and comparison of the result. > > Remove this duplication and leave up to the arches only the needed > assembly which is now in arch_futex_atomic_op_inuser. > > Note that s390 removed access_ok check in d12a29703 ("s390/uaccess: > remove pointless access_ok() checks") as access_ok there returns true. > We introduce it back to the helper for the sake of simplicity (it gets > optimized away anyway). > > Signed-off-by: Jiri Slaby <jslaby@suse.cz> > --- > arch/s390/include/asm/futex.h | 23 ++++------------- > include/asm-generic/futex.h | 50 +++++++------------------------------ > kernel/futex.c | 36 ++++++++++++++++++++++++++ Looks good to me and still boots on s390. Therefore for the s390 bits: Acked-by: Heiko Carstens <heiko.carstens@de.ibm.com> Thanks! ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/3] futex: remove duplicated code [not found] <20170303122712.13353-1-jslaby@suse.cz> 2017-03-03 14:08 ` [PATCH 1/3] futex: remove duplicated code Heiko Carstens @ 2017-03-03 14:48 ` Chris Metcalf 2017-03-04 13:05 ` Russell King - ARM Linux 2017-03-06 1:52 ` Rich Felker 3 siblings, 0 replies; 6+ messages in thread From: Chris Metcalf @ 2017-03-03 14:48 UTC (permalink / raw) To: linux-arm-kernel On 3/3/2017 7:27 AM, Jiri Slaby wrote: > There is code duplicated over all architecture's headers for > futex_atomic_op_inuser. Namely op decoding, access_ok check for uaddr, > and comparison of the result. > > Remove this duplication and leave up to the arches only the needed > assembly which is now in arch_futex_atomic_op_inuser. > > Note that s390 removed access_ok check in d12a29703 ("s390/uaccess: > remove pointless access_ok() checks") as access_ok there returns true. > We introduce it back to the helper for the sake of simplicity (it gets > optimized away anyway). > > Signed-off-by: Jiri Slaby<jslaby@suse.cz> Acked-by: Chris Metcalf <cmetcalf@mellanox.com> [for tile] -- Chris Metcalf, Mellanox Technologies http://www.mellanox.com ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/3] futex: remove duplicated code [not found] <20170303122712.13353-1-jslaby@suse.cz> 2017-03-03 14:08 ` [PATCH 1/3] futex: remove duplicated code Heiko Carstens 2017-03-03 14:48 ` Chris Metcalf @ 2017-03-04 13:05 ` Russell King - ARM Linux 2017-03-04 19:15 ` H. Peter Anvin 2017-03-06 1:52 ` Rich Felker 3 siblings, 1 reply; 6+ messages in thread From: Russell King - ARM Linux @ 2017-03-04 13:05 UTC (permalink / raw) To: linux-arm-kernel On Fri, Mar 03, 2017 at 01:27:10PM +0100, Jiri Slaby wrote: > diff --git a/arch/arm/include/asm/futex.h b/arch/arm/include/asm/futex.h > index 6795368ad023..cc414382dab4 100644 > --- a/arch/arm/include/asm/futex.h > +++ b/arch/arm/include/asm/futex.h > @@ -128,20 +128,10 @@ futex_atomic_cmpxchg_inatomic(u32 *uval, u32 __user *uaddr, > #endif /* !SMP */ > > static inline int > -futex_atomic_op_inuser (int encoded_op, u32 __user *uaddr) > +arch_futex_atomic_op_inuser(int op, int oparg, int *oval, u32 __user *uaddr) > { > - int op = (encoded_op >> 28) & 7; > - int cmp = (encoded_op >> 24) & 15; > - int oparg = (encoded_op << 8) >> 20; > - int cmparg = (encoded_op << 20) >> 20; > int oldval = 0, ret, tmp; > > - if (encoded_op & (FUTEX_OP_OPARG_SHIFT << 28)) > - oparg = 1 << oparg; > - > - if (!access_ok(VERIFY_WRITE, uaddr, sizeof(u32))) > - return -EFAULT; > - > #ifndef CONFIG_SMP > preempt_disable(); > #endif > @@ -172,17 +162,9 @@ futex_atomic_op_inuser (int encoded_op, u32 __user *uaddr) > preempt_enable(); > #endif > > - if (!ret) { > - switch (cmp) { > - case FUTEX_OP_CMP_EQ: ret = (oldval == cmparg); break; > - case FUTEX_OP_CMP_NE: ret = (oldval != cmparg); break; > - case FUTEX_OP_CMP_LT: ret = (oldval < cmparg); break; > - case FUTEX_OP_CMP_GE: ret = (oldval >= cmparg); break; > - case FUTEX_OP_CMP_LE: ret = (oldval <= cmparg); break; > - case FUTEX_OP_CMP_GT: ret = (oldval > cmparg); break; > - default: ret = -ENOSYS; > - } > - } > + if (!ret) > + *oval = oldval; > + > return ret; > } > > diff --git a/kernel/futex.c b/kernel/futex.c > index b687cb22301c..c5ff9850952f 100644 > --- a/kernel/futex.c > +++ b/kernel/futex.c > @@ -1457,6 +1457,42 @@ futex_wake(u32 __user *uaddr, unsigned int flags, int nr_wake, u32 bitset) > return ret; > } > > +static int futex_atomic_op_inuser(int encoded_op, u32 __user *uaddr) > +{ > + int op = (encoded_op >> 28) & 7; > + int cmp = (encoded_op >> 24) & 15; > + int oparg = (encoded_op << 8) >> 20; > + int cmparg = (encoded_op << 20) >> 20; Hmm. oparg and cmparg look like they're doing these shifts to get sign extension of the 12-bit values by assuming that "int" is 32-bit - probably worth a comment, or for safety, they should be "s32" so it's not dependent on the bit-width of "int". > + int oldval, ret; > + > + if (encoded_op & (FUTEX_OP_OPARG_SHIFT << 28)) > + oparg = 1 << oparg; I guess it doesn't matter that oparg can be >= the bit size of oparg (so large values produce an undefined result) as it's no different from userspace trying to do the same with large shifts. > + > + if (!access_ok(VERIFY_WRITE, uaddr, sizeof(u32))) > + return -EFAULT; > + > + ret = arch_futex_atomic_op_inuser(op, oparg, &oldval, uaddr); > + if (ret) > + return ret; > + > + switch (cmp) { > + case FUTEX_OP_CMP_EQ: > + return oldval == cmparg; > + case FUTEX_OP_CMP_NE: > + return oldval != cmparg; > + case FUTEX_OP_CMP_LT: > + return oldval < cmparg; > + case FUTEX_OP_CMP_GE: > + return oldval >= cmparg; > + case FUTEX_OP_CMP_LE: > + return oldval <= cmparg; > + case FUTEX_OP_CMP_GT: > + return oldval > cmparg; > + default: > + return -ENOSYS; > + } > +} > + > /* > * Wake up all waiters hashed on the physical page that is mapped > * to this virtual address: As it's no worse than our existing code, for the above, Acked-by: Russell King <rmk+kernel@armlinux.org.uk> Thanks. -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net. ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/3] futex: remove duplicated code 2017-03-04 13:05 ` Russell King - ARM Linux @ 2017-03-04 19:15 ` H. Peter Anvin 2017-03-04 21:38 ` Stafford Horne 0 siblings, 1 reply; 6+ messages in thread From: H. Peter Anvin @ 2017-03-04 19:15 UTC (permalink / raw) To: linux-arm-kernel On 03/04/17 05:05, Russell King - ARM Linux wrote: >> >> +static int futex_atomic_op_inuser(int encoded_op, u32 __user *uaddr) >> +{ >> + int op = (encoded_op >> 28) & 7; >> + int cmp = (encoded_op >> 24) & 15; >> + int oparg = (encoded_op << 8) >> 20; >> + int cmparg = (encoded_op << 20) >> 20; > > Hmm. oparg and cmparg look like they're doing these shifts to get sign > extension of the 12-bit values by assuming that "int" is 32-bit - > probably worth a comment, or for safety, they should be "s32" so it's > not dependent on the bit-width of "int". > For readability, perhaps we should make sign- and zero-extension an explicit facility? /* * Truncate an integer x to n bits, using sign- or * zero-extension, respectively. */ static inline __const_func__ s32 sex32(s32 x, int n) { return (x << (32-n)) >> (32-n); } static inline __const_func__ s64 sex64(s64 x, int n) { return (x << (64-n)) >> (64-n); } #define sex(x,y) \ ((__typeof__(x)) \ (((__builtin_constant_p(y) && ((y) <= 32)) || \ (sizeof(x) <= sizeof(s32))) \ ? sex32((x),(y)) : sex64((x),(y)))) static inline __const_func__ u32 zex32(u32 x, int n) { return (x << (32-n)) >> (32-n); } static inline __const_func__ u64 zex64(u64 x, int n) { return (x << (64-n)) >> (64-n); } #define zex(x,y) \ ((__typeof__(x)) \ (((__builtin_constant_p(y) && ((y) <= 32)) || \ (sizeof(x) <= sizeof(u32))) \ ? zex32((x),(y)) : zex64((x),(y)))) ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/3] futex: remove duplicated code 2017-03-04 19:15 ` H. Peter Anvin @ 2017-03-04 21:38 ` Stafford Horne 0 siblings, 0 replies; 6+ messages in thread From: Stafford Horne @ 2017-03-04 21:38 UTC (permalink / raw) To: linux-arm-kernel On Sat, Mar 04, 2017 at 11:15:17AM -0800, H. Peter Anvin wrote: > On 03/04/17 05:05, Russell King - ARM Linux wrote: > >> > >> +static int futex_atomic_op_inuser(int encoded_op, u32 __user *uaddr) > >> +{ > >> + int op = (encoded_op >> 28) & 7; > >> + int cmp = (encoded_op >> 24) & 15; > >> + int oparg = (encoded_op << 8) >> 20; > >> + int cmparg = (encoded_op << 20) >> 20; > > > > Hmm. oparg and cmparg look like they're doing these shifts to get sign > > extension of the 12-bit values by assuming that "int" is 32-bit - > > probably worth a comment, or for safety, they should be "s32" so it's > > not dependent on the bit-width of "int". > > > > For readability, perhaps we should make sign- and zero-extension an > explicit facility? There is some of this in already here, 32 and 64 bit versions: include/linux/bitops.h Do we really need zero extension? It seems the same. Example implementation from bitops.h static inline __s32 sign_extend32(__u32 value, int index) { __u8 shift = 31 - index; return (__s32)(value << shift) >> shift; } > /* > * Truncate an integer x to n bits, using sign- or > * zero-extension, respectively. > */ > static inline __const_func__ s32 sex32(s32 x, int n) > { > return (x << (32-n)) >> (32-n); > } > > static inline __const_func__ s64 sex64(s64 x, int n) > { > return (x << (64-n)) >> (64-n); > } > > #define sex(x,y) \ > ((__typeof__(x)) \ > (((__builtin_constant_p(y) && ((y) <= 32)) || \ > (sizeof(x) <= sizeof(s32))) \ > ? sex32((x),(y)) : sex64((x),(y)))) > > static inline __const_func__ u32 zex32(u32 x, int n) > { > return (x << (32-n)) >> (32-n); > } > > static inline __const_func__ u64 zex64(u64 x, int n) > { > return (x << (64-n)) >> (64-n); > } > > #define zex(x,y) \ > ((__typeof__(x)) \ > (((__builtin_constant_p(y) && ((y) <= 32)) || \ > (sizeof(x) <= sizeof(u32))) \ > ? zex32((x),(y)) : zex64((x),(y)))) > ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/3] futex: remove duplicated code [not found] <20170303122712.13353-1-jslaby@suse.cz> ` (2 preceding siblings ...) 2017-03-04 13:05 ` Russell King - ARM Linux @ 2017-03-06 1:52 ` Rich Felker 3 siblings, 0 replies; 6+ messages in thread From: Rich Felker @ 2017-03-06 1:52 UTC (permalink / raw) To: linux-arm-kernel On Fri, Mar 03, 2017 at 01:27:10PM +0100, Jiri Slaby wrote: > There is code duplicated over all architecture's headers for > futex_atomic_op_inuser. Namely op decoding, access_ok check for uaddr, > and comparison of the result. > > Remove this duplication and leave up to the arches only the needed > assembly which is now in arch_futex_atomic_op_inuser. > > Note that s390 removed access_ok check in d12a29703 ("s390/uaccess: > remove pointless access_ok() checks") as access_ok there returns true. > We introduce it back to the helper for the sake of simplicity (it gets > optimized away anyway). Overall I'm in favor of this patch, and it's close to what I had in mind in the commit message for 00b73d8d1b7131da03aec73011a7286f566fe87f. But I'd actually like to see it go further. These ops are mainly (only?) used for the (almost never used) FUTEX_WAKE_OP operation, and there's very little sense in trying to optimize them with dedicated arch-specific forms like "lock xadd". Instead the entire logic should be in an arch-generic file, and all the arch should need to provide is a cmpxchg-on-user-memory primitive for it to use. On most archs, the same cmpxchg used in kernelspace should also work for user addresses, meaning a huge amount of unmaintained, largely untested, junk code can be removed. Rich ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-03-06 1:52 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20170303122712.13353-1-jslaby@suse.cz>
2017-03-03 14:08 ` [PATCH 1/3] futex: remove duplicated code Heiko Carstens
2017-03-03 14:48 ` Chris Metcalf
2017-03-04 13:05 ` Russell King - ARM Linux
2017-03-04 19:15 ` H. Peter Anvin
2017-03-04 21:38 ` Stafford Horne
2017-03-06 1:52 ` Rich Felker
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).