[Question] bluetooth/{bnep,cmtp,hidp}: memory barriers

[Question] bluetooth/{bnep,cmtp,hidp}: memory barriers

[Andrea Parri]

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

Re: [Question] bluetooth/{bnep,cmtp,hidp}: memory barriers

[Brian Norris]

On Mon, Jul 30, 2018 at 05:10:30AM +0200, Andrea Parri wrote:
> Hi,

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:

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

> 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>

> 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 <jeffy.chen@rock-chips.com>
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

Re: [Question] bluetooth/{bnep,cmtp,hidp}: memory barriers

[JeffyChen]

Hi guys,

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

On 08/14/2018 07:18 AM, Brian Norris wrote:
>
> commit 5da8e47d849d3d37b14129f038782a095b9ad049
> Author: Jeffy Chen<jeffy.chen@rock-chips.com>
> 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.

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 :)

>
> 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).

Re: [Question] bluetooth/{bnep,cmtp,hidp}: memory barriers

[Andrea Parri]

Hi Jeffy, Brian,

On Tue, Aug 14, 2018 at 12:26:58PM +0800, JeffyChen wrote:
> Hi guys,
> 
> Thanks for your mails, and sorry for the late response..
> 
> On 08/14/2018 07:18 AM, Brian Norris wrote:
> >
> >commit 5da8e47d849d3d37b14129f038782a095b9ad049
> >Author: Jeffy Chen<jeffy.chen@rock-chips.com>
> >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.
> 
> 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 :)

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...


> 
> >
> >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).
> 
>