bluetooth/{bnep,cmtp,hidp}: memory barriers

Question Andrea Parri 3 participants 4 messages Jul 30, 2018 – Aug 14, 2018

Subsystems (3)

Networking Network drivers Bluetooth drivers

lore

Share & tools

lore

Andrea Parri

author

2018-07-30 03:10 UTC #0 lore raw

Hi,

I'm currently puzzled by the the three calls to smp_mb__before_atomic() in bnep_session(), cmtp_session() and hidp_session_run() respectively:

On the one hand, these barriers provide no guarantee on the subsequent atomic_read(s->terminate) (as the comments preceding the barriers seem to suggest), because atomic_read() is not a read-modify-write.

On the other hand, I'm currently unable to say *why such an "mb" would be required: not being too familiar with this code, I figured I should ask before sending a patch. ;-)

Andrea

Brian Norris

2018-08-13 23:18 UTC #1 lore raw

Quoted 1 line · Andrea Parri

Hi,

Hi!

Quoted 2 lines

I'm currently puzzled by the the three calls to smp_mb__before_atomic() in bnep_session(), cmtp_session() and hidp_session_run() respectively:

For the curious: I believe Jeffy Chen added all of those.

Quoted 3 lines

On the one hand, these barriers provide no guarantee on the subsequent atomic_read(s->terminate) (as the comments preceding the barriers seem to suggest), because atomic_read() is not a read-modify-write.

I'll admit, I didn't notice that piece of the documentation when reviewing this the first time:

Documentation/atomic_t.txt
<quote>
The barriers:

smp_mb__{before,after}_atomic()

only apply to the RMW ops and can be used to augment/upgrade the ordering inherent to the used atomic op.
</quote>

Quoted 3 lines

On the other hand, I'm currently unable to say *why such an "mb" would be required: not being too familiar with this code, I figured I should ask before sending a patch. ;-)

I can't fully speak for Jeffy, but I expect based on the initial development of his patches like this one

commit 5da8e47d849d3d37b14129f038782a095b9ad049

Author: Jeffy Chen <[email protected]>
Date:   Tue Jun 27 17:34:44 2017 +0800

Bluetooth: hidp: fix possible might sleep error in hidp_session_thread

that *some* kind of barrier was stuck in there simply as a response to comments like this, that were going away:

-                *
-                * Note: set_current_state() performs any necessary
-                * memory-barriers for us.
                 */

- set_current_state(TASK_INTERRUPTIBLE);

+ /* Ensure session->terminate is updated */
+ smp_mb__before_atomic();

It was probably an attempt to fill in the gap for the set_current_state() (and comment) which was being removed. I believe Jeffy originally added more barriers in other places, but I convinced him not to.

I have to say, I'm not really up-to-speed on the use of manual barriers in Linux (it's much preferable when they're wrapped into higher-level data structures already), but I believe the main intention here is to ensure that any change to 'terminate' that happened during the previous "wait_woken()" would be visible to our atomic_read().

Looking into wait_woken(), I'm feeling like none of these additional barriers are necessary at all. I believe wait_woken() handles the visibility issues we care about (that if we were woken for termination, we'll see the terminating condition).

That's my two cents, even if it's only worth about two cents.

HTH,
Brian

JeffyChen

2018-08-14 04:26 UTC #2 lore raw

Hi guys,

Thanks for your mails, and sorry for the late response..

Quoted 20 lines · Brian Norris

commit 5da8e47d849d3d37b14129f038782a095b9ad049
Author: Jeffy Chen<[email protected]>
Date:   Tue Jun 27 17:34:44 2017 +0800
Bluetooth: hidp: fix possible might sleep error in hidp_session_thread
that*some* kind of barrier was stuck in there simply as a response to comments like this, that were going away:
(quoted diff) +2 -4
-                *
-                * Note: set_current_state() performs any necessary
-                * memory-barriers for us.
                  */
-               set_current_state(TASK_INTERRUPTIBLE);
 
+               /* Ensure session->terminate is updated */
+               smp_mb__before_atomic();

Quoted 4 lines

It was probably an attempt to fill in the gap for the set_current_state() (and comment) which was being removed. I believe Jeffy originally added more barriers in other places, but I convinced him not to.

right, i was trying to avoid losing memory-barriers when removing set_current_state and changing wake_up_process to wake_up_interruptible.

and checking these code again, it's true the smp_mb__before_atomic before atomic_read is not needed, the smp_mb after atomic_inc(&session->terminate) should be enough.

and as Brian point out, there's already an smp_store_mb at the end of wait_woken, i agree we can remove all the smp_mb__{before,after}_atomic() i wrongly added :)

Quoted 11 lines

I have to say, I'm not really up-to-speed on the use of manual barriers in Linux (it's much preferable when they're wrapped into higher-level data structures already), but I believe the main intention here is to ensure that any change to 'terminate' that happened during the previous "wait_woken()" would be visible to our atomic_read().
Looking into wait_woken(), I'm feeling like none of these additional barriers are necessary at all. I believe wait_woken() handles the visibility issues we care about (that if we were woken for termination, we'll see the terminating condition).

Andrea Parri

author

2018-08-14 18:33 UTC #3 lore raw

Hi Jeffy, Brian,

Quoted 33 lines · JeffyChen

Hi guys,
Thanks for your mails, and sorry for the late response..
Quoted 20 lines · Brian Norris
commit 5da8e47d849d3d37b14129f038782a095b9ad049
Author: Jeffy Chen<[email protected]>
Date:   Tue Jun 27 17:34:44 2017 +0800
Bluetooth: hidp: fix possible might sleep error in hidp_session_thread
that*some* kind of barrier was stuck in there simply as a response to comments like this, that were going away:
(quoted diff) +2 -4
-                *
-                * Note: set_current_state() performs any necessary
-                * memory-barriers for us.
                 */
-               set_current_state(TASK_INTERRUPTIBLE);
 
+               /* Ensure session->terminate is updated */
+               smp_mb__before_atomic();
Quoted 4 lines
It was probably an attempt to fill in the gap for the set_current_state() (and comment) which was being removed. I believe Jeffy originally added more barriers in other places, but I convinced him not to.
right, i was trying to avoid losing memory-barriers when removing set_current_state and changing wake_up_process to wake_up_interruptible.

Quoted 7 lines

and checking these code again, it's true the smp_mb__before_atomic before atomic_read is not needed, the smp_mb after atomic_inc(&session->terminate) should be enough.
and as Brian point out, there's already an smp_store_mb at the end of wait_woken, i agree we can remove all the smp_mb__{before,after}_atomic() i wrongly added :)

Thank you for checking this once again. I'll send out a patch removing these barriers shortly.

Andrea

P.S. I'm out of office for the next two weeks, so my replies could come with some delay until ~ -rc1...

Quoted 14 lines

Quoted 12 lines
I have to say, I'm not really up-to-speed on the use of manual barriers in Linux (it's much preferable when they're wrapped into higher-level data structures already), but I believe the main intention here is to ensure that any change to 'terminate' that happened during the previous "wait_woken()" would be visible to our atomic_read().
Looking into wait_woken(), I'm feeling like none of these additional barriers are necessary at all. I believe wait_woken() handles the visibility issues we care about (that if we were woken for termination, we'll see the terminating condition).