| Message ID | 1438935406-5762-1-git-send-email-vgupta@synopsys.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 08/07, Vineet Gupta wrote: > > --- a/fs/exec.c > +++ b/fs/exec.c > @@ -1690,15 +1690,10 @@ EXPORT_SYMBOL(set_binfmt); > */ > void set_dumpable(struct mm_struct *mm, int value) > { > - unsigned long old, new; > - > if (WARN_ON((unsigned)value > SUID_DUMP_ROOT)) > return; > > - do { > - old = ACCESS_ONCE(mm->flags); > - new = (old & ~MMF_DUMPABLE_MASK) | value; > - } while (cmpxchg(&mm->flags, old, new) != old); > + set_mask_bits(&mm->flags, MMF_DUMPABLE_MASK, value); > } Acked-by: Oleg Nesterov <oleg@redhat.com> -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Friday 07 August 2015 05:27 PM, Oleg Nesterov wrote: > On 08/07, Vineet Gupta wrote: >> >> --- a/fs/exec.c >> +++ b/fs/exec.c >> @@ -1690,15 +1690,10 @@ EXPORT_SYMBOL(set_binfmt); >> */ >> void set_dumpable(struct mm_struct *mm, int value) >> { >> - unsigned long old, new; >> - >> if (WARN_ON((unsigned)value > SUID_DUMP_ROOT)) >> return; >> >> - do { >> - old = ACCESS_ONCE(mm->flags); >> - new = (old & ~MMF_DUMPABLE_MASK) | value; >> - } while (cmpxchg(&mm->flags, old, new) != old); >> + set_mask_bits(&mm->flags, MMF_DUMPABLE_MASK, value); >> } > > Acked-by: Oleg Nesterov <oleg@redhat.com> I have a fundamental question though, perhaps stupid, do use cases like these warrant the data to be atomic_t in first place. Do API like set_mask_bits() make sense at all - or shd they be moved to atomic_* (after changing the underlying data) See, I have such a cmpxchg loop in ARC code - originally from Peter :-) arch/arc/kernel/smp.c. @ipi_data_ptr is NOT atomic_t do { new = old = ACCESS_ONCE(*ipi_data_ptr); new |= 1U << msg; } while (cmpxchg(ipi_data_ptr, old, new) != old); Given that ARC (and some other RISC cores) lack native cmpxchg, we use LLSC instructions to implement atomics including cpmxchg - the implementation itself ensures loop is builtin making the outer loping superfluous and waste of cycles (see e.g. cover letter @ http://www.spinics.net/lists/kernel/msg2029217.html) So I wanted to convert that loop (and similar other cases to "some" API which could be built conditionally based on cmpxchg or llsc. None such exist and I was thinking of converting my case to atomic_t. Is that the right approach ? Thx, -Vineet -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Aug 07, 2015 at 08:14:03PM +0530, Vineet Gupta wrote: > See, I have such a cmpxchg loop in ARC code - originally from Peter :-) > arch/arc/kernel/smp.c. @ipi_data_ptr is NOT atomic_t > > do { > new = old = ACCESS_ONCE(*ipi_data_ptr); > new |= 1U << msg; > } while (cmpxchg(ipi_data_ptr, old, new) != old); > Well, you'll have atomic_or() real soon now. -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Friday 07 August 2015 08:27 PM, Peter Zijlstra wrote: > On Fri, Aug 07, 2015 at 08:14:03PM +0530, Vineet Gupta wrote: > >> > See, I have such a cmpxchg loop in ARC code - originally from Peter :-) >> > arch/arc/kernel/smp.c. @ipi_data_ptr is NOT atomic_t >> > >> > do { >> > new = old = ACCESS_ONCE(*ipi_data_ptr); >> > new |= 1U << msg; >> > } while (cmpxchg(ipi_data_ptr, old, new) != old); >> > > Well, you'll have atomic_or() real soon now. Doesn't help my cause - ipi_data_ptr is not atomic_t - hence my prev question in this thread -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Aug 07, 2015 at 09:05:06PM +0530, Vineet Gupta wrote: > On Friday 07 August 2015 08:27 PM, Peter Zijlstra wrote: > > On Fri, Aug 07, 2015 at 08:14:03PM +0530, Vineet Gupta wrote: > > > >> > See, I have such a cmpxchg loop in ARC code - originally from Peter :-) > >> > arch/arc/kernel/smp.c. @ipi_data_ptr is NOT atomic_t > >> > > >> > do { > >> > new = old = ACCESS_ONCE(*ipi_data_ptr); > >> > new |= 1U << msg; > >> > } while (cmpxchg(ipi_data_ptr, old, new) != old); > >> > > > Well, you'll have atomic_or() real soon now. > > Doesn't help my cause - ipi_data_ptr is not atomic_t - hence my prev question in > this thread A cast will work :-) But yes, ideally everything will be type safe because of those archs that cannot have atomic RmW ops like !ARC_HAS_LLSC. Mixing cmpxchg()/xchg() with regular stores is broken on those. Fwiw, you might want to set ARCH_SUPPORTS_ATOMIC_RMW for ARC_HAS_LLSC. -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Friday 07 August 2015 09:15 PM, Peter Zijlstra wrote: > On Fri, Aug 07, 2015 at 09:05:06PM +0530, Vineet Gupta wrote: >> On Friday 07 August 2015 08:27 PM, Peter Zijlstra wrote: >>> On Fri, Aug 07, 2015 at 08:14:03PM +0530, Vineet Gupta wrote: >>> >>>>> See, I have such a cmpxchg loop in ARC code - originally from Peter :-) >>>>> arch/arc/kernel/smp.c. @ipi_data_ptr is NOT atomic_t >>>>> >>>>> do { >>>>> new = old = ACCESS_ONCE(*ipi_data_ptr); >>>>> new |= 1U << msg; >>>>> } while (cmpxchg(ipi_data_ptr, old, new) != old); >>>>> >>> Well, you'll have atomic_or() real soon now. >> >> Doesn't help my cause - ipi_data_ptr is not atomic_t - hence my prev question in >> this thread > > A cast will work :-) > How ? We have typedef struct { int counter; } atomic_t; > But yes, ideally everything will be type safe because of those archs > that cannot have atomic RmW ops like !ARC_HAS_LLSC. Type safe - how / what ? > > Mixing cmpxchg()/xchg() with regular stores is broken on those. Right, but how does that relate to this discussion - perhaps I shd stop talking - long friday already :-) > > Fwiw, you might want to set ARCH_SUPPORTS_ATOMIC_RMW for ARC_HAS_LLSC. > Sure I will ! -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Aug 07, 2015 at 09:28:05PM +0530, Vineet Gupta wrote: > On Friday 07 August 2015 09:15 PM, Peter Zijlstra wrote: > > On Fri, Aug 07, 2015 at 09:05:06PM +0530, Vineet Gupta wrote: > >> On Friday 07 August 2015 08:27 PM, Peter Zijlstra wrote: > >>> On Fri, Aug 07, 2015 at 08:14:03PM +0530, Vineet Gupta wrote: > >>> > >>>>> See, I have such a cmpxchg loop in ARC code - originally from Peter :-) > >>>>> arch/arc/kernel/smp.c. @ipi_data_ptr is NOT atomic_t > >>>>> > >>>>> do { > >>>>> new = old = ACCESS_ONCE(*ipi_data_ptr); > >>>>> new |= 1U << msg; > >>>>> } while (cmpxchg(ipi_data_ptr, old, new) != old); > >>>>> > >>> Well, you'll have atomic_or() real soon now. > >> > >> Doesn't help my cause - ipi_data_ptr is not atomic_t - hence my prev question in > >> this thread > > > > A cast will work :-) > > > > How ? We have > > typedef struct { > int counter; > } atomic_t; ARC is 32bit, right? So int and unsigned long are of the same size. Therefore: atomic_or(1 << msg, (atomic_t *)ipi_data_ptr); Ugly, yes, but it should DTRT. > > But yes, ideally everything will be type safe because of those archs > > that cannot have atomic RmW ops like !ARC_HAS_LLSC. > > Type safe - how / what ? All atomic stuff restricted to atomic*t and bitmap functions (and ideally we'd also have bitmap_t to avoid passing random unsigned long * into bitmap functions and praying it all works, we do, and it doesn't, well mostly :-). > > Mixing cmpxchg()/xchg() with regular stores is broken on those. > > Right, but how does that relate to this discussion - perhaps I shd stop talking - > long friday already :-) :-) Well, its a very good argument for why we should not use cmpxchg/xchg on !atomic*t types, and therefore why the function at hand (set_mask_bit) should really be on an atomic_t. That said, it will probably make the fs code fugly for having to use atomic_t and all its accessors all over the place. -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/exec.c b/fs/exec.c index 1977c2a553ac..25078627f048 100644 --- a/fs/exec.c +++ b/fs/exec.c @@ -1690,15 +1690,10 @@ EXPORT_SYMBOL(set_binfmt); */ void set_dumpable(struct mm_struct *mm, int value) { - unsigned long old, new; - if (WARN_ON((unsigned)value > SUID_DUMP_ROOT)) return; - do { - old = ACCESS_ONCE(mm->flags); - new = (old & ~MMF_DUMPABLE_MASK) | value; - } while (cmpxchg(&mm->flags, old, new) != old); + set_mask_bits(&mm->flags, MMF_DUMPABLE_MASK, value); } SYSCALL_DEFINE3(execve,
Cc: Alexander Viro <viro@zeniv.linux.org.uk> Cc: Oleg Nesterov <oleg@redhat.com> Cc: Peter Zijlstra (Intel) <peterz@infradead.org> Cc: linux-fsdevel@vger.kernel.org Cc: linux-kernel@vger.kernel.org Signed-off-by: Vineet Gupta <vgupta@synopsys.com> --- fs/exec.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-)