| Message ID | 20231214-vv-dax_abi-v6-2-ad900d698438@intel.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | Add DAX ABI for memmap_on_memory | expand |
On Thu, Dec 14, 2023 at 10:25:27PM -0700, Vishal Verma wrote: > @@ -294,13 +294,10 @@ static ssize_t available_size_show(struct device *dev, > struct device_attribute *attr, char *buf) > { > struct dax_region *dax_region = dev_get_drvdata(dev); > - unsigned long long size; > > - device_lock(dev); > - size = dax_region_avail_size(dax_region); > - device_unlock(dev); > + guard(device)(dev); > > - return sprintf(buf, "%llu\n", size); > + return sprintf(buf, "%llu\n", dax_region_avail_size(dax_region)); > } Is this an appropriate use of guard()? sprintf is not the fastest of functions, so we will end up holding the device_lock for longer than we used to. > @@ -908,9 +890,8 @@ static ssize_t size_show(struct device *dev, > struct dev_dax *dev_dax = to_dev_dax(dev); > unsigned long long size; > > - device_lock(dev); > + guard(device)(dev); > size = dev_dax_size(dev_dax); > - device_unlock(dev); > > return sprintf(buf, "%llu\n", size); > } If it is appropriate, then you can do without the 'size' variable here. > @@ -1137,21 +1117,20 @@ static ssize_t mapping_store(struct device *dev, struct device_attribute *attr, > if (rc) > return rc; > > - rc = -ENXIO; > - device_lock(dax_region->dev); > - if (!dax_region->dev->driver) { > - device_unlock(dax_region->dev); > - return rc; > - } > - device_lock(dev); > + guard(device)(dax_region->dev); > + if (!dax_region->dev->driver) > + return -ENXIO; > > + guard(device)(dev); > to_alloc = range_len(&r); > - if (alloc_is_aligned(dev_dax, to_alloc)) > - rc = alloc_dev_dax_range(dev_dax, r.start, to_alloc); > - device_unlock(dev); > - device_unlock(dax_region->dev); > + if (!alloc_is_aligned(dev_dax, to_alloc)) > + return -ENXIO; > > - return rc == 0 ? len : rc; > + rc = alloc_dev_dax_range(dev_dax, r.start, to_alloc); > + if (rc) > + return rc; > + > + return len; > } Have I mentioned how much I hate the "rc" naming convention? It tells you nothing useful about the contents of the variable. If you called it 'err', I'd know it was an error, and then the end of this function would make sense. if (err) return err; return len;
On Fri, 2023-12-15 at 05:56 +0000, Matthew Wilcox wrote: > On Thu, Dec 14, 2023 at 10:25:27PM -0700, Vishal Verma wrote: > > @@ -294,13 +294,10 @@ static ssize_t available_size_show(struct device *dev, > > struct device_attribute *attr, char *buf) > > { > > struct dax_region *dax_region = dev_get_drvdata(dev); > > - unsigned long long size; > > > > - device_lock(dev); > > - size = dax_region_avail_size(dax_region); > > - device_unlock(dev); > > + guard(device)(dev); > > > > - return sprintf(buf, "%llu\n", size); > > + return sprintf(buf, "%llu\n", dax_region_avail_size(dax_region)); > > } > > Is this an appropriate use of guard()? sprintf is not the fastest of > functions, so we will end up holding the device_lock for longer than > we used to. Hi Matthew, Agreed that we end up holding the lock for a bit longer in many of these. I'm inclined to say this is okay, since these are all user configuration paths through sysfs, not affecting any sort of runtime performance. > > > @@ -908,9 +890,8 @@ static ssize_t size_show(struct device *dev, > > struct dev_dax *dev_dax = to_dev_dax(dev); > > unsigned long long size; > > > > - device_lock(dev); > > + guard(device)(dev); > > size = dev_dax_size(dev_dax); > > - device_unlock(dev); > > > > return sprintf(buf, "%llu\n", size); > > } > > If it is appropriate, then you can do without the 'size' variable here. Yep will remove. I suppose a lot of these can also switch to sysfs_emit as Greg pointed out in a previous posting. I can add that as a separate cleanup patch. > > > @@ -1137,21 +1117,20 @@ static ssize_t mapping_store(struct device *dev, struct device_attribute *attr, > > if (rc) > > return rc; > > > > - rc = -ENXIO; > > - device_lock(dax_region->dev); > > - if (!dax_region->dev->driver) { > > - device_unlock(dax_region->dev); > > - return rc; > > - } > > - device_lock(dev); > > + guard(device)(dax_region->dev); > > + if (!dax_region->dev->driver) > > + return -ENXIO; > > > > + guard(device)(dev); > > to_alloc = range_len(&r); > > - if (alloc_is_aligned(dev_dax, to_alloc)) > > - rc = alloc_dev_dax_range(dev_dax, r.start, to_alloc); > > - device_unlock(dev); > > - device_unlock(dax_region->dev); > > + if (!alloc_is_aligned(dev_dax, to_alloc)) > > + return -ENXIO; > > > > - return rc == 0 ? len : rc; > > + rc = alloc_dev_dax_range(dev_dax, r.start, to_alloc); > > + if (rc) > > + return rc; > > + > > + return len; > > } > > Have I mentioned how much I hate the "rc" naming convention? It tells > you nothing useful about the contents of the variable. If you called it > 'err', I'd know it was an error, and then the end of this function would > make sense. > > if (err) > return err; > return len; > I'm a little hesitant to change this because the 'rc' convention is used all over this file, and while I don't mind making this change for the bits I touch in this patch, it would just result in a mix of 'rc' and 'err' in this file.
On Thu, Dec 14, 2023 at 10:25:27PM -0700, Vishal Verma wrote: > Use the guard(device) macro to lock a 'struct device', and unlock it > automatically when going out of scope using Scope Based Resource > Management semantics. A lot of the sysfs attribute writes in > drivers/dax/bus.c benefit from a cleanup using these, so change these > where applicable. Wait, why are you needing to call device_lock() at all here? Why is dax special in needing this when no other subsystem requires it? > > Cc: Joao Martins <joao.m.martins@oracle.com> > Cc: Dan Williams <dan.j.williams@intel.com> > Signed-off-by: Vishal Verma <vishal.l.verma@intel.com> > --- > drivers/dax/bus.c | 143 ++++++++++++++++++++++-------------------------------- > 1 file changed, 59 insertions(+), 84 deletions(-) > > diff --git a/drivers/dax/bus.c b/drivers/dax/bus.c > index 1ff1ab5fa105..6226de131d17 100644 > --- a/drivers/dax/bus.c > +++ b/drivers/dax/bus.c > @@ -294,13 +294,10 @@ static ssize_t available_size_show(struct device *dev, > struct device_attribute *attr, char *buf) > { > struct dax_region *dax_region = dev_get_drvdata(dev); > - unsigned long long size; > > - device_lock(dev); > - size = dax_region_avail_size(dax_region); > - device_unlock(dev); > + guard(device)(dev); You have a valid device here, why are you locking it? How can it go away? And if it can, shouldn't you have a local lock for it, and not abuse the driver core lock? > > - return sprintf(buf, "%llu\n", size); > + return sprintf(buf, "%llu\n", dax_region_avail_size(dax_region)); sysfs_emit() everywhere please. But again, the issue is "why do you need a lock"? thanks, greg k-h
On Fri, Dec 15, 2023 at 06:33:58AM +0000, Verma, Vishal L wrote: > On Fri, 2023-12-15 at 05:56 +0000, Matthew Wilcox wrote: > > On Thu, Dec 14, 2023 at 10:25:27PM -0700, Vishal Verma wrote: > > > @@ -294,13 +294,10 @@ static ssize_t available_size_show(struct device *dev, > > > struct device_attribute *attr, char *buf) > > > { > > > struct dax_region *dax_region = dev_get_drvdata(dev); > > > - unsigned long long size; > > > > > > - device_lock(dev); > > > - size = dax_region_avail_size(dax_region); > > > - device_unlock(dev); > > > + guard(device)(dev); > > > > > > - return sprintf(buf, "%llu\n", size); > > > + return sprintf(buf, "%llu\n", dax_region_avail_size(dax_region)); > > > } > > > > Is this an appropriate use of guard()? sprintf is not the fastest of > > functions, so we will end up holding the device_lock for longer than > > we used to. > > Hi Matthew, > > Agreed that we end up holding the lock for a bit longer in many of > these. I'm inclined to say this is okay, since these are all user > configuration paths through sysfs, not affecting any sort of runtime > performance. Why does the lock have to be taken at all? You have a valid reference, isn't that all you need? thanks, greg k-h
Greg Kroah-Hartman wrote: > On Thu, Dec 14, 2023 at 10:25:27PM -0700, Vishal Verma wrote: > > Use the guard(device) macro to lock a 'struct device', and unlock it > > automatically when going out of scope using Scope Based Resource > > Management semantics. A lot of the sysfs attribute writes in > > drivers/dax/bus.c benefit from a cleanup using these, so change these > > where applicable. > > Wait, why are you needing to call device_lock() at all here? Why is dax > special in needing this when no other subsystem requires it? > > > > > Cc: Joao Martins <joao.m.martins@oracle.com> > > Cc: Dan Williams <dan.j.williams@intel.com> > > Signed-off-by: Vishal Verma <vishal.l.verma@intel.com> > > --- > > drivers/dax/bus.c | 143 ++++++++++++++++++++++-------------------------------- > > 1 file changed, 59 insertions(+), 84 deletions(-) > > > > diff --git a/drivers/dax/bus.c b/drivers/dax/bus.c > > index 1ff1ab5fa105..6226de131d17 100644 > > --- a/drivers/dax/bus.c > > +++ b/drivers/dax/bus.c > > @@ -294,13 +294,10 @@ static ssize_t available_size_show(struct device *dev, > > struct device_attribute *attr, char *buf) > > { > > struct dax_region *dax_region = dev_get_drvdata(dev); > > - unsigned long long size; > > > > - device_lock(dev); > > - size = dax_region_avail_size(dax_region); > > - device_unlock(dev); > > + guard(device)(dev); > > You have a valid device here, why are you locking it? How can it go > away? And if it can, shouldn't you have a local lock for it, and not > abuse the driver core lock? Yes, this is a driver-core lock abuse written by someone who should have known better. And yes, a local lock to protect the dax_region resource tree should replace this. A new rwsem to synchronize all list walks seems appropriate.
On Fri, 2023-12-15 at 09:15 -0800, Dan Williams wrote: > Greg Kroah-Hartman wrote: > > On Thu, Dec 14, 2023 at 10:25:27PM -0700, Vishal Verma wrote: > > > Use the guard(device) macro to lock a 'struct device', and unlock it > > > automatically when going out of scope using Scope Based Resource > > > Management semantics. A lot of the sysfs attribute writes in > > > drivers/dax/bus.c benefit from a cleanup using these, so change these > > > where applicable. > > > > Wait, why are you needing to call device_lock() at all here? Why is dax > > special in needing this when no other subsystem requires it? > > > > > > > > Cc: Joao Martins <joao.m.martins@oracle.com> > > > Cc: Dan Williams <dan.j.williams@intel.com> > > > Signed-off-by: Vishal Verma <vishal.l.verma@intel.com> > > > --- > > > drivers/dax/bus.c | 143 ++++++++++++++++++++++-------------------------------- > > > 1 file changed, 59 insertions(+), 84 deletions(-) > > > > > > diff --git a/drivers/dax/bus.c b/drivers/dax/bus.c > > > index 1ff1ab5fa105..6226de131d17 100644 > > > --- a/drivers/dax/bus.c > > > +++ b/drivers/dax/bus.c > > > @@ -294,13 +294,10 @@ static ssize_t available_size_show(struct device *dev, > > > struct device_attribute *attr, char *buf) > > > { > > > struct dax_region *dax_region = dev_get_drvdata(dev); > > > - unsigned long long size; > > > > > > - device_lock(dev); > > > - size = dax_region_avail_size(dax_region); > > > - device_unlock(dev); > > > + guard(device)(dev); > > > > You have a valid device here, why are you locking it? How can it go > > away? And if it can, shouldn't you have a local lock for it, and not > > abuse the driver core lock? > > Yes, this is a driver-core lock abuse written by someone who should have > known better. And yes, a local lock to protect the dax_region resource > tree should replace this. A new rwsem to synchronize all list walks > seems appropriate. I see why _a_ lock is needed both here and in size_show() - the size calculations do a walk over discontiguous ranges, and we don't want the device to get reconfigured in the middle of that. A different local lock seems reasonable - however can that go as a separate cleanup that stands on its own? For this series, I'll add a cleanup to replace the sprintfs with sysfs_emit().
On Fri, Dec 15, 2023 at 05:32:50PM +0000, Verma, Vishal L wrote: > On Fri, 2023-12-15 at 09:15 -0800, Dan Williams wrote: > > Greg Kroah-Hartman wrote: > > > On Thu, Dec 14, 2023 at 10:25:27PM -0700, Vishal Verma wrote: > > > > Use the guard(device) macro to lock a 'struct device', and unlock it > > > > automatically when going out of scope using Scope Based Resource > > > > Management semantics. A lot of the sysfs attribute writes in > > > > drivers/dax/bus.c benefit from a cleanup using these, so change these > > > > where applicable. > > > > > > Wait, why are you needing to call device_lock() at all here? Why is dax > > > special in needing this when no other subsystem requires it? > > > > > > > > > > > Cc: Joao Martins <joao.m.martins@oracle.com> > > > > Cc: Dan Williams <dan.j.williams@intel.com> > > > > Signed-off-by: Vishal Verma <vishal.l.verma@intel.com> > > > > --- > > > > drivers/dax/bus.c | 143 ++++++++++++++++++++++-------------------------------- > > > > 1 file changed, 59 insertions(+), 84 deletions(-) > > > > > > > > diff --git a/drivers/dax/bus.c b/drivers/dax/bus.c > > > > index 1ff1ab5fa105..6226de131d17 100644 > > > > --- a/drivers/dax/bus.c > > > > +++ b/drivers/dax/bus.c > > > > @@ -294,13 +294,10 @@ static ssize_t available_size_show(struct device *dev, > > > > struct device_attribute *attr, char *buf) > > > > { > > > > struct dax_region *dax_region = dev_get_drvdata(dev); > > > > - unsigned long long size; > > > > > > > > - device_lock(dev); > > > > - size = dax_region_avail_size(dax_region); > > > > - device_unlock(dev); > > > > + guard(device)(dev); > > > > > > You have a valid device here, why are you locking it? How can it go > > > away? And if it can, shouldn't you have a local lock for it, and not > > > abuse the driver core lock? > > > > Yes, this is a driver-core lock abuse written by someone who should have > > known better. And yes, a local lock to protect the dax_region resource > > tree should replace this. A new rwsem to synchronize all list walks > > seems appropriate. > > I see why _a_ lock is needed both here and in size_show() - the size > calculations do a walk over discontiguous ranges, and we don't want the > device to get reconfigured in the middle of that. A different local > lock seems reasonable - however can that go as a separate cleanup that > stands on its own? Sure, but do not add a conversion to use guard(device) here, as that will be pointless as you will just use a real lock instead. > For this series, I'll add a cleanup to replace the sprintfs with > sysfs_emit(). Why not have that be the first patch in the series? Then add your local lock and convert everything to use it instead of the device lock? thanks, greg k-h
On Thu, 14 Dec 2023 22:25:27 -0700 Vishal Verma <vishal.l.verma@intel.com> wrote: > Use the guard(device) macro to lock a 'struct device', and unlock it > automatically when going out of scope using Scope Based Resource > Management semantics. A lot of the sysfs attribute writes in > drivers/dax/bus.c benefit from a cleanup using these, so change these > where applicable. > > Cc: Joao Martins <joao.m.martins@oracle.com> > Cc: Dan Williams <dan.j.williams@intel.com> > Signed-off-by: Vishal Verma <vishal.l.verma@intel.com> Hi Vishal, A few really minor suggestions inline if you happen to be doing a v7. Either way Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > @@ -481,12 +466,9 @@ static int __free_dev_dax_id(struct dev_dax *dev_dax) > static int free_dev_dax_id(struct dev_dax *dev_dax) > { > struct device *dev = &dev_dax->dev; > - int rc; > > - device_lock(dev); > - rc = __free_dev_dax_id(dev_dax); > - device_unlock(dev); > - return rc; > + guard(device)(dev); guard(device)(&dev_dax->dev); /* Only one user now */ > + return __free_dev_dax_id(dev_dax); > } > > static int alloc_dev_dax_id(struct dev_dax *dev_dax) > @@ -908,9 +890,8 @@ static ssize_t size_show(struct device *dev, > struct dev_dax *dev_dax = to_dev_dax(dev); > unsigned long long size; > > - device_lock(dev); > + guard(device)(dev); > size = dev_dax_size(dev_dax); > - device_unlock(dev); > > return sprintf(buf, "%llu\n", size); Might as well make this guard(device)(dev); return sprintf(buf, "%llu\n", dev_dax_size(to_dev_dax(dev)); > }
diff --git a/drivers/dax/bus.c b/drivers/dax/bus.c index 1ff1ab5fa105..6226de131d17 100644 --- a/drivers/dax/bus.c +++ b/drivers/dax/bus.c @@ -294,13 +294,10 @@ static ssize_t available_size_show(struct device *dev, struct device_attribute *attr, char *buf) { struct dax_region *dax_region = dev_get_drvdata(dev); - unsigned long long size; - device_lock(dev); - size = dax_region_avail_size(dax_region); - device_unlock(dev); + guard(device)(dev); - return sprintf(buf, "%llu\n", size); + return sprintf(buf, "%llu\n", dax_region_avail_size(dax_region)); } static DEVICE_ATTR_RO(available_size); @@ -309,17 +306,14 @@ static ssize_t seed_show(struct device *dev, { struct dax_region *dax_region = dev_get_drvdata(dev); struct device *seed; - ssize_t rc; if (is_static(dax_region)) return -EINVAL; - device_lock(dev); + guard(device)(dev); seed = dax_region->seed; - rc = sprintf(buf, "%s\n", seed ? dev_name(seed) : ""); - device_unlock(dev); - return rc; + return sprintf(buf, "%s\n", seed ? dev_name(seed) : ""); } static DEVICE_ATTR_RO(seed); @@ -328,24 +322,28 @@ static ssize_t create_show(struct device *dev, { struct dax_region *dax_region = dev_get_drvdata(dev); struct device *youngest; - ssize_t rc; if (is_static(dax_region)) return -EINVAL; - device_lock(dev); + guard(device)(dev); youngest = dax_region->youngest; - rc = sprintf(buf, "%s\n", youngest ? dev_name(youngest) : ""); - device_unlock(dev); - return rc; + return sprintf(buf, "%s\n", youngest ? dev_name(youngest) : ""); } static ssize_t create_store(struct device *dev, struct device_attribute *attr, const char *buf, size_t len) { struct dax_region *dax_region = dev_get_drvdata(dev); + struct dev_dax_data data = { + .dax_region = dax_region, + .size = 0, + .id = -1, + .memmap_on_memory = false, + }; unsigned long long avail; + struct dev_dax *dev_dax; ssize_t rc; int val; @@ -358,38 +356,25 @@ static ssize_t create_store(struct device *dev, struct device_attribute *attr, if (val != 1) return -EINVAL; - device_lock(dev); + guard(device)(dev); avail = dax_region_avail_size(dax_region); if (avail == 0) - rc = -ENOSPC; - else { - struct dev_dax_data data = { - .dax_region = dax_region, - .size = 0, - .id = -1, - .memmap_on_memory = false, - }; - struct dev_dax *dev_dax = devm_create_dev_dax(&data); + return -ENOSPC; - if (IS_ERR(dev_dax)) - rc = PTR_ERR(dev_dax); - else { - /* - * In support of crafting multiple new devices - * simultaneously multiple seeds can be created, - * but only the first one that has not been - * successfully bound is tracked as the region - * seed. - */ - if (!dax_region->seed) - dax_region->seed = &dev_dax->dev; - dax_region->youngest = &dev_dax->dev; - rc = len; - } - } - device_unlock(dev); + dev_dax = devm_create_dev_dax(&data); + if (IS_ERR(dev_dax)) + return PTR_ERR(dev_dax); - return rc; + /* + * In support of crafting multiple new devices simultaneously multiple + * seeds can be created, but only the first one that has not been + * successfully bound is tracked as the region seed. + */ + if (!dax_region->seed) + dax_region->seed = &dev_dax->dev; + dax_region->youngest = &dev_dax->dev; + + return len; } static DEVICE_ATTR_RW(create); @@ -481,12 +466,9 @@ static int __free_dev_dax_id(struct dev_dax *dev_dax) static int free_dev_dax_id(struct dev_dax *dev_dax) { struct device *dev = &dev_dax->dev; - int rc; - device_lock(dev); - rc = __free_dev_dax_id(dev_dax); - device_unlock(dev); - return rc; + guard(device)(dev); + return __free_dev_dax_id(dev_dax); } static int alloc_dev_dax_id(struct dev_dax *dev_dax) @@ -908,9 +890,8 @@ static ssize_t size_show(struct device *dev, struct dev_dax *dev_dax = to_dev_dax(dev); unsigned long long size; - device_lock(dev); + guard(device)(dev); size = dev_dax_size(dev_dax); - device_unlock(dev); return sprintf(buf, "%llu\n", size); } @@ -1080,17 +1061,16 @@ static ssize_t size_store(struct device *dev, struct device_attribute *attr, return -EINVAL; } - device_lock(dax_region->dev); - if (!dax_region->dev->driver) { - device_unlock(dax_region->dev); + guard(device)(dax_region->dev); + if (!dax_region->dev->driver) return -ENXIO; - } - device_lock(dev); + + guard(device)(dev); rc = dev_dax_resize(dax_region, dev_dax, val); - device_unlock(dev); - device_unlock(dax_region->dev); + if (rc) + return rc; - return rc == 0 ? len : rc; + return len; } static DEVICE_ATTR_RW(size); @@ -1137,21 +1117,20 @@ static ssize_t mapping_store(struct device *dev, struct device_attribute *attr, if (rc) return rc; - rc = -ENXIO; - device_lock(dax_region->dev); - if (!dax_region->dev->driver) { - device_unlock(dax_region->dev); - return rc; - } - device_lock(dev); + guard(device)(dax_region->dev); + if (!dax_region->dev->driver) + return -ENXIO; + guard(device)(dev); to_alloc = range_len(&r); - if (alloc_is_aligned(dev_dax, to_alloc)) - rc = alloc_dev_dax_range(dev_dax, r.start, to_alloc); - device_unlock(dev); - device_unlock(dax_region->dev); + if (!alloc_is_aligned(dev_dax, to_alloc)) + return -ENXIO; - return rc == 0 ? len : rc; + rc = alloc_dev_dax_range(dev_dax, r.start, to_alloc); + if (rc) + return rc; + + return len; } static DEVICE_ATTR_WO(mapping); @@ -1196,27 +1175,23 @@ static ssize_t align_store(struct device *dev, struct device_attribute *attr, if (!dax_align_valid(val)) return -EINVAL; - device_lock(dax_region->dev); - if (!dax_region->dev->driver) { - device_unlock(dax_region->dev); + guard(device)(dax_region->dev); + if (!dax_region->dev->driver) return -ENXIO; - } - device_lock(dev); - if (dev->driver) { - rc = -EBUSY; - goto out_unlock; - } + guard(device)(dev); + if (dev->driver) + return -EBUSY; align_save = dev_dax->align; dev_dax->align = val; rc = dev_dax_validate_align(dev_dax); - if (rc) + if (rc) { dev_dax->align = align_save; -out_unlock: - device_unlock(dev); - device_unlock(dax_region->dev); - return rc == 0 ? len : rc; + return rc; + } + + return len; } static DEVICE_ATTR_RW(align);
Use the guard(device) macro to lock a 'struct device', and unlock it automatically when going out of scope using Scope Based Resource Management semantics. A lot of the sysfs attribute writes in drivers/dax/bus.c benefit from a cleanup using these, so change these where applicable. Cc: Joao Martins <joao.m.martins@oracle.com> Cc: Dan Williams <dan.j.williams@intel.com> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com> --- drivers/dax/bus.c | 143 ++++++++++++++++++++++-------------------------------- 1 file changed, 59 insertions(+), 84 deletions(-)