Re: [PATCH v3 2/9] hwmon: (core) New hwmon registration API

Guenter Roeck
2016-10-16T18:20:31Z

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 ?

Thanks,
Guenter