Re: [PATCH v9 05/12] x86, mpx: on-demand kernel allocation of bounds tables
On Tue, 28 Oct 2014, Dave Hansen wrote: > On 10/24/2014 05:08 AM, Thomas Gleixner wrote: > > On Sun, 12 Oct 2014, Qiaowei Ren wrote: > >> + /* > >> + * Go poke the address of the new bounds table in to the > >> + * bounds directory entry out in userspace memory. Note: > >> + * we may race with another CPU instantiating the same table. > >> + * In that case the cmpxchg will see an unexpected > >> + * 'actual_old_val'. > >> + */ > >> + ret = user_atomic_cmpxchg_inatomic(_old_val, bd_entry, > >> + expected_old_val, bt_addr); > > > > This is fully preemptible non-atomic context, right? > > > > So this wants a proper comment, why using > > user_atomic_cmpxchg_inatomic() is the right thing to do here. > > Hey Thomas, > > How's this for a new comment? Does this cover the points you think need > clarified? > > > > The kernel has allocated a bounds table and needs to point the > (userspace-allocated) directory to it. The directory entry is the > *only* place we track that this table was allocated, so we essentially > use it instead of an kernel data structure for synchronization. A > copy_to_user()-style function would not give us the atomicity that we need. > > If two threads race to instantiate a table, the cmpxchg ensures we know > which one lost the race and that the loser frees the table that they > just allocated. Yup. That explains the cmpxchg. The other thing which puzzled me was that it calls user_atomic_cmpxchg_inatomic() but the context is not atomic at all. Its fully preemptible and actually we want it to be able to handle the fault. The implementation does that, just the function itself suggest something different. Thanks, tglx -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v9 05/12] x86, mpx: on-demand kernel allocation of bounds tables
On 10/24/2014 05:08 AM, Thomas Gleixner wrote: > On Sun, 12 Oct 2014, Qiaowei Ren wrote: >> +/* >> + * Go poke the address of the new bounds table in to the >> + * bounds directory entry out in userspace memory. Note: >> + * we may race with another CPU instantiating the same table. >> + * In that case the cmpxchg will see an unexpected >> + * 'actual_old_val'. >> + */ >> +ret = user_atomic_cmpxchg_inatomic(_old_val, bd_entry, >> + expected_old_val, bt_addr); > > This is fully preemptible non-atomic context, right? > > So this wants a proper comment, why using > user_atomic_cmpxchg_inatomic() is the right thing to do here. Hey Thomas, How's this for a new comment? Does this cover the points you think need clarified? The kernel has allocated a bounds table and needs to point the (userspace-allocated) directory to it. The directory entry is the *only* place we track that this table was allocated, so we essentially use it instead of an kernel data structure for synchronization. A copy_to_user()-style function would not give us the atomicity that we need. If two threads race to instantiate a table, the cmpxchg ensures we know which one lost the race and that the loser frees the table that they just allocated. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v9 05/12] x86, mpx: on-demand kernel allocation of bounds tables
On 10/24/2014 05:08 AM, Thomas Gleixner wrote: On Sun, 12 Oct 2014, Qiaowei Ren wrote: +/* + * Go poke the address of the new bounds table in to the + * bounds directory entry out in userspace memory. Note: + * we may race with another CPU instantiating the same table. + * In that case the cmpxchg will see an unexpected + * 'actual_old_val'. + */ +ret = user_atomic_cmpxchg_inatomic(actual_old_val, bd_entry, + expected_old_val, bt_addr); This is fully preemptible non-atomic context, right? So this wants a proper comment, why using user_atomic_cmpxchg_inatomic() is the right thing to do here. Hey Thomas, How's this for a new comment? Does this cover the points you think need clarified? The kernel has allocated a bounds table and needs to point the (userspace-allocated) directory to it. The directory entry is the *only* place we track that this table was allocated, so we essentially use it instead of an kernel data structure for synchronization. A copy_to_user()-style function would not give us the atomicity that we need. If two threads race to instantiate a table, the cmpxchg ensures we know which one lost the race and that the loser frees the table that they just allocated. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v9 05/12] x86, mpx: on-demand kernel allocation of bounds tables
On Tue, 28 Oct 2014, Dave Hansen wrote: On 10/24/2014 05:08 AM, Thomas Gleixner wrote: On Sun, 12 Oct 2014, Qiaowei Ren wrote: + /* + * Go poke the address of the new bounds table in to the + * bounds directory entry out in userspace memory. Note: + * we may race with another CPU instantiating the same table. + * In that case the cmpxchg will see an unexpected + * 'actual_old_val'. + */ + ret = user_atomic_cmpxchg_inatomic(actual_old_val, bd_entry, + expected_old_val, bt_addr); This is fully preemptible non-atomic context, right? So this wants a proper comment, why using user_atomic_cmpxchg_inatomic() is the right thing to do here. Hey Thomas, How's this for a new comment? Does this cover the points you think need clarified? The kernel has allocated a bounds table and needs to point the (userspace-allocated) directory to it. The directory entry is the *only* place we track that this table was allocated, so we essentially use it instead of an kernel data structure for synchronization. A copy_to_user()-style function would not give us the atomicity that we need. If two threads race to instantiate a table, the cmpxchg ensures we know which one lost the race and that the loser frees the table that they just allocated. Yup. That explains the cmpxchg. The other thing which puzzled me was that it calls user_atomic_cmpxchg_inatomic() but the context is not atomic at all. Its fully preemptible and actually we want it to be able to handle the fault. The implementation does that, just the function itself suggest something different. Thanks, tglx -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v9 05/12] x86, mpx: on-demand kernel allocation of bounds tables
On 10/24/2014 08:08 PM, Thomas Gleixner wrote: On Sun, 12 Oct 2014, Qiaowei Ren wrote: + /* +* Go poke the address of the new bounds table in to the +* bounds directory entry out in userspace memory. Note: +* we may race with another CPU instantiating the same table. +* In that case the cmpxchg will see an unexpected +* 'actual_old_val'. +*/ + ret = user_atomic_cmpxchg_inatomic(_old_val, bd_entry, + expected_old_val, bt_addr); This is fully preemptible non-atomic context, right? So this wants a proper comment, why using user_atomic_cmpxchg_inatomic() is the right thing to do here. Well, we will address it. Thanks, Qiaowei -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v9 05/12] x86, mpx: on-demand kernel allocation of bounds tables
On 10/24/2014 08:08 PM, Thomas Gleixner wrote: On Sun, 12 Oct 2014, Qiaowei Ren wrote: + /* +* Go poke the address of the new bounds table in to the +* bounds directory entry out in userspace memory. Note: +* we may race with another CPU instantiating the same table. +* In that case the cmpxchg will see an unexpected +* 'actual_old_val'. +*/ + ret = user_atomic_cmpxchg_inatomic(actual_old_val, bd_entry, + expected_old_val, bt_addr); This is fully preemptible non-atomic context, right? So this wants a proper comment, why using user_atomic_cmpxchg_inatomic() is the right thing to do here. Well, we will address it. Thanks, Qiaowei -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v9 05/12] x86, mpx: on-demand kernel allocation of bounds tables
On Sun, 12 Oct 2014, Qiaowei Ren wrote: > + /* > + * Go poke the address of the new bounds table in to the > + * bounds directory entry out in userspace memory. Note: > + * we may race with another CPU instantiating the same table. > + * In that case the cmpxchg will see an unexpected > + * 'actual_old_val'. > + */ > + ret = user_atomic_cmpxchg_inatomic(_old_val, bd_entry, > +expected_old_val, bt_addr); This is fully preemptible non-atomic context, right? So this wants a proper comment, why using user_atomic_cmpxchg_inatomic() is the right thing to do here. Thanks, tglx -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v9 05/12] x86, mpx: on-demand kernel allocation of bounds tables
On Sun, 12 Oct 2014, Qiaowei Ren wrote: + /* + * Go poke the address of the new bounds table in to the + * bounds directory entry out in userspace memory. Note: + * we may race with another CPU instantiating the same table. + * In that case the cmpxchg will see an unexpected + * 'actual_old_val'. + */ + ret = user_atomic_cmpxchg_inatomic(actual_old_val, bd_entry, +expected_old_val, bt_addr); This is fully preemptible non-atomic context, right? So this wants a proper comment, why using user_atomic_cmpxchg_inatomic() is the right thing to do here. Thanks, tglx -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
