* [PATCH] arm: spinlock: const qualify read-only functions @ 2015-10-29 0:47 Ian Coolidge 2015-10-29 0:47 ` [PATCH] arm64: " Ian Coolidge 2015-10-31 16:30 ` [PATCH] arm: " Ard Biesheuvel 0 siblings, 2 replies; 10+ messages in thread From: Ian Coolidge @ 2015-10-29 0:47 UTC (permalink / raw) To: linux-arm-kernel This allows assert_spin_locked() to be used against spinlocks that are const qualified. Signed-off-by: Ian Coolidge <icoolidge@google.com> --- arch/arm/include/asm/spinlock.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/arch/arm/include/asm/spinlock.h b/arch/arm/include/asm/spinlock.h index 0fa4184..3512d2d 100644 --- a/arch/arm/include/asm/spinlock.h +++ b/arch/arm/include/asm/spinlock.h @@ -113,17 +113,17 @@ static inline void arch_spin_unlock(arch_spinlock_t *lock) dsb_sev(); } -static inline int arch_spin_value_unlocked(arch_spinlock_t lock) +static inline int arch_spin_value_unlocked(const arch_spinlock_t lock) { return lock.tickets.owner == lock.tickets.next; } -static inline int arch_spin_is_locked(arch_spinlock_t *lock) +static inline int arch_spin_is_locked(const arch_spinlock_t *lock) { return !arch_spin_value_unlocked(READ_ONCE(*lock)); } -static inline int arch_spin_is_contended(arch_spinlock_t *lock) +static inline int arch_spin_is_contended(const arch_spinlock_t *lock) { struct __raw_tickets tickets = READ_ONCE(lock->tickets); return (tickets.next - tickets.owner) > 1; -- 2.6.0.rc2.230.g3dd15c0 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH] arm64: spinlock: const qualify read-only functions 2015-10-29 0:47 [PATCH] arm: spinlock: const qualify read-only functions Ian Coolidge @ 2015-10-29 0:47 ` Ian Coolidge 2015-10-31 16:30 ` [PATCH] arm: " Ard Biesheuvel 1 sibling, 0 replies; 10+ messages in thread From: Ian Coolidge @ 2015-10-29 0:47 UTC (permalink / raw) To: linux-arm-kernel This allows assert_spin_locked() to be used against spinlocks that are const qualified. Signed-off-by: Ian Coolidge <icoolidge@google.com> --- arch/arm64/include/asm/spinlock.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/arch/arm64/include/asm/spinlock.h b/arch/arm64/include/asm/spinlock.h index c85e96d..72a7f33 100644 --- a/arch/arm64/include/asm/spinlock.h +++ b/arch/arm64/include/asm/spinlock.h @@ -122,17 +122,17 @@ static inline void arch_spin_unlock(arch_spinlock_t *lock) : "memory"); } -static inline int arch_spin_value_unlocked(arch_spinlock_t lock) +static inline int arch_spin_value_unlocked(const arch_spinlock_t lock) { return lock.owner == lock.next; } -static inline int arch_spin_is_locked(arch_spinlock_t *lock) +static inline int arch_spin_is_locked(const arch_spinlock_t *lock) { return !arch_spin_value_unlocked(READ_ONCE(*lock)); } -static inline int arch_spin_is_contended(arch_spinlock_t *lock) +static inline int arch_spin_is_contended(const arch_spinlock_t *lock) { arch_spinlock_t lockval = READ_ONCE(*lock); return (lockval.next - lockval.owner) > 1; -- 2.6.0.rc2.230.g3dd15c0 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH] arm: spinlock: const qualify read-only functions 2015-10-29 0:47 [PATCH] arm: spinlock: const qualify read-only functions Ian Coolidge 2015-10-29 0:47 ` [PATCH] arm64: " Ian Coolidge @ 2015-10-31 16:30 ` Ard Biesheuvel 1 sibling, 0 replies; 10+ messages in thread From: Ard Biesheuvel @ 2015-10-31 16:30 UTC (permalink / raw) To: linux-arm-kernel On 29 October 2015 at 01:47, Ian Coolidge <icoolidge@google.com> wrote: > This allows assert_spin_locked() to be used against > spinlocks that are const qualified. > > Signed-off-by: Ian Coolidge <icoolidge@google.com> > --- > arch/arm/include/asm/spinlock.h | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/arch/arm/include/asm/spinlock.h b/arch/arm/include/asm/spinlock.h > index 0fa4184..3512d2d 100644 > --- a/arch/arm/include/asm/spinlock.h > +++ b/arch/arm/include/asm/spinlock.h > @@ -113,17 +113,17 @@ static inline void arch_spin_unlock(arch_spinlock_t *lock) > dsb_sev(); > } > > -static inline int arch_spin_value_unlocked(arch_spinlock_t lock) > +static inline int arch_spin_value_unlocked(const arch_spinlock_t lock) In this case, 'lock' is passed by value, so there is really no point in constifying it. > { > return lock.tickets.owner == lock.tickets.next; > } > > -static inline int arch_spin_is_locked(arch_spinlock_t *lock) > +static inline int arch_spin_is_locked(const arch_spinlock_t *lock) > { > return !arch_spin_value_unlocked(READ_ONCE(*lock)); > } > > -static inline int arch_spin_is_contended(arch_spinlock_t *lock) > +static inline int arch_spin_is_contended(const arch_spinlock_t *lock) > { > struct __raw_tickets tickets = READ_ONCE(lock->tickets); > return (tickets.next - tickets.owner) > 1; For these remaining cases, the 'const' only enforces that these functions may not change the state of the lock. I think this is of limited value for single-line static inline functions such as these. Could you explain your use case? Do you have code that only has access to constified references to arch_spinlock_t instances? -- Ard. ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] arm: spinlock: const qualify read-only functions @ 2015-10-28 19:44 Ian Coolidge 2015-10-29 21:51 ` Arnd Bergmann 2015-10-30 0:27 ` Ian Coolidge 0 siblings, 2 replies; 10+ messages in thread From: Ian Coolidge @ 2015-10-28 19:44 UTC (permalink / raw) To: linux-arm-kernel This allows assert_spin_locked() to be used against spinlocks that are const qualified. Signed-off-by: Ian Coolidge <icoolidge@google.com> --- arch/arm/include/asm/spinlock.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/arch/arm/include/asm/spinlock.h b/arch/arm/include/asm/spinlock.h index 0fa4184..3512d2d 100644 --- a/arch/arm/include/asm/spinlock.h +++ b/arch/arm/include/asm/spinlock.h @@ -113,17 +113,17 @@ static inline void arch_spin_unlock(arch_spinlock_t *lock) dsb_sev(); } -static inline int arch_spin_value_unlocked(arch_spinlock_t lock) +static inline int arch_spin_value_unlocked(const arch_spinlock_t lock) { return lock.tickets.owner == lock.tickets.next; } -static inline int arch_spin_is_locked(arch_spinlock_t *lock) +static inline int arch_spin_is_locked(const arch_spinlock_t *lock) { return !arch_spin_value_unlocked(READ_ONCE(*lock)); } -static inline int arch_spin_is_contended(arch_spinlock_t *lock) +static inline int arch_spin_is_contended(const arch_spinlock_t *lock) { struct __raw_tickets tickets = READ_ONCE(lock->tickets); return (tickets.next - tickets.owner) > 1; -- 2.6.0.rc2.230.g3dd15c0 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH] arm: spinlock: const qualify read-only functions 2015-10-28 19:44 Ian Coolidge @ 2015-10-29 21:51 ` Arnd Bergmann 2015-10-30 0:27 ` Ian Coolidge 1 sibling, 0 replies; 10+ messages in thread From: Arnd Bergmann @ 2015-10-29 21:51 UTC (permalink / raw) To: linux-arm-kernel On Wednesday 28 October 2015 12:44:54 Ian Coolidge wrote: > This allows assert_spin_locked() to be used against > spinlocks that are const qualified. > > Signed-off-by: Ian Coolidge <icoolidge@google.com> > Can you give an example in the changelog where this might be useful? Constant spinlocks seem counterintuitive. Arnd ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] arm: spinlock: const qualify read-only functions. 2015-10-28 19:44 Ian Coolidge 2015-10-29 21:51 ` Arnd Bergmann @ 2015-10-30 0:27 ` Ian Coolidge 2015-10-31 17:26 ` Ian Coolidge 1 sibling, 1 reply; 10+ messages in thread From: Ian Coolidge @ 2015-10-30 0:27 UTC (permalink / raw) To: linux-arm-kernel This allows assert_spin_locked() to be used against spinlocks that are const qualified. For example: struct instance_t { int data; spinlock_t lock; }; /* * foo does not mutate instance. * foo must be called while the lock is held. */ int foo(const instance_t *instance) { assert_spin_locked(&instance->lock); /* Code that assumes the lock is held */ return 0; } Signed-off-by: Ian Coolidge <icoolidge@google.com> --- arch/arm/include/asm/spinlock.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/arch/arm/include/asm/spinlock.h b/arch/arm/include/asm/spinlock.h index 0fa4184..274ceb1 100644 --- a/arch/arm/include/asm/spinlock.h +++ b/arch/arm/include/asm/spinlock.h @@ -118,12 +118,12 @@ static inline int arch_spin_value_unlocked(arch_spinlock_t lock) return lock.tickets.owner == lock.tickets.next; } -static inline int arch_spin_is_locked(arch_spinlock_t *lock) +static inline int arch_spin_is_locked(const arch_spinlock_t *lock) { return !arch_spin_value_unlocked(READ_ONCE(*lock)); } -static inline int arch_spin_is_contended(arch_spinlock_t *lock) +static inline int arch_spin_is_contended(const arch_spinlock_t *lock) { struct __raw_tickets tickets = READ_ONCE(lock->tickets); return (tickets.next - tickets.owner) > 1; -- 2.6.0.rc2.230.g3dd15c0 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH] arm: spinlock: const qualify read-only functions. 2015-10-30 0:27 ` Ian Coolidge @ 2015-10-31 17:26 ` Ian Coolidge 2015-10-31 18:34 ` Ard Biesheuvel 0 siblings, 1 reply; 10+ messages in thread From: Ian Coolidge @ 2015-10-31 17:26 UTC (permalink / raw) To: linux-arm-kernel On Thu, Oct 29, 2015 at 5:27 PM, Ian Coolidge <icoolidge@google.com> wrote: > This allows assert_spin_locked() to be used against > spinlocks that are const qualified. > > For example: > > struct instance_t { > int data; > spinlock_t lock; > }; > > /* > * foo does not mutate instance. > * foo must be called while the lock is held. > */ > int foo(const instance_t *instance) { > assert_spin_locked(&instance->lock); > > /* Code that assumes the lock is held */ > > return 0; > } > > Signed-off-by: Ian Coolidge <icoolidge@google.com> > --- > arch/arm/include/asm/spinlock.h | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/arch/arm/include/asm/spinlock.h b/arch/arm/include/asm/spinlock.h > index 0fa4184..274ceb1 100644 > --- a/arch/arm/include/asm/spinlock.h > +++ b/arch/arm/include/asm/spinlock.h > @@ -118,12 +118,12 @@ static inline int arch_spin_value_unlocked(arch_spinlock_t lock) > return lock.tickets.owner == lock.tickets.next; > } > > -static inline int arch_spin_is_locked(arch_spinlock_t *lock) > +static inline int arch_spin_is_locked(const arch_spinlock_t *lock) > { > return !arch_spin_value_unlocked(READ_ONCE(*lock)); > } > > -static inline int arch_spin_is_contended(arch_spinlock_t *lock) > +static inline int arch_spin_is_contended(const arch_spinlock_t *lock) > { > struct __raw_tickets tickets = READ_ONCE(lock->tickets); > return (tickets.next - tickets.owner) > 1; > -- > 2.6.0.rc2.230.g3dd15c0 > Arnd, Please see this patch for an updated version. I should have noted it is PATCH v2 and replied to the old thread, sorry about that. Anyways, I provided an example of a const spinlock. Here, in that example, the spinlock is not const itself, but it is part of a structure that may be const. I believe the example is a good one. Also, with regards to your feedback, I don't think the length of the function body is a good argument against promising not to mutate it. Thanks for taking a look, Ian ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] arm: spinlock: const qualify read-only functions. 2015-10-31 17:26 ` Ian Coolidge @ 2015-10-31 18:34 ` Ard Biesheuvel 2015-10-31 19:46 ` Ian Coolidge 0 siblings, 1 reply; 10+ messages in thread From: Ard Biesheuvel @ 2015-10-31 18:34 UTC (permalink / raw) To: linux-arm-kernel On 31 October 2015 at 18:26, Ian Coolidge <icoolidge@google.com> wrote: > On Thu, Oct 29, 2015 at 5:27 PM, Ian Coolidge <icoolidge@google.com> wrote: >> This allows assert_spin_locked() to be used against >> spinlocks that are const qualified. >> >> For example: >> >> struct instance_t { >> int data; >> spinlock_t lock; >> }; >> >> /* >> * foo does not mutate instance. >> * foo must be called while the lock is held. >> */ >> int foo(const instance_t *instance) { >> assert_spin_locked(&instance->lock); >> >> /* Code that assumes the lock is held */ >> >> return 0; >> } >> >> Signed-off-by: Ian Coolidge <icoolidge@google.com> >> --- >> arch/arm/include/asm/spinlock.h | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/arch/arm/include/asm/spinlock.h b/arch/arm/include/asm/spinlock.h >> index 0fa4184..274ceb1 100644 >> --- a/arch/arm/include/asm/spinlock.h >> +++ b/arch/arm/include/asm/spinlock.h >> @@ -118,12 +118,12 @@ static inline int arch_spin_value_unlocked(arch_spinlock_t lock) >> return lock.tickets.owner == lock.tickets.next; >> } >> >> -static inline int arch_spin_is_locked(arch_spinlock_t *lock) >> +static inline int arch_spin_is_locked(const arch_spinlock_t *lock) >> { >> return !arch_spin_value_unlocked(READ_ONCE(*lock)); >> } >> >> -static inline int arch_spin_is_contended(arch_spinlock_t *lock) >> +static inline int arch_spin_is_contended(const arch_spinlock_t *lock) >> { >> struct __raw_tickets tickets = READ_ONCE(lock->tickets); >> return (tickets.next - tickets.owner) > 1; >> -- >> 2.6.0.rc2.230.g3dd15c0 >> > > Arnd, > > Please see this patch for an updated version. I should have noted it > is PATCH v2 and replied to the old thread, sorry about that. > Anyways, I provided an example of a const spinlock. Here, in that > example, the spinlock is not const itself, but it is part of a > structure that may be const. > I believe the example is a good one. > > Also, with regards to your feedback, I don't think the length of the > function body is a good argument against promising not to mutate it. > Hi, Note that Ard (me) and Arnd are two different persons. I think your patch makes sense in itself, I just wonder if there are any real examples in the current codebase that would require it. Which struct is it that you are looking to constify, and in which context? Regards, Ard. ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] arm: spinlock: const qualify read-only functions. 2015-10-31 18:34 ` Ard Biesheuvel @ 2015-10-31 19:46 ` Ian Coolidge 2015-10-31 19:49 ` Ian Coolidge 0 siblings, 1 reply; 10+ messages in thread From: Ian Coolidge @ 2015-10-31 19:46 UTC (permalink / raw) To: linux-arm-kernel Hi Ard, :) D'oh, yes, I had assumed you were the same person. It is for a driver that I would eventually like to upstream. I see that there are many cases where assert_spin_lock is used, but perhaps driver developers are underutilizing const, or compiling the kernel with warnings tolerated. On Sat, Oct 31, 2015 at 11:34 AM, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > On 31 October 2015 at 18:26, Ian Coolidge <icoolidge@google.com> wrote: >> On Thu, Oct 29, 2015 at 5:27 PM, Ian Coolidge <icoolidge@google.com> wrote: >>> This allows assert_spin_locked() to be used against >>> spinlocks that are const qualified. >>> >>> For example: >>> >>> struct instance_t { >>> int data; >>> spinlock_t lock; >>> }; >>> >>> /* >>> * foo does not mutate instance. >>> * foo must be called while the lock is held. >>> */ >>> int foo(const instance_t *instance) { >>> assert_spin_locked(&instance->lock); >>> >>> /* Code that assumes the lock is held */ >>> >>> return 0; >>> } >>> >>> Signed-off-by: Ian Coolidge <icoolidge@google.com> >>> --- >>> arch/arm/include/asm/spinlock.h | 4 ++-- >>> 1 file changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/arch/arm/include/asm/spinlock.h b/arch/arm/include/asm/spinlock.h >>> index 0fa4184..274ceb1 100644 >>> --- a/arch/arm/include/asm/spinlock.h >>> +++ b/arch/arm/include/asm/spinlock.h >>> @@ -118,12 +118,12 @@ static inline int arch_spin_value_unlocked(arch_spinlock_t lock) >>> return lock.tickets.owner == lock.tickets.next; >>> } >>> >>> -static inline int arch_spin_is_locked(arch_spinlock_t *lock) >>> +static inline int arch_spin_is_locked(const arch_spinlock_t *lock) >>> { >>> return !arch_spin_value_unlocked(READ_ONCE(*lock)); >>> } >>> >>> -static inline int arch_spin_is_contended(arch_spinlock_t *lock) >>> +static inline int arch_spin_is_contended(const arch_spinlock_t *lock) >>> { >>> struct __raw_tickets tickets = READ_ONCE(lock->tickets); >>> return (tickets.next - tickets.owner) > 1; >>> -- >>> 2.6.0.rc2.230.g3dd15c0 >>> >> >> Arnd, >> >> Please see this patch for an updated version. I should have noted it >> is PATCH v2 and replied to the old thread, sorry about that. >> Anyways, I provided an example of a const spinlock. Here, in that >> example, the spinlock is not const itself, but it is part of a >> structure that may be const. >> I believe the example is a good one. >> >> Also, with regards to your feedback, I don't think the length of the >> function body is a good argument against promising not to mutate it. >> > > Hi, > > Note that Ard (me) and Arnd are two different persons. > > I think your patch makes sense in itself, I just wonder if there are > any real examples in the current codebase that would require it. Which > struct is it that you are looking to constify, and in which context? > > Regards, > Ard. ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] arm: spinlock: const qualify read-only functions. 2015-10-31 19:46 ` Ian Coolidge @ 2015-10-31 19:49 ` Ian Coolidge 0 siblings, 0 replies; 10+ messages in thread From: Ian Coolidge @ 2015-10-31 19:49 UTC (permalink / raw) To: linux-arm-kernel On Sat, Oct 31, 2015 at 12:46 PM, Ian Coolidge <icoolidge@google.com> wrote: > Hi Ard, > > :) D'oh, yes, I had assumed you were the same person. > > It is for a driver that I would eventually like to upstream. > > I see that there are many cases where assert_spin_lock is used, but > perhaps driver developers are underutilizing const, > or compiling the kernel with warnings tolerated. > >> Note that Ard (me) and Arnd are two different persons. >> >> I think your patch makes sense in itself, I just wonder if there are >> any real examples in the current codebase that would require it. Which >> struct is it that you are looking to constify, and in which context? >> >> Regards, >> Ard. Sorry for the top-post, and I meant 'assert_spin_locked'. ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2015-10-31 19:49 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2015-10-29 0:47 [PATCH] arm: spinlock: const qualify read-only functions Ian Coolidge 2015-10-29 0:47 ` [PATCH] arm64: " Ian Coolidge 2015-10-31 16:30 ` [PATCH] arm: " Ard Biesheuvel -- strict thread matches above, loose matches on Subject: below -- 2015-10-28 19:44 Ian Coolidge 2015-10-29 21:51 ` Arnd Bergmann 2015-10-30 0:27 ` Ian Coolidge 2015-10-31 17:26 ` Ian Coolidge 2015-10-31 18:34 ` Ard Biesheuvel 2015-10-31 19:46 ` Ian Coolidge 2015-10-31 19:49 ` Ian Coolidge
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).