On Thu, 14 Nov 2024 20:25:59 +0800, Zijun Hu <zijun_hu@xxxxxxxxxx> wrote:
> On 2024/11/14 19:29, Matteo Martelli wrote:
> >>>> The address of a chunk allocated with `kmalloc` is aligned to at least
> >>>> ARCH_KMALLOC_MINALIGN bytes. For sizes of power of two bytes, the
> >>>> alignment is also guaranteed to be at least to the respective size.
> >>>>
> >>>> To do so I was thinking to try to move the devres metadata after the
> >>>> data buffer, so that the latter would directly correspond to pointer
> >>>> returned by kmalloc. I then found out that it had been already suggested
> >>>> previously to address a memory optimization [2]. Thus I am reporting the
> >>>> issue before submitting any patch as some discussions might be helpful
> >>>> first.
> >>>>
> >> no, IMO, that is not good idea absolutely.
> > Itâ??s now quite clear to me that the issue is a rare corner case, and the
> > potential impact of making such a change does not justify it. However,
> > for completeness and future reference, are there any additional reasons
> > why this change is a bad idea?
>
> 1)
> as i ever commented, below existing APIs is very suitable for your
> requirements. right ?
> addr = devm_get_free_pages(dev, GFP_KERNEL|__GFP_ZERO, 0);
> devm_free_pages(dev,addr);
Yes, but I was concerned by the possibility that other users assumed by
mistake that devm_kmalloc() would have provided the same alignment
guarantees as kmalloc(), so at that point a more generic approach could
have been worth a consideration. Given that today the issue seems to be
confined in only one IIO driver it's clearly a corner case and it is
just a matter of fixing that driver by using kmalloc()+devred_add(), or
devm_get_free_pages() as you suggested, instead of using devm_kmalloc().
>
> 2)
> touching existing API which have been used frequently means high risk?
Indeed. Same answer for 1) applies here.
>
> 3) if you put the important metadata at the end of the memory block.
> 3.1) it is easy to be destroyed by out of memory access.
This is a good point.
> 3.2) the API will be used to allocate memory with various sizes
> how to seek the tail metadata ? is it easy to seek it?
Apparently yes, but likely very hacky by using ksize(). See
data2devres() in [2] for an example.
> 3.3) if you allocate one page, the size to allocate is page size
> + meta size, it will waste memory align.
I think this is already the case with the current devm_kmalloc().
> 4) below simple alternative is better than your idea. it keep all
> attributes of original kmalloc(). right ?
>
> static int devres_raw_kmalloc_match(struct device *dev, void *res, void *p)
> {
> void **ptr = res;
> return *ptr == p;
> }
>
> static void devres_raw_kmalloc_release(struct device *dev, void *res)
> {
> void **ptr = res;
> kfree(*ptr);
> }
>
> void *devm_raw_kmalloc(struct device *dev, size_t size, gfp_t gfp)
> {
> void **ptr;
>
> ptr = devres_alloc(devres_raw_kmalloc_release, sizeof(*ptr), GFP_KERNEL);
> f (!ptr)
> return NULL;
>
> *ptr = kmalloc(size, gfp);
> if (!*ptr) {
> devres_free(ptr);
> return NULL;
> }
> devres_add(dev, ptr);
> return *ptr;
> }
> EXPORT(...)
>
> void *devm_raw_kfree(struct device *dev, void *p)
> {
> devres_release(dev, devres_raw_kmalloc_release,
> devres_raw_kmalloc_match, p);
> }
> EXPORT(...)
I also considered an alternative to decouple the two allocations of the
devres metadata and the actual buffer as you suggested here. However, I
would have preferred avoiding an additional API and applying this
approach directly within the original devres_kmalloc() if it turned out
to be necessary. At that point, though, I am not sure which of the two
approaches would have had less impact.
Thanks for sharing this, it could be useful if a similar discussion
arises in future.
>>>> [2]: https://lore.kernel.org/all/20191220140655.GN2827@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx/
Best regards,
Matteo Martelli
