On Thu, Apr 4, 2024 at 4:01 PM Kent Overstreet
<[email protected]> wrote:
>
> On Thu, Apr 04, 2024 at 03:41:50PM -0700, Andrew Morton wrote:
> > On Thu, 4 Apr 2024 18:38:39 -0400 Kent Overstreet <[email protected]> wrote:
> >
> > > On Thu, Apr 04, 2024 at 11:33:22PM +0100, Matthew Wilcox wrote:
> > > > On Thu, Apr 04, 2024 at 03:17:43PM -0700, Suren Baghdasaryan wrote:
> > > > > Ironically, checkpatch generates warnings for these type casts:
> > > > >
> > > > > WARNING: unnecessary cast may hide bugs, see
> > > > > http://c-faq.com/malloc/mallocnocast.html
> > > > > #425: FILE: include/linux/dma-fence-chain.h:90:
> > > > > + ((struct dma_fence_chain *)kmalloc(sizeof(struct dma_fence_chain),
> > > > > GFP_KERNEL))
> > > > >
> > > > > I guess I can safely ignore them in this case (since we cast to the
> > > > > expected type)?
> > > >
> > > > I find ignoring checkpatch to be a solid move 99% of the time.
> > > >
> > > > I really don't like the codetags. This is so much churn, and it could
> > > > all be avoided by just passing in _RET_IP_ or _THIS_IP_ depending on
> > > > whether we wanted to profile this function or its caller. vmalloc
> > > > has done it this way since 2008 (OK, using __builtin_return_address())
> > > > and lockdep has used _THIS_IP_ / _RET_IP_ since 2006.
> > >
> > > Except you can't. We've been over this; using that approach for tracing
> > > is one thing, using it for actual accounting isn't workable.
> >
> > I missed that. There have been many emails. Please remind us of the
> > reasoning here.
>
> I think it's on the other people claiming 'oh this would be so easy if
> you just do it this other way' to put up some code - or at least more
> than hot takes.
>
> But, since you asked - one of the main goals of this patchset was to be
> fast enough to run in production, and if you do it by return address
> then you've added at minimum a hash table lookup to every allocate and
> free; if you do that, running it in production is completely out of the
> question.
>
> Besides that - the issues with annotating and tracking the correct
> callsite really don't go away, they just shift around a bit. It's true
> that the return address approach would be easier initially, but that's
> not all we're concerned with; we're concerned with making sure
> allocations get accounted to the _correct_ callsite so that we're giving
> numbers that you can trust, and by making things less explicit you make
> that harder.
>
> Additionally: the alloc_hooks() macro is for more than this. It's also
> for more usable fault injection - remember every thread we have where
> people are begging for every allocation to be __GFP_NOFAIL - "oh, error
> paths are hard to test, let's just get rid of them" - never mind that
> actually do have to have error paths - but _per callsite_ selectable
> fault injection will actually make it practical to test memory error
> paths.
>
> And Kees working on stuff that'll make use of the alloc_hooks() macro
> for segregating kmem_caches.
Yeah, that pretty much summarizes it. Note that we don't have to make
the conversions in this patch and accounting will still work but then
all allocations from different callers will be accounted to the helper
function and that's less useful than accounting at the call site.
It's a sizable churn but the conversions are straight-forward and we
do get accurate, performant and easy to use memory accounting.
>
> This is all stuff that I've explained before; let's please dial back on
> the whining - or I'll just bookmark this for next time...
On Thu 04-04-24 16:16:15, Suren Baghdasaryan wrote:
> On Thu, Apr 4, 2024 at 4:01 PM Kent Overstreet
> <[email protected]> wrote:
> >
> > On Thu, Apr 04, 2024 at 03:41:50PM -0700, Andrew Morton wrote:
> > > On Thu, 4 Apr 2024 18:38:39 -0400 Kent Overstreet <[email protected]> wrote:
> > >
> > > > On Thu, Apr 04, 2024 at 11:33:22PM +0100, Matthew Wilcox wrote:
> > > > > On Thu, Apr 04, 2024 at 03:17:43PM -0700, Suren Baghdasaryan wrote:
> > > > > > Ironically, checkpatch generates warnings for these type casts:
> > > > > >
> > > > > > WARNING: unnecessary cast may hide bugs, see
> > > > > > http://c-faq.com/malloc/mallocnocast.html
> > > > > > #425: FILE: include/linux/dma-fence-chain.h:90:
> > > > > > + ((struct dma_fence_chain *)kmalloc(sizeof(struct dma_fence_chain),
> > > > > > GFP_KERNEL))
> > > > > >
> > > > > > I guess I can safely ignore them in this case (since we cast to the
> > > > > > expected type)?
> > > > >
> > > > > I find ignoring checkpatch to be a solid move 99% of the time.
> > > > >
> > > > > I really don't like the codetags. This is so much churn, and it could
> > > > > all be avoided by just passing in _RET_IP_ or _THIS_IP_ depending on
> > > > > whether we wanted to profile this function or its caller. vmalloc
> > > > > has done it this way since 2008 (OK, using __builtin_return_address())
> > > > > and lockdep has used _THIS_IP_ / _RET_IP_ since 2006.
> > > >
> > > > Except you can't. We've been over this; using that approach for tracing
> > > > is one thing, using it for actual accounting isn't workable.
> > >
> > > I missed that. There have been many emails. Please remind us of the
> > > reasoning here.
> >
> > I think it's on the other people claiming 'oh this would be so easy if
> > you just do it this other way' to put up some code - or at least more
> > than hot takes.
> >
> > But, since you asked - one of the main goals of this patchset was to be
> > fast enough to run in production, and if you do it by return address
> > then you've added at minimum a hash table lookup to every allocate and
> > free; if you do that, running it in production is completely out of the
> > question.
> >
> > Besides that - the issues with annotating and tracking the correct
> > callsite really don't go away, they just shift around a bit. It's true
> > that the return address approach would be easier initially, but that's
> > not all we're concerned with; we're concerned with making sure
> > allocations get accounted to the _correct_ callsite so that we're giving
> > numbers that you can trust, and by making things less explicit you make
> > that harder.
> >
> > Additionally: the alloc_hooks() macro is for more than this. It's also
> > for more usable fault injection - remember every thread we have where
> > people are begging for every allocation to be __GFP_NOFAIL - "oh, error
> > paths are hard to test, let's just get rid of them" - never mind that
> > actually do have to have error paths - but _per callsite_ selectable
> > fault injection will actually make it practical to test memory error
> > paths.
> >
> > And Kees working on stuff that'll make use of the alloc_hooks() macro
> > for segregating kmem_caches.
>
> Yeah, that pretty much summarizes it. Note that we don't have to make
> the conversions in this patch and accounting will still work but then
> all allocations from different callers will be accounted to the helper
> function and that's less useful than accounting at the call site.
> It's a sizable churn but the conversions are straight-forward and we
> do get accurate, performant and easy to use memory accounting.
OK, fair enough. I guess I can live with the allocation macros in jbd2 if
type safety is preserved. But please provide a short summary of why we need
these macros (e.g. instead of RET_IP approach) in the changelog (or at
least a link to some email explaining this if the explanation would get too
long). Because I was wondering about the same as Andrew (and yes, this is
because I wasn't really following the huge discussion last time).
Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR