Re: [RFC PATCH 02/15] cxl/core/hdm: Bail on endpoint init fail

[Dan Williams <dan.j.williams@intel.com>]

On Fri, May 13, 2022 at 8:12 AM Luis Chamberlain <mcgrof@kernel.org> wrote:
>
> On Fri, May 13, 2022 at 01:09:09PM +0100, Jonathan Cameron wrote:
> > On Thu, 12 May 2022 10:27:38 -0700
> > Luis Chamberlain <mcgrof@kernel.org> wrote:
> >
> > > On Thu, May 12, 2022 at 08:50:14AM -0700, Ben Widawsky wrote:
> > > > On 22-04-18 16:37:12, Adam Manzanares wrote:
> > > > > On Wed, Apr 13, 2022 at 02:31:42PM -0700, Dan Williams wrote:
> > > > > > On Wed, Apr 13, 2022 at 11:38 AM Ben Widawsky <ben.widawsky@intel.com> wrote:
> > > > > > >
> > > > > > > Endpoint decoder enumeration is the only way in which we can determine
> > > > > > > Device Physical Address (DPA) -> Host Physical Address (HPA) mappings.
> > > > > > > Information is obtained only when the register state can be read
> > > > > > > sequentially. If when enumerating the decoders a failure occurs, all
> > > > > > > other decoders must also fail since the decoders can no longer be
> > > > > > > accurately managed (unless it's the last decoder in which case it can
> > > > > > > still work).
> > > > > >
> > > > > > I think this should be expanded to fail if any decoder fails to
> > > > > > allocate anywhere in the topology otherwise it leaves a mess for
> > > > > > future address translation code to work through cases where decoder
> > > > > > information is missing.
> > > > > >
> > > > > > The current approach is based around the current expectation that
> > > > > > nothing is enumerating pre-existing regions, and nothing is performing
> > > > > > address translation.
> > > > >
> > > > > Does the qemu support currently allow testing of this patch? If so, it would
> > > > > be good to reference qemu configurations. Any other alternatives would be
> > > > > welcome as well.
> > > > >
> > > > > +Luis on cc.
> > > > >
> > > >
> > > > No. This type of error injection would be cool to have, but I'm not sure of a
> > > > good way to support that in a scalable way. Maybe Jonathan has some ideas?
> > >
> > > In case it helps on the Linux front the least intrusive way is to use
> > > ALLOW_ERROR_INJECTION(). It's what I hope we'll slowly strive for on
> > > the block layer and filesystems slowly. That incurs one macro call per error
> > > routine you want to allow error injection on.
> > >
> > > Then you use debugfs to dynamically enable / disable the error
> > > injection / rate etc.
> > >
> > > So I think this begs the question, what error injection mechanisms
> > > exist for qemu and would new functionality be welcomed?
> >
> > So what paths can actually cause this to fail?
>
> If you are asking about adopting something like the failmalloc
> should_fail() strategy in qemu, you'd essentially open code a call to
> a should_fail() and in it pass the arguments you want from your
> own call down. If you want to ignore size you can just pass 0
> for instance.
>
> > Looking at the upstream
> > code in init_hdm_decoder() looks like there are only a few things that
> > are checked.
>
> If you mean in Linux, you would open code a should_fail()
> specific to the area as in this commit old commit example, and
> adding a respective kconfig entry for it:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=8a8b6502fb669c3a0638a08955442814cedc86b1
>
> Eech of these knobs then get its own probability, times, and space
> debugfs entries which let the routine should_fail() fail when the
> parameters set meet the criteria set by debugfs.
>
> There are ways to make this much more scalable though, but I had not
> seen many efforts to do so. I did start such an approach using debugfs
> specific to *one* kconfig entry, for instance see this block layer proposed
> change, which would in turn enable tons of different ways to enable failing
> if CONFIG_FAIL_ADD_DISK would be used:
>
> https://lore.kernel.org/linux-block/20210512064629.13899-9-mcgrof@kernel.org/
>
> However, at the recent discussion at LSFMM for this we decided instead
> to just sprinkle ALLOW_ERROR_INJECTION() after each routine. Otherwise
> you are open coding tons of new "should_fail()" calls in your runtime
> path and that can make it hard to review patches and is just a lot of
> noise in code.
>
> But with CONFIG_FAIL_FUNCTION this means you don't have to open code
> should_fail() calls, but instead for each routine you want to add a failure
> injection support you'd just use ALLOW_ERROR_INJECTION() per call.

So cxl_test takes the opposite approach and tries not to pollute the
production code with test instrumentation. All of the infrastructure
to replace calls and inject mocked values is self contained in
tools/testing/cxl/ where it builds replacement modules with test
instrumentation. Otherwise its a maintenance burden, in my view, to
read the error injection macros in the nominal code paths.

> Read Documentation/fault-injection/fault-injection.rst on
> fail_function/injectable and fail_function/<function-name>/retval,
> so we could do for instance, to avoid a namespace clash I just
> added the cxl_ prefix:

Certainly those would be good to use behind the mocked interfaces.