| Message ID | 148143773119.10950.13840765929901351011.stgit@dwillia2-desk3.amr.corp.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Dan, On Sat, Dec 10, 2016 at 10:28:51PM -0800, Dan Williams wrote: > In preparation for other callers of these routines make the locking the > responsibility of the caller. > > Signed-off-by: Dan Williams <dan.j.williams@intel.com> > --- > drivers/dax/dax.c | 30 ++++++++++++++++++++++++------ > 1 file changed, 24 insertions(+), 6 deletions(-) > > diff --git a/drivers/dax/dax.c b/drivers/dax/dax.c > index d878a56cf3e3..5b65eaff6ace 100644 > --- a/drivers/dax/dax.c > +++ b/drivers/dax/dax.c > @@ -90,11 +90,11 @@ static unsigned long long dax_region_avail_size( > unsigned long long size; > struct resource *res; > > - mutex_lock(&dax_region->lock); > + WARN_ON_ONCE(!mutex_is_locked(&dax_region->lock)); I'd prefer it a lockdep_assert_held(&dex_region->lock). This of cause has the drawback that it won't trigger w/o lockdep but enabled, but I don't think it's the responibility of a production kernel to have this warnings anyway. On the flip side you get all the lockdep beauty for free with it. > + > size = resource_size(&dax_region->res); > for_each_dax_region_resource(dax_region, res) > size -= resource_size(res); > - mutex_unlock(&dax_region->lock); > > return size; > } [...] > +static unsigned long long dax_dev_size(struct dax_dev *dax_dev) > { > - struct dax_dev *dax_dev = to_dax_dev(dev); > + struct dax_region *dax_region = dax_dev->region; > unsigned long long size = 0; > int i; > > + WARN_ON_ONCE(!mutex_is_locked(&dax_region->lock)); > + See above > for (i = 0; i < dax_dev->num_resources; i++) > size += resource_size(dax_dev->res[i]); > > + return size; > +} > + Byte, Johannes
On Wed, Dec 14, 2016 at 7:01 AM, Johannes Thumshirn <jthumshirn@suse.de> wrote: > Hi Dan, > > On Sat, Dec 10, 2016 at 10:28:51PM -0800, Dan Williams wrote: >> In preparation for other callers of these routines make the locking the >> responsibility of the caller. >> >> Signed-off-by: Dan Williams <dan.j.williams@intel.com> >> --- >> drivers/dax/dax.c | 30 ++++++++++++++++++++++++------ >> 1 file changed, 24 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/dax/dax.c b/drivers/dax/dax.c >> index d878a56cf3e3..5b65eaff6ace 100644 >> --- a/drivers/dax/dax.c >> +++ b/drivers/dax/dax.c >> @@ -90,11 +90,11 @@ static unsigned long long dax_region_avail_size( >> unsigned long long size; >> struct resource *res; >> >> - mutex_lock(&dax_region->lock); >> + WARN_ON_ONCE(!mutex_is_locked(&dax_region->lock)); > > I'd prefer it a lockdep_assert_held(&dex_region->lock). This of cause has the > drawback that it won't trigger w/o lockdep but enabled, but I don't think it's > the responibility of a production kernel to have this warnings anyway. On the > flip side you get all the lockdep beauty for free with it. True, yes, it's only a developer debug warning. I'll change it and fix up the other pattern like this in libnvdimm.
diff --git a/drivers/dax/dax.c b/drivers/dax/dax.c index d878a56cf3e3..5b65eaff6ace 100644 --- a/drivers/dax/dax.c +++ b/drivers/dax/dax.c @@ -90,11 +90,11 @@ static unsigned long long dax_region_avail_size( unsigned long long size; struct resource *res; - mutex_lock(&dax_region->lock); + WARN_ON_ONCE(!mutex_is_locked(&dax_region->lock)); + size = resource_size(&dax_region->res); for_each_dax_region_resource(dax_region, res) size -= resource_size(res); - mutex_unlock(&dax_region->lock); return size; } @@ -107,8 +107,11 @@ static ssize_t available_size_show(struct device *dev, device_lock(dev); dax_region = dev_get_drvdata(dev); - if (dax_region) + if (dax_region) { + mutex_lock(&dax_region->lock); rc = sprintf(buf, "%llu\n", dax_region_avail_size(dax_region)); + mutex_unlock(&dax_region->lock); + } device_unlock(dev); return rc; @@ -389,16 +392,31 @@ static struct dax_dev *to_dax_dev(struct device *dev) return container_of(dev, struct dax_dev, dev); } -static ssize_t size_show(struct device *dev, - struct device_attribute *attr, char *buf) +static unsigned long long dax_dev_size(struct dax_dev *dax_dev) { - struct dax_dev *dax_dev = to_dax_dev(dev); + struct dax_region *dax_region = dax_dev->region; unsigned long long size = 0; int i; + WARN_ON_ONCE(!mutex_is_locked(&dax_region->lock)); + for (i = 0; i < dax_dev->num_resources; i++) size += resource_size(dax_dev->res[i]); + return size; +} + +static ssize_t size_show(struct device *dev, + struct device_attribute *attr, char *buf) +{ + unsigned long long size; + struct dax_dev *dax_dev = to_dax_dev(dev); + struct dax_region *dax_region = dax_dev->region; + + mutex_lock(&dax_region->lock); + size = dax_dev_size(dax_dev); + mutex_unlock(&dax_region->lock); + return sprintf(buf, "%llu\n", size); } static DEVICE_ATTR_RO(size);
In preparation for other callers of these routines make the locking the responsibility of the caller. Signed-off-by: Dan Williams <dan.j.williams@intel.com> --- drivers/dax/dax.c | 30 ++++++++++++++++++++++++------ 1 file changed, 24 insertions(+), 6 deletions(-)