On Tue, Mar 30, 2021 at 10:31:15AM -0700, Dan Williams wrote: > On Tue, Mar 30, 2021 at 10:03 AM Jason Gunthorpe <[email protected]> wrote: > > > > On Tue, Mar 30, 2021 at 09:05:29AM -0700, Dan Williams wrote: > > > > > > If you can't clearly point to the *data* under RCU protection it is > > > > being used wrong. > > > > > > Agree. > > > > > > The data being protected is the value of > > > dev->kobj.state_in_sysfs. The > > > > So where is that read under: > > > > + idx = srcu_read_lock(&cxl_memdev_srcu); > > + rc = __cxl_memdev_ioctl(cxlmd, cmd, arg); > > + srcu_read_unlock(&cxl_memdev_srcu, idx); > > > > ? > > device_is_registered() inside __cxl_memdev_ioctl().
Oh, I see, I missed that > > It can't read the RCU protected data outside the RCU critical region, > > and it can't read/write RCU protected data without using the helper > > macros which insert the required barriers. > > The required barriers are there. srcu_read_lock() + > device_is_registered() is paired with cdev_device_del() + > synchronize_rcu(). RCU needs barriers on the actual load/store just a naked device_is_registered() alone is not strong enough. > > IMHO this can't use 'dev->kobj.state_in_sysfs' as the RCU protected data. > > This usage of srcu is functionally equivalent to replacing > srcu_read_lock() with down_read() and the shutdown path with: Sort of, but the rules for load/store under RCU are different than for load/store under a normal barriered lock. All the data is unstable for instance and minimially needs READ_ONCE. > cdev_device_del(...); > down_write(...): > up_write(...); The lock would have to enclose the store to state_in_sysfs, otherwise as written it has the same data race problems. Jason
