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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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





[Index of Archives]     [Bluez Devel]     [Linux Wireless Networking]     [Linux Wireless Personal Area Networking]     [Linux ATH6KL]     [Linux USB Devel]     [Linux Media Drivers]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Big List of Linux Books]

  Powered by Linux