Messages in this thread | | | Date | Tue, 30 Mar 2021 14:54:31 -0300 | From | Jason Gunthorpe <> | Subject | Re: [PATCH v2 2/4] cxl/mem: Fix synchronization mechanism for device removal vs ioctl operations |
| |
On Tue, Mar 30, 2021 at 10:31:15AM -0700, Dan Williams wrote: > On Tue, Mar 30, 2021 at 10:03 AM Jason Gunthorpe <jgg@nvidia.com> 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
| |