| Message ID | 20210210144556.10932-1-longman@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | locking/arch: Move qrwlock.h include after qspinlock.h | expand |
On 2/10/21 6:45 AM, Waiman Long wrote: > The queued rwlock code has a dependency on the current spinlock > implementation (likely to be qspinlock), but not vice versa. Including > qrwlock.h before qspinlock.h can be problematic when expanding qrwlock > functionality. > > If both qspinlock.h and qrwlock.h are to be included, the qrwlock.h > include should always be after qspinlock.h. Update the current set of > asm/spinlock.h files to enforce that. > > Signed-off-by: Waiman Long <longman@redhat.com> There should be a Fixes: tag here. If the SHA of the offending commit is not stable, there should be a better reference than "The queued rwlock code". This patch fixes the build problem I had observed on mips. I also tested xtensa:defconfig and arm64:defconfig with no problems observed. Tested-by: Guenter Roeck <linux@roeck-us.net> Guenter > --- > arch/arm64/include/asm/spinlock.h | 2 +- > arch/mips/include/asm/spinlock.h | 2 +- > arch/xtensa/include/asm/spinlock.h | 2 +- > 3 files changed, 3 insertions(+), 3 deletions(-) > > diff --git a/arch/arm64/include/asm/spinlock.h b/arch/arm64/include/asm/spinlock.h > index 9083d6992603..0525c0b089ed 100644 > --- a/arch/arm64/include/asm/spinlock.h > +++ b/arch/arm64/include/asm/spinlock.h > @@ -5,8 +5,8 @@ > #ifndef __ASM_SPINLOCK_H > #define __ASM_SPINLOCK_H > > -#include <asm/qrwlock.h> > #include <asm/qspinlock.h> > +#include <asm/qrwlock.h> > > /* See include/linux/spinlock.h */ > #define smp_mb__after_spinlock() smp_mb() > diff --git a/arch/mips/include/asm/spinlock.h b/arch/mips/include/asm/spinlock.h > index 8a88eb265516..6ce2117e49f6 100644 > --- a/arch/mips/include/asm/spinlock.h > +++ b/arch/mips/include/asm/spinlock.h > @@ -10,7 +10,6 @@ > #define _ASM_SPINLOCK_H > > #include <asm/processor.h> > -#include <asm/qrwlock.h> > > #include <asm-generic/qspinlock_types.h> > > @@ -27,5 +26,6 @@ static inline void queued_spin_unlock(struct qspinlock *lock) > } > > #include <asm/qspinlock.h> > +#include <asm/qrwlock.h> > > #endif /* _ASM_SPINLOCK_H */ > diff --git a/arch/xtensa/include/asm/spinlock.h b/arch/xtensa/include/asm/spinlock.h > index 584b0de6f2ca..41c449ece2d8 100644 > --- a/arch/xtensa/include/asm/spinlock.h > +++ b/arch/xtensa/include/asm/spinlock.h > @@ -12,8 +12,8 @@ > #define _XTENSA_SPINLOCK_H > > #include <asm/barrier.h> > -#include <asm/qrwlock.h> > #include <asm/qspinlock.h> > +#include <asm/qrwlock.h> > > #define smp_mb__after_spinlock() smp_mb() > >
On 2/10/21 10:05 AM, Guenter Roeck wrote: > On 2/10/21 6:45 AM, Waiman Long wrote: >> The queued rwlock code has a dependency on the current spinlock >> implementation (likely to be qspinlock), but not vice versa. Including >> qrwlock.h before qspinlock.h can be problematic when expanding qrwlock >> functionality. >> >> If both qspinlock.h and qrwlock.h are to be included, the qrwlock.h >> include should always be after qspinlock.h. Update the current set of >> asm/spinlock.h files to enforce that. >> >> Signed-off-by: Waiman Long <longman@redhat.com> > There should be a Fixes: tag here. If the SHA of the offending commit is not > stable, there should be a better reference than "The queued rwlock code". I originally have a Fixes tag when I was modifying the mips' asm/spinlock.h file. After I realize that there are more files to modify, I take that out. Anyway, the problem was exposed by Ben's qrwlock patch. So existing stable releases should still be fine without this patch. > > This patch fixes the build problem I had observed on mips. I also tested > xtensa:defconfig and arm64:defconfig with no problems observed. > > Tested-by: Guenter Roeck <linux@roeck-us.net> Thanks for the testing as I don't have a build environment to verify that. Cheers, Longman
On Wed, Feb 10, 2021 at 09:45:56AM -0500, Waiman Long wrote: > The queued rwlock code has a dependency on the current spinlock > implementation (likely to be qspinlock), but not vice versa. Including > qrwlock.h before qspinlock.h can be problematic when expanding qrwlock > functionality. > > If both qspinlock.h and qrwlock.h are to be included, the qrwlock.h > include should always be after qspinlock.h. Update the current set of > asm/spinlock.h files to enforce that. > > Signed-off-by: Waiman Long <longman@redhat.com> > --- > arch/arm64/include/asm/spinlock.h | 2 +- > arch/mips/include/asm/spinlock.h | 2 +- > arch/xtensa/include/asm/spinlock.h | 2 +- > 3 files changed, 3 insertions(+), 3 deletions(-) which tree should this go through ? I can take it via mips-next, if everybody agrees. Thomas.
On Wed, Feb 10, 2021 at 7:54 AM Waiman Long <longman@redhat.com> wrote: > > On 2/10/21 10:05 AM, Guenter Roeck wrote: > > On 2/10/21 6:45 AM, Waiman Long wrote: > >> The queued rwlock code has a dependency on the current spinlock > >> implementation (likely to be qspinlock), but not vice versa. Including > >> qrwlock.h before qspinlock.h can be problematic when expanding qrwlock > >> functionality. > >> > >> If both qspinlock.h and qrwlock.h are to be included, the qrwlock.h > >> include should always be after qspinlock.h. Update the current set of > >> asm/spinlock.h files to enforce that. > >> > >> Signed-off-by: Waiman Long <longman@redhat.com> > > There should be a Fixes: tag here. If the SHA of the offending commit is not > > stable, there should be a better reference than "The queued rwlock code". > I originally have a Fixes tag when I was modifying the mips' > asm/spinlock.h file. After I realize that there are more files to > modify, I take that out. Anyway, the problem was exposed by Ben's > qrwlock patch. So existing stable releases should still be fine without > this patch. > > > > This patch fixes the build problem I had observed on mips. I also tested > > xtensa:defconfig and arm64:defconfig with no problems observed. > > > > Tested-by: Guenter Roeck <linux@roeck-us.net> > > Thanks for the testing as I don't have a build environment to verify that. > > Cheers, > Longman > Thanks Longman and Guenter for developing and testing this fix! I don't have the environment to test this either, but the patch looks good to me. Reviewed-by: Ben Gardon <bgardon@google.com>
On 10/02/21 15:45, Waiman Long wrote: > The queued rwlock code has a dependency on the current spinlock > implementation (likely to be qspinlock), but not vice versa. Including > qrwlock.h before qspinlock.h can be problematic when expanding qrwlock > functionality. > > If both qspinlock.h and qrwlock.h are to be included, the qrwlock.h > include should always be after qspinlock.h. Update the current set of > asm/spinlock.h files to enforce that. > > Signed-off-by: Waiman Long <longman@redhat.com> arch/sparc/include/asm/spinlock_64.h is missing. Also, the include in kernel/locking/qrwlock.c is not necessary (it may be there for aesthetic reasons, but it complicates thing in this case). I'll send a v2 that is based on the kvm/next tree. Paolo > --- > arch/arm64/include/asm/spinlock.h | 2 +- > arch/mips/include/asm/spinlock.h | 2 +- > arch/xtensa/include/asm/spinlock.h | 2 +- > 3 files changed, 3 insertions(+), 3 deletions(-) > > diff --git a/arch/arm64/include/asm/spinlock.h b/arch/arm64/include/asm/spinlock.h > index 9083d6992603..0525c0b089ed 100644 > --- a/arch/arm64/include/asm/spinlock.h > +++ b/arch/arm64/include/asm/spinlock.h > @@ -5,8 +5,8 @@ > #ifndef __ASM_SPINLOCK_H > #define __ASM_SPINLOCK_H > > -#include <asm/qrwlock.h> > #include <asm/qspinlock.h> > +#include <asm/qrwlock.h> > > /* See include/linux/spinlock.h */ > #define smp_mb__after_spinlock() smp_mb() > diff --git a/arch/mips/include/asm/spinlock.h b/arch/mips/include/asm/spinlock.h > index 8a88eb265516..6ce2117e49f6 100644 > --- a/arch/mips/include/asm/spinlock.h > +++ b/arch/mips/include/asm/spinlock.h > @@ -10,7 +10,6 @@ > #define _ASM_SPINLOCK_H > > #include <asm/processor.h> > -#include <asm/qrwlock.h> > > #include <asm-generic/qspinlock_types.h> > > @@ -27,5 +26,6 @@ static inline void queued_spin_unlock(struct qspinlock *lock) > } > > #include <asm/qspinlock.h> > +#include <asm/qrwlock.h> > > #endif /* _ASM_SPINLOCK_H */ > diff --git a/arch/xtensa/include/asm/spinlock.h b/arch/xtensa/include/asm/spinlock.h > index 584b0de6f2ca..41c449ece2d8 100644 > --- a/arch/xtensa/include/asm/spinlock.h > +++ b/arch/xtensa/include/asm/spinlock.h > @@ -12,8 +12,8 @@ > #define _XTENSA_SPINLOCK_H > > #include <asm/barrier.h> > -#include <asm/qrwlock.h> > #include <asm/qspinlock.h> > +#include <asm/qrwlock.h> > > #define smp_mb__after_spinlock() smp_mb() > >
On 2/10/21 1:28 PM, Paolo Bonzini wrote: > On 10/02/21 15:45, Waiman Long wrote: >> The queued rwlock code has a dependency on the current spinlock >> implementation (likely to be qspinlock), but not vice versa. Including >> qrwlock.h before qspinlock.h can be problematic when expanding qrwlock >> functionality. >> >> If both qspinlock.h and qrwlock.h are to be included, the qrwlock.h >> include should always be after qspinlock.h. Update the current set of >> asm/spinlock.h files to enforce that. >> >> Signed-off-by: Waiman Long <longman@redhat.com> > > arch/sparc/include/asm/spinlock_64.h is missing. Also, the include in > kernel/locking/qrwlock.c is not necessary (it may be there for > aesthetic reasons, but it complicates thing in this case). Sorry for missing arch/sparc/include/asm/spinlock_64.h. I was just focusing on asm/spinlock.h and not aware that there are other variants there. It is true that the asm/qrwlock.h include in qrwlock.c is not really necessary. I can't recall why it was there. > > I'll send a v2 that is based on the kvm/next tree. > > Paolo > Thanks for taking care of that. Cheers, Longman
On 10/02/21 17:19, Thomas Bogendoerfer wrote: >> arch/arm64/include/asm/spinlock.h | 2 +- >> arch/mips/include/asm/spinlock.h | 2 +- >> arch/xtensa/include/asm/spinlock.h | 2 +- >> 3 files changed, 3 insertions(+), 3 deletions(-) > which tree should this go through ? I can take it via mips-next, > if everybody agrees. The breakage is in the KVM tree, and the existing patch has acked-by from the locking primitives folks. So I'll queue it there in order to limit the range that breaks bisection. Paolo
On Thu, Feb 11, 2021 at 01:59:35PM +0100, Paolo Bonzini wrote: > On 10/02/21 17:19, Thomas Bogendoerfer wrote: > > > arch/arm64/include/asm/spinlock.h | 2 +- > > > arch/mips/include/asm/spinlock.h | 2 +- > > > arch/xtensa/include/asm/spinlock.h | 2 +- > > > 3 files changed, 3 insertions(+), 3 deletions(-) > > which tree should this go through ? I can take it via mips-next, > > if everybody agrees. > > The breakage is in the KVM tree, and the existing patch has acked-by from > the locking primitives folks. So I'll queue it there in order to limit the > range that breaks bisection. if it's not too late you can add by Acked-by: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
diff --git a/arch/arm64/include/asm/spinlock.h b/arch/arm64/include/asm/spinlock.h index 9083d6992603..0525c0b089ed 100644 --- a/arch/arm64/include/asm/spinlock.h +++ b/arch/arm64/include/asm/spinlock.h @@ -5,8 +5,8 @@ #ifndef __ASM_SPINLOCK_H #define __ASM_SPINLOCK_H -#include <asm/qrwlock.h> #include <asm/qspinlock.h> +#include <asm/qrwlock.h> /* See include/linux/spinlock.h */ #define smp_mb__after_spinlock() smp_mb() diff --git a/arch/mips/include/asm/spinlock.h b/arch/mips/include/asm/spinlock.h index 8a88eb265516..6ce2117e49f6 100644 --- a/arch/mips/include/asm/spinlock.h +++ b/arch/mips/include/asm/spinlock.h @@ -10,7 +10,6 @@ #define _ASM_SPINLOCK_H #include <asm/processor.h> -#include <asm/qrwlock.h> #include <asm-generic/qspinlock_types.h> @@ -27,5 +26,6 @@ static inline void queued_spin_unlock(struct qspinlock *lock) } #include <asm/qspinlock.h> +#include <asm/qrwlock.h> #endif /* _ASM_SPINLOCK_H */ diff --git a/arch/xtensa/include/asm/spinlock.h b/arch/xtensa/include/asm/spinlock.h index 584b0de6f2ca..41c449ece2d8 100644 --- a/arch/xtensa/include/asm/spinlock.h +++ b/arch/xtensa/include/asm/spinlock.h @@ -12,8 +12,8 @@ #define _XTENSA_SPINLOCK_H #include <asm/barrier.h> -#include <asm/qrwlock.h> #include <asm/qspinlock.h> +#include <asm/qrwlock.h> #define smp_mb__after_spinlock() smp_mb()
The queued rwlock code has a dependency on the current spinlock implementation (likely to be qspinlock), but not vice versa. Including qrwlock.h before qspinlock.h can be problematic when expanding qrwlock functionality. If both qspinlock.h and qrwlock.h are to be included, the qrwlock.h include should always be after qspinlock.h. Update the current set of asm/spinlock.h files to enforce that. Signed-off-by: Waiman Long <longman@redhat.com> --- arch/arm64/include/asm/spinlock.h | 2 +- arch/mips/include/asm/spinlock.h | 2 +- arch/xtensa/include/asm/spinlock.h | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-)