[lkml]   [2016]   [Oct]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
SubjectRe: [PATCH v3 2/9] hwmon: (core) New hwmon registration API
Hi Jean,

On 10/07/2016 05:32 AM, Jean Delvare wrote:
[ ... ]
>>>> + name = (char *)template;
>>>> + } else {
>>>> + name = devm_kzalloc(dev, strlen(template) + 16, GFP_KERNEL);
>>>> + if (!name)
>>>> + return ERR_PTR(-ENOMEM);
>>>> + scnprintf(name, strlen(template) + 16, template,
>>>> + index + hwmon_attr_base(type));
>>>> + }
>>>> +
>>>> + hattr = devm_kzalloc(dev, sizeof(*hattr), GFP_KERNEL);
>>>> + if (!hattr)
>>>> + return ERR_PTR(-ENOMEM);
>>> So basically you are doing 1 or 2 memory allocations for every
>>> attribute? Looks quite bad from a memory fragmentation
>>> perspective :-( Not to mention performance. Given that you have all the
>>> data at hand before you start, can't you preallocate an array with the
>>> right number of hattr and pick from it? That would at least solve half
>>> of the problem.
> FTR I took a quick look at the iio code and there seems to be something
> like the idea above implemented in iio_device_register_sysfs(). But
> attributes themselves as instantiated by iio_device_register_sysfs()
> are still allocated individually. But hey I'm not familiar with the iio
> code anyway, I'm sure you know it better than I do.
>>> Something similar for string allocation may work too, although it's
>>> somewhat more complex due to the variable length. But I think the
>>> abituguru3 driver is doing it right, so maybe you can too.
>> I'll look into it.

Merging name allocation and hattr allocation turned out to be easy by adding
the name to struct hwmon_device_attribute. However, allocating an array of
struct hwmon_device_attribute for all attributes in one go is more difficult.
Main issue is that we _don't_ really know how many attributes are going to be
created; we only know the ceiling, which is calculated in __hwmon_create_attrs().
That number is currently used to allocate the array of attributes. The extra
pointers did not seem that important there (to me). However, allocating an
array of struct hwmon_device_attribute would result in memory being allocated
for non-existing attributes as well (ie those for which is_visible returns 0),
and that would at least potentially be much more. We can either accept that,
or I would have to implement a second pass over the attributes to determine
how many are actually implemented, or we could leave the per-attribute
allocation unchanged.

Thoughts ?


 \ /
  Last update: 2016-10-16 20:20    [W:0.103 / U:0.800 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site