LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Dave Chinner <david@fromorbit.com>
To: "Darrick J. Wong" <djwong@kernel.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>,
	Thomas Gleixner <tglx@linutronix.de>,
	Dennis Zhou <dennis@kernel.org>, Tejun Heo <tj@kernel.org>,
	linux-fsdevel <linux-fsdevel@vger.kernel.org>,
	linux-xfs <linux-xfs@vger.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	Eric Sandeen <sandeen@sandeen.net>,
	Christoph Hellwig <hch@lst.de>
Subject: Re: [GIT PULL] xfs: new code for 5.15
Date: Fri, 3 Sep 2021 08:35:45 +1000	[thread overview]
Message-ID: <20210902223545.GA1826899@dread.disaster.area> (raw)
In-Reply-To: <20210902174311.GG9942@magnolia>

On Thu, Sep 02, 2021 at 10:43:11AM -0700, Darrick J. Wong wrote:
> On Thu, Sep 02, 2021 at 08:47:42AM -0700, Linus Torvalds wrote:
> > On Tue, Aug 31, 2021 at 2:18 PM Darrick J. Wong <djwong@kernel.org> wrote:
> > >
> > > As for new features: we now batch inode inactivations in percpu
> > > background threads, which sharply decreases frontend thread wait time
> > > when performing file deletions and should improve overall directory tree
> > > deletion times.
> > 
> > So no complaints on this one, but I do have a reaction: we have a lot
> > of these random CPU hotplug events, and XFS now added another one.
> > 
> > I don't see that as a problem, but just the _randomness_ of these
> > callbacks makes me go "hmm". And that "enum cpuhp_state" thing isn't
> > exactly a thing of beauty, and just makes me think there's something
> > nasty going on.
> > 
> > For the new xfs usage, I really get the feeling that it's not that XFS
> > actually cares about the CPU states, but that this is literally tied
> > to just having percpu state allocated and active, and that maybe it
> > would be sensible to have something more specific to that kind of use.
> 
> Correct -- we don't really care about cpu state at all; all xfs needs is
> to push batched work items on a per-cpu list to another cpu when a cpu
> goes offline.  I didn't see anything that looked like it handled that
> kind of thing, so ... cpuhp_state it was. :/

Yeah, it appeared to me that cpuhp_state is the replacement for the
simple, old register_cpu_notifier() hotplug interface that we've
used in the past in XFS (e.g. for per-cpu superblock counters back
in 2006). So, like many other subsystems, I just hooked into it with
a subsystem notifier so we don't have to modify global headers in
future for new internal CPU dead notifiers because, as Darrick
mentions, I have more percpu based modifications coming up for
XFS...


> > We have other things that are very similar in nature - like the page
> > allocator percpu caches etc, which for very similar reasons want cpu
> > dead/online notification.
> > 
> > I'm only throwing this out as a reaction to this - I'm not sure
> > another interface would be good or worthwhile, but that "enum
> > cpuhp_state" is ugly enough that I thought I'd rope in Thomas for CPU
> > hotplug, and the percpu memory allocation people for comments.

Calling it ugly is being kind. :/

The part I dislike most about it is that we have to modify a header
file that triggers full kernel rebuilds. Managing patch stacks and
branches where one of them modifies such a header file means quick,
XFS subsystem only kernel rebuilds are a rare thing...

> > IOW, just _maybe_ we would want to have some kind of callback model
> > for "percpu_alloc()" and it being explicitly about allocations
> > becoming available or going away, rather than about CPU state.
> > 
> > Comments?
> 
> Seems like a good fit for us, though I'll let Dave Chinner chime in
> since he's the one with more per-cpu list patches coming up.

I think this might just be semantics - I understand the intent of
Linus's suggestion is to provide a hotplug callback with a per-cpu
allocation region. The thing that we rely on here is, however, that
per-cpu allocation regions are static for the life of the allocation
and always cover all possible CPUs. Hence the callbacks we want here
are really about CPU state changes and I'm not convinced it's the
best idea to conflate CPU state change callbacks with memory
allocation...

That said, I'm all for a better interface to the CPU hotplug
notifications. THe current interface is ... esoteric and to
understand how to use it effectively requires becoming a CPU hotplug
expert. There's something to be said for the simplicity of the old
register_cpu_notifier() interface we used to have...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

  reply	other threads:[~2021-09-02 22:35 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-31 21:18 Darrick J. Wong
2021-09-02 15:47 ` Linus Torvalds
2021-09-02 17:43   ` Darrick J. Wong
2021-09-02 22:35     ` Dave Chinner [this message]
2021-09-03  6:26       ` Thomas Gleixner
2021-09-05  0:21         ` Dave Chinner
2021-09-05 23:28           ` Thomas Gleixner
2021-09-06  2:11             ` Randy Dunlap
2021-09-06  9:42               ` Thomas Gleixner
2021-09-02 19:13   ` Thomas Gleixner
2021-09-03  4:29     ` Christoph Hellwig
2021-09-03 18:40   ` Dennis Zhou
2021-09-02 17:37 ` pr-tracker-bot

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=20210902223545.GA1826899@dread.disaster.area \
    --to=david@fromorbit.com \
    --cc=dennis@kernel.org \
    --cc=djwong@kernel.org \
    --cc=hch@lst.de \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-xfs@vger.kernel.org \
    --cc=sandeen@sandeen.net \
    --cc=tglx@linutronix.de \
    --cc=tj@kernel.org \
    --cc=torvalds@linux-foundation.org \
    --subject='Re: [GIT PULL] xfs: new code for 5.15' \
    /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

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 NNTP newsgroup(s).