Re: [PATCH 1/8] dax: add region-available-size attribute

[Dan Williams]

On Wed, Dec 14, 2016 at 7:53 AM, Dan Williams  wrote:
> On Wed, Dec 14, 2016 at 6:38 AM, Johannes Thumshirn  
> wrote:
>> Hi Dan,
>>
>> On Sat, Dec 10, 2016 at 10:28:30PM -0800, Dan Williams wrote:
>>> In preparation for a facility that enables dax regions to be
>>> sub-divided, introduce a 'dax/available_size' attribute.  This attribute
>>> appears under the parent device that registered the device-dax region,
>>> and it assumes that the device-dax-core owns the driver-data for that
>>> device.
>>>
>>> 'dax/available_size' adjusts dynamically as dax-device instances are
>>> registered and unregistered.
>>>
>>> As a side effect of using __request_region() to reserve capacity from
>>> the dax_region we now track pointers to those returned resources rather
>>> than duplicating the passed in resource array.
>>>
>>> Signed-off-by: Dan Williams 
>>> ---
>>
>> [...]
>>
>>> +static const struct attribute_group *dax_region_attribute_groups[] = {
>>> + _region_attribute_group,
>>> + NULL,
>>>  };
>>>
>>>  static struct inode *dax_alloc_inode(struct super_block *sb)
>>> @@ -200,12 +251,27 @@ void dax_region_put(struct dax_region *dax_region)
>>>  }
>>>  EXPORT_SYMBOL_GPL(dax_region_put);
>>>
>>> +
>>
>> Stray extra newline?
>>
>> [...]
>>
>>>  struct dax_region *alloc_dax_region(struct device *parent, int region_id,
>>>   struct resource *res, unsigned int align, void *addr,
>>>   unsigned long pfn_flags)
>>>  {
>>>   struct dax_region *dax_region;
>>>
>>> + if (dev_get_drvdata(parent)) {
>>> + dev_WARN(parent, "dax core found drvdata already in use\n");
>>> + return NULL;
>>> + }
>>> +
>>
>> My first thought was, it might be interesting to see who already claimed
>> the drvdata. Then I figured, how are multiple sub-regions of a dax-device
>> supposed to work? What am I missing here?
>
> This is a check similar to the -EBUSY return you would get from
> request_mem_region(). In fact if all dax drivers are correctly calling
> request_mem_region() before alloc_dax_region() then it would be
> impossible for this check to ever fire. It's already impossible
> because there's only one dax driver upstream (dax_pmem). It's not
> really benefiting the kernel at all until we have multiple dax
> drivers, I'll remove it.

No, I went to go delete this and remembered the real reason this was
added. A device driver that calls alloc_dax_region() commits to
letting the dax core own dev->driver_data. Since this wasn't even
clear to me, I'll go fix up the comment.

Re: [PATCH 1/8] dax: add region-available-size attribute

[Dan Williams]

On Wed, Dec 14, 2016 at 7:53 AM, Dan Williams  wrote:
> On Wed, Dec 14, 2016 at 6:38 AM, Johannes Thumshirn  
> wrote:
>> Hi Dan,
>>
>> On Sat, Dec 10, 2016 at 10:28:30PM -0800, Dan Williams wrote:
>>> In preparation for a facility that enables dax regions to be
>>> sub-divided, introduce a 'dax/available_size' attribute.  This attribute
>>> appears under the parent device that registered the device-dax region,
>>> and it assumes that the device-dax-core owns the driver-data for that
>>> device.
>>>
>>> 'dax/available_size' adjusts dynamically as dax-device instances are
>>> registered and unregistered.
>>>
>>> As a side effect of using __request_region() to reserve capacity from
>>> the dax_region we now track pointers to those returned resources rather
>>> than duplicating the passed in resource array.
>>>
>>> Signed-off-by: Dan Williams 
>>> ---
>>
>> [...]
>>
>>> +static const struct attribute_group *dax_region_attribute_groups[] = {
>>> + _region_attribute_group,
>>> + NULL,
>>>  };
>>>
>>>  static struct inode *dax_alloc_inode(struct super_block *sb)
>>> @@ -200,12 +251,27 @@ void dax_region_put(struct dax_region *dax_region)
>>>  }
>>>  EXPORT_SYMBOL_GPL(dax_region_put);
>>>
>>> +
>>
>> Stray extra newline?
>>
>> [...]
>>
>>>  struct dax_region *alloc_dax_region(struct device *parent, int region_id,
>>>   struct resource *res, unsigned int align, void *addr,
>>>   unsigned long pfn_flags)
>>>  {
>>>   struct dax_region *dax_region;
>>>
>>> + if (dev_get_drvdata(parent)) {
>>> + dev_WARN(parent, "dax core found drvdata already in use\n");
>>> + return NULL;
>>> + }
>>> +
>>
>> My first thought was, it might be interesting to see who already claimed
>> the drvdata. Then I figured, how are multiple sub-regions of a dax-device
>> supposed to work? What am I missing here?
>
> This is a check similar to the -EBUSY return you would get from
> request_mem_region(). In fact if all dax drivers are correctly calling
> request_mem_region() before alloc_dax_region() then it would be
> impossible for this check to ever fire. It's already impossible
> because there's only one dax driver upstream (dax_pmem). It's not
> really benefiting the kernel at all until we have multiple dax
> drivers, I'll remove it.

No, I went to go delete this and remembered the real reason this was
added. A device driver that calls alloc_dax_region() commits to
letting the dax core own dev->driver_data. Since this wasn't even
clear to me, I'll go fix up the comment.

Re: [PATCH 1/8] dax: add region-available-size attribute

[Dan Williams]

On Wed, Dec 14, 2016 at 6:38 AM, Johannes Thumshirn  wrote:
> Hi Dan,
>
> On Sat, Dec 10, 2016 at 10:28:30PM -0800, Dan Williams wrote:
>> In preparation for a facility that enables dax regions to be
>> sub-divided, introduce a 'dax/available_size' attribute.  This attribute
>> appears under the parent device that registered the device-dax region,
>> and it assumes that the device-dax-core owns the driver-data for that
>> device.
>>
>> 'dax/available_size' adjusts dynamically as dax-device instances are
>> registered and unregistered.
>>
>> As a side effect of using __request_region() to reserve capacity from
>> the dax_region we now track pointers to those returned resources rather
>> than duplicating the passed in resource array.
>>
>> Signed-off-by: Dan Williams 
>> ---
>
> [...]
>
>> +static const struct attribute_group *dax_region_attribute_groups[] = {
>> + _region_attribute_group,
>> + NULL,
>>  };
>>
>>  static struct inode *dax_alloc_inode(struct super_block *sb)
>> @@ -200,12 +251,27 @@ void dax_region_put(struct dax_region *dax_region)
>>  }
>>  EXPORT_SYMBOL_GPL(dax_region_put);
>>
>> +
>
> Stray extra newline?
>
> [...]
>
>>  struct dax_region *alloc_dax_region(struct device *parent, int region_id,
>>   struct resource *res, unsigned int align, void *addr,
>>   unsigned long pfn_flags)
>>  {
>>   struct dax_region *dax_region;
>>
>> + if (dev_get_drvdata(parent)) {
>> + dev_WARN(parent, "dax core found drvdata already in use\n");
>> + return NULL;
>> + }
>> +
>
> My first thought was, it might be interesting to see who already claimed
> the drvdata. Then I figured, how are multiple sub-regions of a dax-device
> supposed to work? What am I missing here?

This is a check similar to the -EBUSY return you would get from
request_mem_region(). In fact if all dax drivers are correctly calling
request_mem_region() before alloc_dax_region() then it would be
impossible for this check to ever fire. It's already impossible
because there's only one dax driver upstream (dax_pmem). It's not
really benefiting the kernel at all until we have multiple dax
drivers, I'll remove it.

Re: [PATCH 1/8] dax: add region-available-size attribute

[Dan Williams]

On Wed, Dec 14, 2016 at 6:38 AM, Johannes Thumshirn  wrote:
> Hi Dan,
>
> On Sat, Dec 10, 2016 at 10:28:30PM -0800, Dan Williams wrote:
>> In preparation for a facility that enables dax regions to be
>> sub-divided, introduce a 'dax/available_size' attribute.  This attribute
>> appears under the parent device that registered the device-dax region,
>> and it assumes that the device-dax-core owns the driver-data for that
>> device.
>>
>> 'dax/available_size' adjusts dynamically as dax-device instances are
>> registered and unregistered.
>>
>> As a side effect of using __request_region() to reserve capacity from
>> the dax_region we now track pointers to those returned resources rather
>> than duplicating the passed in resource array.
>>
>> Signed-off-by: Dan Williams 
>> ---
>
> [...]
>
>> +static const struct attribute_group *dax_region_attribute_groups[] = {
>> + _region_attribute_group,
>> + NULL,
>>  };
>>
>>  static struct inode *dax_alloc_inode(struct super_block *sb)
>> @@ -200,12 +251,27 @@ void dax_region_put(struct dax_region *dax_region)
>>  }
>>  EXPORT_SYMBOL_GPL(dax_region_put);
>>
>> +
>
> Stray extra newline?
>
> [...]
>
>>  struct dax_region *alloc_dax_region(struct device *parent, int region_id,
>>   struct resource *res, unsigned int align, void *addr,
>>   unsigned long pfn_flags)
>>  {
>>   struct dax_region *dax_region;
>>
>> + if (dev_get_drvdata(parent)) {
>> + dev_WARN(parent, "dax core found drvdata already in use\n");
>> + return NULL;
>> + }
>> +
>
> My first thought was, it might be interesting to see who already claimed
> the drvdata. Then I figured, how are multiple sub-regions of a dax-device
> supposed to work? What am I missing here?

This is a check similar to the -EBUSY return you would get from
request_mem_region(). In fact if all dax drivers are correctly calling
request_mem_region() before alloc_dax_region() then it would be
impossible for this check to ever fire. It's already impossible
because there's only one dax driver upstream (dax_pmem). It's not
really benefiting the kernel at all until we have multiple dax
drivers, I'll remove it.

Re: [PATCH 1/8] dax: add region-available-size attribute

[Johannes Thumshirn]

Hi Dan,

On Sat, Dec 10, 2016 at 10:28:30PM -0800, Dan Williams wrote:
> In preparation for a facility that enables dax regions to be
> sub-divided, introduce a 'dax/available_size' attribute.  This attribute
> appears under the parent device that registered the device-dax region,
> and it assumes that the device-dax-core owns the driver-data for that
> device.
> 
> 'dax/available_size' adjusts dynamically as dax-device instances are
> registered and unregistered.
> 
> As a side effect of using __request_region() to reserve capacity from
> the dax_region we now track pointers to those returned resources rather
> than duplicating the passed in resource array.
> 
> Signed-off-by: Dan Williams 
> ---

[...]

> +static const struct attribute_group *dax_region_attribute_groups[] = {
> + _region_attribute_group,
> + NULL,
>  };
>  
>  static struct inode *dax_alloc_inode(struct super_block *sb)
> @@ -200,12 +251,27 @@ void dax_region_put(struct dax_region *dax_region)
>  }
>  EXPORT_SYMBOL_GPL(dax_region_put);
>  
> +

Stray extra newline?

[...]

>  struct dax_region *alloc_dax_region(struct device *parent, int region_id,
>   struct resource *res, unsigned int align, void *addr,
>   unsigned long pfn_flags)
>  {
>   struct dax_region *dax_region;
>  
> + if (dev_get_drvdata(parent)) {
> + dev_WARN(parent, "dax core found drvdata already in use\n");
> + return NULL;
> + }
> +

My first thought was, it might be interesting to see who already claimed
the drvdata. Then I figured, how are multiple sub-regions of a dax-device
supposed to work? What am I missing here?

>   if (!IS_ALIGNED(res->start, align)
>   || !IS_ALIGNED(resource_size(res), align))
>   return NULL;
> @@ -213,16 +279,26 @@ struct dax_region *alloc_dax_region(struct device 
> *parent, int region_id,
>   dax_region = kzalloc(sizeof(*dax_region), GFP_KERNEL);
>   if (!dax_region)
>   return NULL;
> -
> - memcpy(_region->res, res, sizeof(*res));
> + dev_set_drvdata(parent, dax_region);
> + dax_region->res.name = dev_name(parent);
> + dax_region->res.start = res->start;
> + dax_region->res.end = res->end;
>   dax_region->pfn_flags = pfn_flags;
> + mutex_init(_region->lock);
>   kref_init(_region->kref);
>   dax_region->id = region_id;
>   ida_init(_region->ida);
>   dax_region->align = align;
>   dax_region->dev = parent;
>   dax_region->base = addr;
> + if (sysfs_create_groups(>kobj, dax_region_attribute_groups)) {
> + kfree(dax_region);
> + return NULL;;
> + }
>  
> + kref_get(_region->kref);
> + if (devm_add_action_or_reset(parent, dax_region_unregister, dax_region))
> + return NULL;
>   return dax_region;
>  }
>  EXPORT_SYMBOL_GPL(alloc_dax_region);

[...]

> @@ -568,28 +654,42 @@ struct dax_dev *devm_create_dax_dev(struct dax_region 
> *dax_region,
>   struct cdev *cdev;
>   dev_t dev_t;
>  
> - dax_dev = kzalloc(sizeof(*dax_dev) + sizeof(*res) * count, GFP_KERNEL);
> + dax_dev = kzalloc(sizeof(*dax_dev), GFP_KERNEL);
>   if (!dax_dev)
>   return ERR_PTR(-ENOMEM);
>  
> + dax_dev->res = kzalloc(sizeof(res) * count, GFP_KERNEL);

dax_dev->res = kcalloc(sizeof(res), count, GFP_KERNEL); ?

> + if (!dax_dev->res)
> + goto err_res;
> +

Byte,
Johannes

Re: [PATCH 1/8] dax: add region-available-size attribute

[Johannes Thumshirn]

Hi Dan,

On Sat, Dec 10, 2016 at 10:28:30PM -0800, Dan Williams wrote:
> In preparation for a facility that enables dax regions to be
> sub-divided, introduce a 'dax/available_size' attribute.  This attribute
> appears under the parent device that registered the device-dax region,
> and it assumes that the device-dax-core owns the driver-data for that
> device.
> 
> 'dax/available_size' adjusts dynamically as dax-device instances are
> registered and unregistered.
> 
> As a side effect of using __request_region() to reserve capacity from
> the dax_region we now track pointers to those returned resources rather
> than duplicating the passed in resource array.
> 
> Signed-off-by: Dan Williams 
> ---

[...]

> +static const struct attribute_group *dax_region_attribute_groups[] = {
> + _region_attribute_group,
> + NULL,
>  };
>  
>  static struct inode *dax_alloc_inode(struct super_block *sb)
> @@ -200,12 +251,27 @@ void dax_region_put(struct dax_region *dax_region)
>  }
>  EXPORT_SYMBOL_GPL(dax_region_put);
>  
> +

Stray extra newline?

[...]

>  struct dax_region *alloc_dax_region(struct device *parent, int region_id,
>   struct resource *res, unsigned int align, void *addr,
>   unsigned long pfn_flags)
>  {
>   struct dax_region *dax_region;
>  
> + if (dev_get_drvdata(parent)) {
> + dev_WARN(parent, "dax core found drvdata already in use\n");
> + return NULL;
> + }
> +

My first thought was, it might be interesting to see who already claimed
the drvdata. Then I figured, how are multiple sub-regions of a dax-device
supposed to work? What am I missing here?

>   if (!IS_ALIGNED(res->start, align)
>   || !IS_ALIGNED(resource_size(res), align))
>   return NULL;
> @@ -213,16 +279,26 @@ struct dax_region *alloc_dax_region(struct device 
> *parent, int region_id,
>   dax_region = kzalloc(sizeof(*dax_region), GFP_KERNEL);
>   if (!dax_region)
>   return NULL;
> -
> - memcpy(_region->res, res, sizeof(*res));
> + dev_set_drvdata(parent, dax_region);
> + dax_region->res.name = dev_name(parent);
> + dax_region->res.start = res->start;
> + dax_region->res.end = res->end;
>   dax_region->pfn_flags = pfn_flags;
> + mutex_init(_region->lock);
>   kref_init(_region->kref);
>   dax_region->id = region_id;
>   ida_init(_region->ida);
>   dax_region->align = align;
>   dax_region->dev = parent;
>   dax_region->base = addr;
> + if (sysfs_create_groups(>kobj, dax_region_attribute_groups)) {
> + kfree(dax_region);
> + return NULL;;
> + }
>  
> + kref_get(_region->kref);
> + if (devm_add_action_or_reset(parent, dax_region_unregister, dax_region))
> + return NULL;
>   return dax_region;
>  }
>  EXPORT_SYMBOL_GPL(alloc_dax_region);

[...]

> @@ -568,28 +654,42 @@ struct dax_dev *devm_create_dax_dev(struct dax_region 
> *dax_region,
>   struct cdev *cdev;
>   dev_t dev_t;
>  
> - dax_dev = kzalloc(sizeof(*dax_dev) + sizeof(*res) * count, GFP_KERNEL);
> + dax_dev = kzalloc(sizeof(*dax_dev), GFP_KERNEL);
>   if (!dax_dev)
>   return ERR_PTR(-ENOMEM);
>  
> + dax_dev->res = kzalloc(sizeof(res) * count, GFP_KERNEL);

dax_dev->res = kcalloc(sizeof(res), count, GFP_KERNEL); ?

> + if (!dax_dev->res)
> + goto err_res;
> +

Byte,
Johannes