From: Suren Baghdasaryan <surenb@google.com>
To: Matthew Wilcox <willy@infradead.org>
Cc: Kent Overstreet <kent.overstreet@linux.dev>,
Andrew Morton <akpm@linux-foundation.org>,
joro@8bytes.org, will@kernel.org,
trond.myklebust@hammerspace.com, anna@kernel.org, arnd@arndb.de,
herbert@gondor.apana.org.au, davem@davemloft.net,
jikos@kernel.org, benjamin.tissoires@redhat.com, tytso@mit.edu,
jack@suse.com, dennis@kernel.org, tj@kernel.org, cl@linux.com,
jakub@cloudflare.com, penberg@kernel.org, rientjes@google.com,
iamjoonsoo.kim@lge.com, vbabka@suse.cz, edumazet@google.com,
kuba@kernel.org, pabeni@redhat.com, iommu@lists.linux.dev,
linux-kernel@vger.kernel.org, linux-nfs@vger.kernel.org,
linux-acpi@vger.kernel.org, acpica-devel@lists.linux.dev,
linux-arch@vger.kernel.org, linux-crypto@vger.kernel.org,
bpf@vger.kernel.org, linux-input@vger.kernel.org,
linux-ext4@vger.kernel.org, linux-mm@kvack.org,
netdev@vger.kernel.org, linux-security-module@vger.kernel.org
Subject: Re: [PATCH 1/1] mm: change inlined allocation helpers to account at the call site
Date: Fri, 5 Apr 2024 06:53:09 -0700 [thread overview]
Message-ID: <CAJuCfpGMSHv7drSu7Veo5CVz3d_Upt8S6Rdx3isi7orct9-uNQ@mail.gmail.com> (raw)
In-Reply-To: <Zg_yHGKpw4HJHdpb@casper.infradead.org>
On Fri, Apr 5, 2024 at 5:44 AM Matthew Wilcox <willy@infradead.org> wrote:
>
> On Thu, Apr 04, 2024 at 07:00:51PM -0400, Kent Overstreet 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 <kent.overstreet@linux.dev> 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.
>
> Well, /proc/vmallocinfo exists, and has existed since 2008, so this is
> slightly more than a "hot take".
>
> > 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.
>
> And yet vmalloc doesn't do that.
>
> > 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.
>
> I'm not convinced that _THIS_IP_ is less precise than a codetag. They
> do essentially the same thing, except that codetags embed the source
> location in the file while _THIS_IP_ requires a tool like faddr2line
> to decode kernel_clone+0xc0/0x430 into a file + line number.
>
> > 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...
>
> Please stop mischaracterising serious thoughtful criticism as whining.
> I don't understand what value codetags bring over using _THIS_IP_ and
> _RET_IP_ and you need to explain that.
The conceptual difference between codetag and _THIS_IP_/_RET_IP_ is
that codetag injects counters at the call site, so you don't need to
spend time finding the appropriate counter to operate on during
allocation.
prev parent reply other threads:[~2024-04-05 13:53 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-04 16:54 [PATCH 1/1] mm: change inlined allocation helpers to account at the call site Suren Baghdasaryan
2024-04-04 17:04 ` Matthew Wilcox
2024-04-04 17:08 ` Suren Baghdasaryan
2024-04-04 22:17 ` Suren Baghdasaryan
2024-04-04 22:28 ` Kent Overstreet
2024-04-04 22:33 ` Matthew Wilcox
2024-04-04 22:38 ` Kent Overstreet
2024-04-04 22:41 ` Andrew Morton
2024-04-04 23:00 ` Kent Overstreet
2024-04-04 23:16 ` Suren Baghdasaryan
2024-04-05 9:53 ` Jan Kara
2024-04-05 13:47 ` Suren Baghdasaryan
2024-04-05 12:44 ` Matthew Wilcox
2024-04-05 13:53 ` Suren Baghdasaryan [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAJuCfpGMSHv7drSu7Veo5CVz3d_Upt8S6Rdx3isi7orct9-uNQ@mail.gmail.com \
--to=surenb@google.com \
--cc=acpica-devel@lists.linux.dev \
--cc=akpm@linux-foundation.org \
--cc=anna@kernel.org \
--cc=arnd@arndb.de \
--cc=benjamin.tissoires@redhat.com \
--cc=bpf@vger.kernel.org \
--cc=cl@linux.com \
--cc=davem@davemloft.net \
--cc=dennis@kernel.org \
--cc=edumazet@google.com \
--cc=herbert@gondor.apana.org.au \
--cc=iamjoonsoo.kim@lge.com \
--cc=iommu@lists.linux.dev \
--cc=jack@suse.com \
--cc=jakub@cloudflare.com \
--cc=jikos@kernel.org \
--cc=joro@8bytes.org \
--cc=kent.overstreet@linux.dev \
--cc=kuba@kernel.org \
--cc=linux-acpi@vger.kernel.org \
--cc=linux-arch@vger.kernel.org \
--cc=linux-crypto@vger.kernel.org \
--cc=linux-ext4@vger.kernel.org \
--cc=linux-input@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=linux-nfs@vger.kernel.org \
--cc=linux-security-module@vger.kernel.org \
--cc=netdev@vger.kernel.org \
--cc=pabeni@redhat.com \
--cc=penberg@kernel.org \
--cc=rientjes@google.com \
--cc=tj@kernel.org \
--cc=trond.myklebust@hammerspace.com \
--cc=tytso@mit.edu \
--cc=vbabka@suse.cz \
--cc=will@kernel.org \
--cc=willy@infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).