2019-05-20 18:21:45

by Andrea Parri

[permalink] [raw]
Subject: [PATCH 0/4] Fix improper uses of smp_mb__{before,after}_atomic()

Hi all, this is a respin of:

https://lkml.kernel.org/r/1556568902-12464-1-git-send-email-andrea.parri@amarulasolutions.com

which includes the following main changes:

- add Reviewed-by: tags (Ming Lei)
- add inline comment (Zheng Yan)

(Applies to 5.2-rc1.) Remark/Disclaimer:

https://lkml.kernel.org/r/20190430164404.GA2874@andrea

Cheers Andrea

Cc: Rob Clark <[email protected]>
Cc: Sean Paul <[email protected]>
Cc: David Airlie <[email protected]>
Cc: Daniel Vetter <[email protected]>
Cc: Jordan Crouse <[email protected]>
Cc: Jens Axboe <[email protected]>
Cc: Omar Sandoval <[email protected]>
Cc: Ming Lei <[email protected]>
Cc: "Yan, Zheng" <[email protected]>
Cc: Sage Weil <[email protected]>
Cc: Ilya Dryomov <[email protected]>
Cc: "Paul E. McKenney" <[email protected]>
Cc: Peter Zijlstra <[email protected]>

Andrea Parri (4):
drm/msm: Fix improper uses of smp_mb__{before,after}_atomic()
bio: fix improper use of smp_mb__before_atomic()
sbitmap: fix improper use of smp_mb__before_atomic()
ceph: fix improper use of smp_mb__before_atomic()

drivers/gpu/drm/msm/adreno/a5xx_preempt.c | 4 ++--
fs/ceph/super.h | 7 ++++++-
include/linux/bio.h | 2 +-
lib/sbitmap.c | 2 +-
4 files changed, 10 insertions(+), 5 deletions(-)

--
2.7.4



2019-05-20 18:21:53

by Andrea Parri

[permalink] [raw]
Subject: [PATCH 2/4] bio: fix improper use of smp_mb__before_atomic()

This barrier only applies to the read-modify-write operations; in
particular, it does not apply to the atomic_set() primitive.

Replace the barrier with an smp_mb().

Fixes: dac56212e8127 ("bio: skip atomic inc/dec of ->bi_cnt for most use cases")
Cc: [email protected]
Reported-by: "Paul E. McKenney" <[email protected]>
Reported-by: Peter Zijlstra <[email protected]>
Signed-off-by: Andrea Parri <[email protected]>
Reviewed-by: Ming Lei <[email protected]>
Cc: Jens Axboe <[email protected]>
Cc: Ming Lei <[email protected]>
Cc: [email protected]
Cc: "Paul E. McKenney" <[email protected]>
Cc: Peter Zijlstra <[email protected]>
---
include/linux/bio.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/bio.h b/include/linux/bio.h
index ea73df36529ad..0f23b56826403 100644
--- a/include/linux/bio.h
+++ b/include/linux/bio.h
@@ -210,7 +210,7 @@ static inline void bio_cnt_set(struct bio *bio, unsigned int count)
{
if (count != 1) {
bio->bi_flags |= (1 << BIO_REFFED);
- smp_mb__before_atomic();
+ smp_mb();
}
atomic_set(&bio->__bi_cnt, count);
}
--
2.7.4


2019-05-20 18:21:57

by Andrea Parri

[permalink] [raw]
Subject: [PATCH 4/4] ceph: fix improper use of smp_mb__before_atomic()

This barrier only applies to the read-modify-write operations; in
particular, it does not apply to the atomic64_set() primitive.

Replace the barrier with an smp_mb().

Fixes: fdd4e15838e59 ("ceph: rework dcache readdir")
Cc: [email protected]
Reported-by: "Paul E. McKenney" <[email protected]>
Reported-by: Peter Zijlstra <[email protected]>
Signed-off-by: Andrea Parri <[email protected]>
Cc: "Yan, Zheng" <[email protected]>
Cc: Sage Weil <[email protected]>
Cc: Ilya Dryomov <[email protected]>
Cc: [email protected]
Cc: "Paul E. McKenney" <[email protected]>
Cc: Peter Zijlstra <[email protected]>
---
fs/ceph/super.h | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/fs/ceph/super.h b/fs/ceph/super.h
index 6edab9a750f8a..e02f4ff0be3f1 100644
--- a/fs/ceph/super.h
+++ b/fs/ceph/super.h
@@ -541,7 +541,12 @@ static inline void __ceph_dir_set_complete(struct ceph_inode_info *ci,
long long release_count,
long long ordered_count)
{
- smp_mb__before_atomic();
+ /*
+ * Makes sure operations that setup readdir cache (update page
+ * cache and i_size) are strongly ordered w.r.t. the following
+ * atomic64_set() operations.
+ */
+ smp_mb();
atomic64_set(&ci->i_complete_seq[0], release_count);
atomic64_set(&ci->i_complete_seq[1], ordered_count);
}
--
2.7.4


2019-05-20 18:21:59

by Andrea Parri

[permalink] [raw]
Subject: [PATCH 1/4] drm/msm: Fix improper uses of smp_mb__{before,after}_atomic()

These barriers only apply to the read-modify-write operations; in
particular, they do not apply to the atomic_set() primitive.

Replace the barriers with smp_mb()s.

Fixes: b1fc2839d2f92 ("drm/msm: Implement preemption for A5XX targets")
Cc: [email protected]
Reported-by: "Paul E. McKenney" <[email protected]>
Reported-by: Peter Zijlstra <[email protected]>
Signed-off-by: Andrea Parri <[email protected]>
Cc: Rob Clark <[email protected]>
Cc: Sean Paul <[email protected]>
Cc: David Airlie <[email protected]>
Cc: Daniel Vetter <[email protected]>
Cc: Jordan Crouse <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: "Paul E. McKenney" <[email protected]>
Cc: Peter Zijlstra <[email protected]>
---
drivers/gpu/drm/msm/adreno/a5xx_preempt.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/msm/adreno/a5xx_preempt.c b/drivers/gpu/drm/msm/adreno/a5xx_preempt.c
index 3d62310a535fb..ee0820ee0c664 100644
--- a/drivers/gpu/drm/msm/adreno/a5xx_preempt.c
+++ b/drivers/gpu/drm/msm/adreno/a5xx_preempt.c
@@ -39,10 +39,10 @@ static inline void set_preempt_state(struct a5xx_gpu *gpu,
* preemption or in the interrupt handler so barriers are needed
* before...
*/
- smp_mb__before_atomic();
+ smp_mb();
atomic_set(&gpu->preempt_state, new);
/* ... and after*/
- smp_mb__after_atomic();
+ smp_mb();
}

/* Write the most recent wptr for the given ring into the hardware */
--
2.7.4


2019-05-20 19:37:28

by Jens Axboe

[permalink] [raw]
Subject: Re: [PATCH 0/4] Fix improper uses of smp_mb__{before,after}_atomic()

On 5/20/19 11:23 AM, Andrea Parri wrote:
> Hi all, this is a respin of:
>
> https://lkml.kernel.org/r/1556568902-12464-1-git-send-email-andrea.parri@amarulasolutions.com
>
> which includes the following main changes:
>
> - add Reviewed-by: tags (Ming Lei)
> - add inline comment (Zheng Yan)
>
> (Applies to 5.2-rc1.) Remark/Disclaimer:
>
> https://lkml.kernel.org/r/20190430164404.GA2874@andrea

Applied 2-3 to the block tree, thanks.

--
Jens Axboe


2019-05-20 21:38:04

by Jordan Crouse

[permalink] [raw]
Subject: Re: [PATCH 1/4] drm/msm: Fix improper uses of smp_mb__{before,after}_atomic()

On Mon, May 20, 2019 at 07:23:55PM +0200, Andrea Parri wrote:
> These barriers only apply to the read-modify-write operations; in
> particular, they do not apply to the atomic_set() primitive.
>
> Replace the barriers with smp_mb()s.
>
> Fixes: b1fc2839d2f92 ("drm/msm: Implement preemption for A5XX targets")
> Cc: [email protected]
> Reported-by: "Paul E. McKenney" <[email protected]>
> Reported-by: Peter Zijlstra <[email protected]>
> Signed-off-by: Andrea Parri <[email protected]>
> Cc: Rob Clark <[email protected]>
> Cc: Sean Paul <[email protected]>
> Cc: David Airlie <[email protected]>
> Cc: Daniel Vetter <[email protected]>
> Cc: Jordan Crouse <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Cc: "Paul E. McKenney" <[email protected]>
> Cc: Peter Zijlstra <[email protected]>

I'll go ahead and ack this - I'm not super clued in on atomic barriers, but this
seems to be in the spirit of what we are trying to do to protect the atomic
value. Rob can disagree, of course.

Acked-by: Jordan Crouse <[email protected]>

> ---
> drivers/gpu/drm/msm/adreno/a5xx_preempt.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/adreno/a5xx_preempt.c b/drivers/gpu/drm/msm/adreno/a5xx_preempt.c
> index 3d62310a535fb..ee0820ee0c664 100644
> --- a/drivers/gpu/drm/msm/adreno/a5xx_preempt.c
> +++ b/drivers/gpu/drm/msm/adreno/a5xx_preempt.c
> @@ -39,10 +39,10 @@ static inline void set_preempt_state(struct a5xx_gpu *gpu,
> * preemption or in the interrupt handler so barriers are needed
> * before...
> */
> - smp_mb__before_atomic();
> + smp_mb();
> atomic_set(&gpu->preempt_state, new);
> /* ... and after*/
> - smp_mb__after_atomic();
> + smp_mb();
> }
>
> /* Write the most recent wptr for the given ring into the hardware */
> --
> 2.7.4
>

--
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

2019-05-21 02:19:40

by Yan, Zheng

[permalink] [raw]
Subject: Re: [PATCH 4/4] ceph: fix improper use of smp_mb__before_atomic()

On 5/21/19 1:23 AM, Andrea Parri wrote:
> This barrier only applies to the read-modify-write operations; in
> particular, it does not apply to the atomic64_set() primitive.
>
> Replace the barrier with an smp_mb().
>
> Fixes: fdd4e15838e59 ("ceph: rework dcache readdir")
> Cc: [email protected]
> Reported-by: "Paul E. McKenney" <[email protected]>
> Reported-by: Peter Zijlstra <[email protected]>
> Signed-off-by: Andrea Parri <[email protected]>
> Cc: "Yan, Zheng" <[email protected]>
> Cc: Sage Weil <[email protected]>
> Cc: Ilya Dryomov <[email protected]>
> Cc: [email protected]
> Cc: "Paul E. McKenney" <[email protected]>
> Cc: Peter Zijlstra <[email protected]>
> ---
> fs/ceph/super.h | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/fs/ceph/super.h b/fs/ceph/super.h
> index 6edab9a750f8a..e02f4ff0be3f1 100644
> --- a/fs/ceph/super.h
> +++ b/fs/ceph/super.h
> @@ -541,7 +541,12 @@ static inline void __ceph_dir_set_complete(struct ceph_inode_info *ci,
> long long release_count,
> long long ordered_count)
> {
> - smp_mb__before_atomic();
> + /*
> + * Makes sure operations that setup readdir cache (update page
> + * cache and i_size) are strongly ordered w.r.t. the following
> + * atomic64_set() operations.
> + */
> + smp_mb();
> atomic64_set(&ci->i_complete_seq[0], release_count);
> atomic64_set(&ci->i_complete_seq[1], ordered_count);
> }
>

Applied, thanks