Re: [PATCH v3] /dev/mem: Revoke mappings when a driver claims the region
On Thu, May 21, 2020 at 4:41 AM Matthew Wilcox wrote: > > On Wed, May 20, 2020 at 09:39:49PM -0700, Dan Williams wrote: > > On Wed, May 20, 2020 at 9:37 PM Dan Williams > > wrote: > > > On Wed, May 20, 2020 at 7:26 PM Matthew Wilcox > > > wrote: > > > > On Wed, May 20, 2020 at 06:35:25PM -0700, Dan Williams wrote: > > > > > +static struct inode *devmem_inode; > > > > > + > > > > > +#ifdef CONFIG_IO_STRICT_DEVMEM > > > > > +void revoke_devmem(struct resource *res) > > > > > +{ > > > > > + struct inode *inode = READ_ONCE(devmem_inode); > > > > > + > > > > > + /* > > > > > + * Check that the initialization has completed. Losing the race > > > > > + * is ok because it means drivers are claiming resources before > > > > > + * the fs_initcall level of init and prevent /dev/mem from > > > > > + * establishing mappings. > > > > > + */ > > > > > + smp_rmb(); > > > > > + if (!inode) > > > > > + return; > > > > > > > > But we don't need the smp_rmb() here, right? READ_ONCE and WRITE_ONCE > > > > are a DATA DEPENDENCY barrier (in Documentation/memory-barriers.txt > > > > parlance) > > > > so the smp_rmb() is superfluous ... > > > > > > Is it? I did not grok that from Documentation/memory-barriers.txt. > > > READ_ONCE and WRITE_ONCE are certainly ordered with respect to each > > > other in the same function, but I thought they still depend on > > > barriers for smp ordering? > > > > > > > > + > > > > > + /* publish /dev/mem initialized */ > > > > > + smp_wmb(); > > > > > + WRITE_ONCE(devmem_inode, inode); > > > > > > > > As above, unnecessary barrier, I think. > > > > > > Well, if you're not sure, how sure should I be? > > > > I'm pretty sure they are needed, because I need the prior writes to > > initialize the inode to be fenced before the final write to publish > > the inode. I don't think WRITE_ONCE() enforces that prior writes have > > completed. > > Completed, no, but I think it does enforce that they're visible to other > CPUs before this write is visible to other CPUs. > > I'll quote relevant bits from the document ... > > (2) Data dependency barriers. > > A data dependency barrier is a weaker form of read barrier. In the case > where two loads are performed such that the second depends on the result > of the first (eg: the first load retrieves the address to which the > second > load will be directed), a data dependency barrier would be required to > make sure that the target of the second load is updated after the address > obtained by the first load is accessed. > > [...] > SMP BARRIER PAIRING > --- > [...] > CPU 1 CPU 2 > === === > a = 1; > > WRITE_ONCE(b, );x = READ_ONCE(b); > > y = *x; > Oh, I read those <* barrier> lines as a requirement not an implied side effect of READ/WRITE_ONCE(). I can see that WRITE_ONCE() is effectively a barrier() and READ_ONCE() includes smp_read_barrier_depends(). I'll drop. > > > > > > > > > > + /* > > > > > + * Use a unified address space to have a single point to manage > > > > > + * revocations when drivers want to take over a /dev/mem mapped > > > > > + * range. > > > > > + */ > > > > > + inode->i_mapping = devmem_inode->i_mapping; > > > > > + inode->i_mapping->host = devmem_inode; > > > > > > > > umm ... devmem_inode->i_mapping->host doesn't already point to > > > > devmem_inode? > > > > > > Not if inode is coming from: > > > > > > mknod ./newmem c 1 1 > > > > > > ...that's the problem that a unified inode solves. You can mknod all > > > you want, but mapping and mapping->host will point to a common > > > instance. > > I don't think I explained myself well enough. > > When we initialise devmem_inode, does devmem_inode->i_mapping->host point > to somewhere other than devmem_inode? > > I appreciate in this function, inode->i_mapping->host will point to inode. > But we're now changing i_mapping to be devmem_inode's i_mapping. Why > do we need to change devmem_inode's i_mapping->host pointer? > Yeah, mistook your comment. The setting of ->host is indeed redundant.
Re: [PATCH v3] /dev/mem: Revoke mappings when a driver claims the region
On Wed, May 20, 2020 at 09:39:49PM -0700, Dan Williams wrote: > On Wed, May 20, 2020 at 9:37 PM Dan Williams wrote: > > On Wed, May 20, 2020 at 7:26 PM Matthew Wilcox wrote: > > > On Wed, May 20, 2020 at 06:35:25PM -0700, Dan Williams wrote: > > > > +static struct inode *devmem_inode; > > > > + > > > > +#ifdef CONFIG_IO_STRICT_DEVMEM > > > > +void revoke_devmem(struct resource *res) > > > > +{ > > > > + struct inode *inode = READ_ONCE(devmem_inode); > > > > + > > > > + /* > > > > + * Check that the initialization has completed. Losing the race > > > > + * is ok because it means drivers are claiming resources before > > > > + * the fs_initcall level of init and prevent /dev/mem from > > > > + * establishing mappings. > > > > + */ > > > > + smp_rmb(); > > > > + if (!inode) > > > > + return; > > > > > > But we don't need the smp_rmb() here, right? READ_ONCE and WRITE_ONCE > > > are a DATA DEPENDENCY barrier (in Documentation/memory-barriers.txt > > > parlance) > > > so the smp_rmb() is superfluous ... > > > > Is it? I did not grok that from Documentation/memory-barriers.txt. > > READ_ONCE and WRITE_ONCE are certainly ordered with respect to each > > other in the same function, but I thought they still depend on > > barriers for smp ordering? > > > > > > + > > > > + /* publish /dev/mem initialized */ > > > > + smp_wmb(); > > > > + WRITE_ONCE(devmem_inode, inode); > > > > > > As above, unnecessary barrier, I think. > > > > Well, if you're not sure, how sure should I be? > > I'm pretty sure they are needed, because I need the prior writes to > initialize the inode to be fenced before the final write to publish > the inode. I don't think WRITE_ONCE() enforces that prior writes have > completed. Completed, no, but I think it does enforce that they're visible to other CPUs before this write is visible to other CPUs. I'll quote relevant bits from the document ... (2) Data dependency barriers. A data dependency barrier is a weaker form of read barrier. In the case where two loads are performed such that the second depends on the result of the first (eg: the first load retrieves the address to which the second load will be directed), a data dependency barrier would be required to make sure that the target of the second load is updated after the address obtained by the first load is accessed. [...] SMP BARRIER PAIRING --- [...] CPU 1 CPU 2 === === a = 1; WRITE_ONCE(b, );x = READ_ONCE(b); y = *x; > > > > > > > + /* > > > > + * Use a unified address space to have a single point to manage > > > > + * revocations when drivers want to take over a /dev/mem mapped > > > > + * range. > > > > + */ > > > > + inode->i_mapping = devmem_inode->i_mapping; > > > > + inode->i_mapping->host = devmem_inode; > > > > > > umm ... devmem_inode->i_mapping->host doesn't already point to > > > devmem_inode? > > > > Not if inode is coming from: > > > > mknod ./newmem c 1 1 > > > > ...that's the problem that a unified inode solves. You can mknod all > > you want, but mapping and mapping->host will point to a common > > instance. I don't think I explained myself well enough. When we initialise devmem_inode, does devmem_inode->i_mapping->host point to somewhere other than devmem_inode? I appreciate in this function, inode->i_mapping->host will point to inode. But we're now changing i_mapping to be devmem_inode's i_mapping. Why do we need to change devmem_inode's i_mapping->host pointer?
Re: [PATCH v3] /dev/mem: Revoke mappings when a driver claims the region
On Wed, May 20, 2020 at 9:37 PM Dan Williams wrote: > > On Wed, May 20, 2020 at 7:26 PM Matthew Wilcox wrote: > > > > On Wed, May 20, 2020 at 06:35:25PM -0700, Dan Williams wrote: > > > +static struct inode *devmem_inode; > > > + > > > +#ifdef CONFIG_IO_STRICT_DEVMEM > > > +void revoke_devmem(struct resource *res) > > > +{ > > > + struct inode *inode = READ_ONCE(devmem_inode); > > > + > > > + /* > > > + * Check that the initialization has completed. Losing the race > > > + * is ok because it means drivers are claiming resources before > > > + * the fs_initcall level of init and prevent /dev/mem from > > > + * establishing mappings. > > > + */ > > > + smp_rmb(); > > > + if (!inode) > > > + return; > > > > But we don't need the smp_rmb() here, right? READ_ONCE and WRITE_ONCE > > are a DATA DEPENDENCY barrier (in Documentation/memory-barriers.txt > > parlance) > > so the smp_rmb() is superfluous ... > > Is it? I did not grok that from Documentation/memory-barriers.txt. > READ_ONCE and WRITE_ONCE are certainly ordered with respect to each > other in the same function, but I thought they still depend on > barriers for smp ordering? > > > > > > + /* > > > + * Use a unified address space to have a single point to manage > > > + * revocations when drivers want to take over a /dev/mem mapped > > > + * range. > > > + */ > > > + inode->i_mapping = devmem_inode->i_mapping; > > > + inode->i_mapping->host = devmem_inode; > > > > umm ... devmem_inode->i_mapping->host doesn't already point to devmem_inode? > > Not if inode is coming from: > > mknod ./newmem c 1 1 > > ...that's the problem that a unified inode solves. You can mknod all > you want, but mapping and mapping->host will point to a common > instance. > > > > > > + > > > + /* publish /dev/mem initialized */ > > > + smp_wmb(); > > > + WRITE_ONCE(devmem_inode, inode); > > > > As above, unnecessary barrier, I think. > > Well, if you're not sure, how sure should I be? I'm pretty sure they are needed, because I need the prior writes to initialize the inode to be fenced before the final write to publish the inode. I don't think WRITE_ONCE() enforces that prior writes have completed.
Re: [PATCH v3] /dev/mem: Revoke mappings when a driver claims the region
On Wed, May 20, 2020 at 7:26 PM Matthew Wilcox wrote: > > On Wed, May 20, 2020 at 06:35:25PM -0700, Dan Williams wrote: > > +static struct inode *devmem_inode; > > + > > +#ifdef CONFIG_IO_STRICT_DEVMEM > > +void revoke_devmem(struct resource *res) > > +{ > > + struct inode *inode = READ_ONCE(devmem_inode); > > + > > + /* > > + * Check that the initialization has completed. Losing the race > > + * is ok because it means drivers are claiming resources before > > + * the fs_initcall level of init and prevent /dev/mem from > > + * establishing mappings. > > + */ > > + smp_rmb(); > > + if (!inode) > > + return; > > But we don't need the smp_rmb() here, right? READ_ONCE and WRITE_ONCE > are a DATA DEPENDENCY barrier (in Documentation/memory-barriers.txt parlance) > so the smp_rmb() is superfluous ... Is it? I did not grok that from Documentation/memory-barriers.txt. READ_ONCE and WRITE_ONCE are certainly ordered with respect to each other in the same function, but I thought they still depend on barriers for smp ordering? > > > + /* > > + * Use a unified address space to have a single point to manage > > + * revocations when drivers want to take over a /dev/mem mapped > > + * range. > > + */ > > + inode->i_mapping = devmem_inode->i_mapping; > > + inode->i_mapping->host = devmem_inode; > > umm ... devmem_inode->i_mapping->host doesn't already point to devmem_inode? Not if inode is coming from: mknod ./newmem c 1 1 ...that's the problem that a unified inode solves. You can mknod all you want, but mapping and mapping->host will point to a common instance. > > > + > > + /* publish /dev/mem initialized */ > > + smp_wmb(); > > + WRITE_ONCE(devmem_inode, inode); > > As above, unnecessary barrier, I think. Well, if you're not sure, how sure should I be?
Re: [PATCH v3] /dev/mem: Revoke mappings when a driver claims the region
On Wed, May 20, 2020 at 06:35:25PM -0700, Dan Williams wrote: > +static struct inode *devmem_inode; > + > +#ifdef CONFIG_IO_STRICT_DEVMEM > +void revoke_devmem(struct resource *res) > +{ > + struct inode *inode = READ_ONCE(devmem_inode); > + > + /* > + * Check that the initialization has completed. Losing the race > + * is ok because it means drivers are claiming resources before > + * the fs_initcall level of init and prevent /dev/mem from > + * establishing mappings. > + */ > + smp_rmb(); > + if (!inode) > + return; But we don't need the smp_rmb() here, right? READ_ONCE and WRITE_ONCE are a DATA DEPENDENCY barrier (in Documentation/memory-barriers.txt parlance) so the smp_rmb() is superfluous ... > + /* > + * Use a unified address space to have a single point to manage > + * revocations when drivers want to take over a /dev/mem mapped > + * range. > + */ > + inode->i_mapping = devmem_inode->i_mapping; > + inode->i_mapping->host = devmem_inode; umm ... devmem_inode->i_mapping->host doesn't already point to devmem_inode? > + > + /* publish /dev/mem initialized */ > + smp_wmb(); > + WRITE_ONCE(devmem_inode, inode); As above, unnecessary barrier, I think.