Re: [PATCH] dma-buf: Remove unnecessary kmalloc() cast

Re: [PATCH] dma-buf: Remove unnecessary kmalloc() cast

[Christoph Hellwig]

On Mon, Jul 01, 2024 at 11:26:34PM -0700, Andrew Morton wrote:
> No, I do think the cast is useful:
> 
> 	struct page *page = dma_fence_chain_alloc();
> 
> will presently generate a warning.  We want this.  Your change will
> remove that useful warning.
> 
> 
> Unrelatedly: there is no earthly reason why this is implemented as a
> macro.  A static inline function would be so much better.  Why do we
> keep doing this.

Agreed with all of the above.  Adding the dmabuf maintainers.

Re: [PATCH] dma-buf: Remove unnecessary kmalloc() cast

[Christian König]

Am 02.07.24 um 08:40 schrieb Christoph Hellwig:
> On Mon, Jul 01, 2024 at 11:26:34PM -0700, Andrew Morton wrote:
>> No, I do think the cast is useful:
>>
>> 	struct page *page = dma_fence_chain_alloc();
>>
>> will presently generate a warning.  We want this.  Your change will
>> remove that useful warning.
>>
>>
>> Unrelatedly: there is no earthly reason why this is implemented as a
>> macro.  A static inline function would be so much better.  Why do we
>> keep doing this.
> Agreed with all of the above.  Adding the dmabuf maintainers.

Thanks for adding me and I have to ask to be added on DMA-buf patches 
when initially sending them out.

First of all: Yes that cast is intentionally there and yes that is 
intentionally a define and not an inline function.

See this patch here which changed that:

commit 2c321f3f70bc284510598f712b702ce8d60c4d14
Author: Suren Baghdasaryan <surenb@google.com>
Date:   Sun Apr 14 19:07:31 2024 -0700

     mm: change inlined allocation helpers to account at the call site

     Main goal of memory allocation profiling patchset is to provide 
accounting
     that is cheap enough to run in production.  To achieve that we inject
     counters using codetags at the allocation call sites to account 
every time
     allocation is made.  This injection allows us to perform accounting
     efficiently because injected counters are immediately available as 
opposed
     to the alternative methods, such as using _RET_IP_, which would require
     counter lookup and appropriate locking that makes accounting much more
     expensive.  This method requires all allocation functions to inject
     separate counters at their call sites so that their callers can be
     individually accounted.  Counter injection is implemented by allocation
     hooks which should wrap all allocation functions.

     Inlined functions which perform allocations but do not use allocation
     hooks are directly charged for the allocations they perform.  In most
     cases these functions are just specialized allocation wrappers used 
from
     multiple places to allocate objects of a specific type.  It would 
be more
     useful to do the accounting at their call sites instead. Instrument 
these
     helpers to do accounting at the call site.  Simple inlined allocation
     wrappers are converted directly into macros.  More complex 
allocators or
     allocators with documentation are converted into _noprof versions and
     allocation hooks are added.  This allows memory allocation profiling
     mechanism to charge allocations to the callers of these functions.

Regards,
Christian.

Re: [PATCH] dma-buf: Remove unnecessary kmalloc() cast

[Andrew Morton]

On Tue, 2 Jul 2024 09:13:35 +0200 Christian König <christian.koenig@amd.com> wrote:

> yes that is 
> intentionally a define and not an inline function.
> 
> See this patch here which changed that:
> 
> commit 2c321f3f70bc284510598f712b702ce8d60c4d14
> Author: Suren Baghdasaryan <surenb@google.com>
> Date:   Sun Apr 14 19:07:31 2024 -0700
> 
>      mm: change inlined allocation helpers to account at the call site

Dang, yes, that was a regrettable change.  But hardly the end of the
world.  I do think each such alteration should have included a comment
to prevent people from going and cleaning them up.

Re: [PATCH] dma-buf: Remove unnecessary kmalloc() cast

[Christoph Hellwig]

On Tue, Jul 02, 2024 at 12:33:57AM -0700, Andrew Morton wrote:
> Dang, yes, that was a regrettable change.  But hardly the end of the
> world.  I do think each such alteration should have included a comment
> to prevent people from going and cleaning them up.

.. or we should have never merged that utter mess ..

Re: [PATCH] dma-buf: Remove unnecessary kmalloc() cast

[Suren Baghdasaryan]

On Tue, Jul 2, 2024 at 7:34 AM Andrew Morton <akpm@linux-foundation.org> wrote:
>
> On Tue, 2 Jul 2024 09:13:35 +0200 Christian König <christian.koenig@amd.com> wrote:
>
> > yes that is
> > intentionally a define and not an inline function.
> >
> > See this patch here which changed that:
> >
> > commit 2c321f3f70bc284510598f712b702ce8d60c4d14
> > Author: Suren Baghdasaryan <surenb@google.com>
> > Date:   Sun Apr 14 19:07:31 2024 -0700
> >
> >      mm: change inlined allocation helpers to account at the call site
>
> Dang, yes, that was a regrettable change.  But hardly the end of the
> world.  I do think each such alteration should have included a comment
> to prevent people from going and cleaning them up.

Sorry I missed this discussion. Yes, the definition was intentional
and I will add comments for all the cases which were changed this way.
Thanks,
Suren.

>
>

Re: [PATCH] dma-buf: Remove unnecessary kmalloc() cast

[Suren Baghdasaryan]

On Tue, Jul 2, 2024 at 8:15 AM Suren Baghdasaryan <surenb@google.com> wrote:
>
> On Tue, Jul 2, 2024 at 7:34 AM Andrew Morton <akpm@linux-foundation.org> wrote:
> >
> > On Tue, 2 Jul 2024 09:13:35 +0200 Christian König <christian.koenig@amd.com> wrote:
> >
> > > yes that is
> > > intentionally a define and not an inline function.
> > >
> > > See this patch here which changed that:
> > >
> > > commit 2c321f3f70bc284510598f712b702ce8d60c4d14
> > > Author: Suren Baghdasaryan <surenb@google.com>
> > > Date:   Sun Apr 14 19:07:31 2024 -0700
> > >
> > >      mm: change inlined allocation helpers to account at the call site
> >
> > Dang, yes, that was a regrettable change.  But hardly the end of the
> > world.  I do think each such alteration should have included a comment
> > to prevent people from going and cleaning them up.
>
> Sorry I missed this discussion. Yes, the definition was intentional
> and I will add comments for all the cases which were changed this way.

Posted https://lore.kernel.org/all/20240703174225.3891393-1-surenb@google.com/
adding clarifying comments.
Thanks,
Suren.

> Thanks,
> Suren.
>
> >
> >