Re: [PATCH v3 02/14] cxl/region: Introduce concept of region configuration

From: Dan Williams <dan.j.williams@intel.com>
To: Ben Widawsky <ben.widawsky@intel.com>
Cc: linux-cxl@vger.kernel.org, patches@lists.linux.dev,
	 kernel test robot <lkp@intel.com>,
	Alison Schofield <alison.schofield@intel.com>,
	 Ira Weiny <ira.weiny@intel.com>,
	Jonathan Cameron <Jonathan.Cameron@huawei.com>,
	 Vishal Verma <vishal.l.verma@intel.com>,
	Bjorn Helgaas <helgaas@kernel.org>,
	 Linux NVDIMM <nvdimm@lists.linux.dev>,
	Linux PCI <linux-pci@vger.kernel.org>
Subject: Re: [PATCH v3 02/14] cxl/region: Introduce concept of region configuration
Date: Thu, 3 Feb 2022 18:45:30 -0800	[thread overview]
Message-ID: <CAPcyv4gYzeYUNBDobktrNRsBvqx1YBchnLi3YN6fTqm0RxemWA@mail.gmail.com> (raw)
In-Reply-To: <20220204001950.cxncxxxsmoyc6jcy@intel.com>

On Thu, Feb 3, 2022 at 4:20 PM Ben Widawsky <ben.widawsky@intel.com> wrote:
[..]
> > > > Basic input validation to me is things like:
> > > >
> > > > - Don't allow writes while the region is active
> > > > - Check that values are in bound. So yes, the interleave-ways value of
> > > > 3 would fail until the kernel supports it, and granularity values >
> > > > 16K would also fail.
> > > > - Check that memdevs are actually downstream targets of the given decoder
> > > > - Check that the region uuid is unique
> > >
> > > These are obviously easy and informative at attr store time (in fact, active was
> > > meant to be checked already for many cases). So if we agree to codify this at
> > > probe via WARN, and add it to kdoc, I've no problem with it.
> >
> > Why is WARN needed? Either the sysfs validation does it job correctly
> > or it doesn't. Also if sysfs didn't WARN when the bad input is
> > specified why would the core do anything higher than dev_err()?
> > Basically I think the bar for WARN is obvious kernel programming error
> > where only a kernel-developer will see it vs EINVAL at runtime
> > scenarios. I have seen Greg raise the bar for WARN in his reviews
> > given how many deployments turn on 'panic_on_warn'.
>
> Ultimately some checking will need to occur in one form or another in region
> probe(). Either explicit via conditional: if (!is_valid(interleave_ways)) return
> -EINVAL, or implicitly, for example 1 << (rootd_ig - cxlr_ig) is some invalid
> nonsense which later fails host bridge programming verification. Before
> discussing further, which are you suggesting?

Explicit validation at probe in addition to the explicit validation at
the sysfs boundary (as much as possible to report errors early). The
"at probe time" validation does not know if this was a new region, or
one enumerated from LSA or the configuration that the BIOS specified.
So I do expect validation overlap, but there will also be distinct
checks for those different scenarios. For example, see how NVDIMM
validates namespace configuration writes via sysfs, but does not
validate the LSA because it's writing the label and had better be
prepared to read what it writes.

>
> >
> > > > - Check that decoder has capacity
> > > > - Check that the memdev has capacity
> > > > - Check that the decoder to map the DPA is actually available given
> > > > decoders must be programmed in increasing DPA order
> > > >
> > > > Essentially any validation short of walking the topology to program
> > > > upstream decoders since those errors are only resolved by racing
> > > > region probes that try to grab upstream decoder resources.
> > > >
> > >
> > > I intentionally avoided doing a lot of these until probe because it seemed like
> > > not a great policy to deny regions from being populated if another region
> > > utilizing those resources hasn't been bound yes. For a simple example, if x1
> > > region A is created and utilizes all of memdev ɑ's capacity you block out any
> > > other region setup using memdev ɑ, even if region A wasn't bound. There's a
> > > similar problem with specifying decoders as part of configuration.
> > >
> > > I'll infer from your comment that you are fine with this tradeoff, or you have
> > > some other way to manage this in mind.
> >
> > It comes back to observability if threadA allocates all the DPA then
> > yes all other threads should see -ENOSPC. No different than if 3 fdisk
> > threads all tried to create a partition, the first one to the kernel
> > wins. If threadA does not end up activating that regionA's capacity
> > that's userspace's fault, and the admin needs to make sure that
> > configuration does not race itself. The kernel allocating DPA
> > immediately lets those races be found early such that threadB finds
> > all the DPA gone and stops trying to create the region.
>
> Okay. I don't have a strong opinion on how userspace should or shouldn't use
> this interface. It seems less friendly to do it this way, but per the following
> comment, if it's root only, it doesn't really matter.
>
> I was under the impression you expected userspace to manage the DPA as well. I
> don't really see any reason why the kernel should manage it if userspace is
> already handling all these other bits. Let userspace set the offset and size
> (can make a single device attr for it), and upon doing so it gets reserved.

Userspace sense requests, kernel allocates or denies that request
after resolving races with other requesters. Yes, this makes the
interface stateful. Sysfs is not suited to stateless operation.