Re: [RFC][PATCH v2 08/10] smp,irq_work: Use the new irq_work API

From: peterz@infradead.org
To: Christoph Hellwig <hch@lst.de>
Cc: mingo@kernel.org, torvalds@linux-foundation.org,
	linux-kernel@vger.kernel.org, will@kernel.org,
	paulmck@kernel.org, axboe@kernel.dk, chris@chris-wilson.co.uk,
	davem@davemloft.net, kuba@kernel.org, fweisbec@gmail.com,
	oleg@redhat.com, vincent.guittot@linaro.org
Subject: Re: [RFC][PATCH v2 08/10] smp,irq_work: Use the new irq_work API
Date: Wed, 19 Aug 2020 09:22:09 +0200	[thread overview]
Message-ID: <20200819072209.GU2674@hirez.programming.kicks-ass.net> (raw)
In-Reply-To: <20200818162542.GB27196@lst.de>

On Tue, Aug 18, 2020 at 06:25:42PM +0200, Christoph Hellwig wrote:
> On Tue, Aug 18, 2020 at 12:51:10PM +0200, Peter Zijlstra wrote:
> > Convert the performance sensitive users of
> > smp_call_single_function_async() over to the new
> > irq_work_queue_remote_static().
> > 
> > The new API is marginally less crap but taking these users away allows
> > fixing up smp_call_single_function_async() without risk of performance
> > regressions.
> 
> You probably want a conversion patch per subsystem so that it sticks
> out.  What is so crap about this API?  How could we as subsystem
> maintainers help to make it less crappy?

The problem with both the current smp_call_function_single_async() and
the proposed irq_work_queue_remote_static() is that they require
external serialization and lifetime management.

That is, the external serialization comes from the non-atomic
test-and-set they both have. This works nicely when there is external
state that already serializes things, but totally comes apart (and
causes trivial list corruption) when you get it wrong.

The life-time thing is because you can't tell when the IPI is done.

The newly introduced irq_work_queue_remote() suffers neither of these
problems, and patch 9 fixes the first for
smp_call_function_single_async(). The whole smp_call_function*() class
suffers the second issue for .wait=0, typically they get combined with a
completion or some other state when/where it matters.

Patch 9 also shows why I introduced irq_work_queue_remote_static(), the
additional atomic op on enqueue is of course not cheap, and I can
imagine a bunch of users that don't really need it not wanting to pay
that price.

From a user pov (I'm one too), I'm not sure what we can do about this,
other than possibly accept the extra overhead :/

I do have a TODO item to see if I can come up with extra debugging
checks to catch abuse of these fragile things. One possible thing is to
have csd_unlock() also set csd->next = NULL, and have the llist_add()
users verify csd->next == NULL before doing the add.