Message-ID: <6500e7cdac9dc_71e582945a@dwillia2-xfh.jf.intel.com.notmuch>
Date:   Tue, 12 Sep 2023 15:35:57 -0700
From:   Dan Williams <dan.j.williams@...el.com>
To:     Ira Weiny <ira.weiny@...el.com>,
        Jonathan Cameron <Jonathan.Cameron@...wei.com>
CC:     Dan Williams <dan.j.williams@...el.com>,
        Navneet Singh <navneet.singh@...el.com>,
        Fan Ni <fan.ni@...sung.com>,
        Davidlohr Bueso <dave@...olabs.net>,
        Dave Jiang <dave.jiang@...el.com>,
        Alison Schofield <alison.schofield@...el.com>,
        Vishal Verma <vishal.l.verma@...el.com>,
        <linux-cxl@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH RFC v2 14/18] dax/region: Support DAX device creation on
 dynamic DAX regions

Ira Weiny wrote:
> Jonathan Cameron wrote:
> > On Tue, 5 Sep 2023 21:35:03 -0700
> > Ira Weiny <ira.weiny@...el.com> wrote:
> > 
> > > Jonathan Cameron wrote:
> > > > On Mon, 28 Aug 2023 22:21:05 -0700
> > > > Ira Weiny <ira.weiny@...el.com> wrote:
> > > >   
> > > > > Dynamic Capacity (DC) DAX regions have a list of extents which define
> > > > > the memory of the region which is available.
> > > > > 
> > > > > Now that DAX region extents are fully realized support DAX device
> > > > > creation on dynamic regions by adjusting the allocation algorithms
> > > > > to account for the extents.  Remember also references must be held on
> > > > > the extents until the DAX devices are done with the memory.
> > > > > 
> > > > > Redefine the region available size to include only extent space.  Reuse
> > > > > the size allocation algorithm by defining sub-resources for each extent
> > > > > and limiting range allocation to those extents which have space.  Do not
> > > > > support direct mapping of DAX devices on dynamic devices.
> > > > > 
> > > > > Enhance DAX device range objects to hold references on the extents until
> > > > > the DAX device is destroyed.
> > > > > 
> > > > > NOTE: At this time all extents within a region are created equally.
> > > > > However, labels are associated with extents which can be used with
> > > > > future DAX device labels to group which extents are used.  
> > > > 
> > > > This sound like a bad place to start to me as we are enabling something
> > > > that is probably 'wrong' in the long term as opposed to just not enabling it
> > > > until we have appropriate support.  
> > > 
> > > I disagree.  I don't think the kernel should be trying to process tags at
> > > the lower level.
> > > 
> > > > I'd argue better to just reject any extents with different labels for now.  
> > > 
> > > Again I disagree.  This is less restrictive.  The idea is that labels can
> > > be changed such that user space can ultimately decided which extents
> > > should be used for which devices.  I have some work on that already.
> > > (Basically it becomes quite easy to assign a label to a dax device and
> > > have the extent search use only dax extents which match that label.)
> > 
> > That sounds good - but if someone expects that and uses it with an old
> > kernel I'm not sure if it is better to say 'we don't support it yet' or
> > do something different from a newer kernel.
> 
> This does provide the 'we don't support that yet' in that dax device
> creation can't be associated with a label yet.  So surfacing the extents
> with the tag as a default label and letting those labels change is more
> informational at this point and not functional.  Simple use cases can use
> the label (from the tag) to detect that some extent with the wrong tag got
> in the region but can't correct it without going through the FM.
> 
> It is easy enough to remove the label sysfs and defer that until the dax
> device has a label and this support though.

Catching up on just this point (still need to go through the whole
thing).  A Sparse DAX region is one where the extents need not be
present at DAX region instantiation and may be added/removed later. The
device-dax allocation scheme just takes a size to do a "first-available"
search for free capacity in the region.

Given that one of the expected DCD use cases is to provide just in time
memory for specific jobs the "first-available" search for free capacity
in a Sparse DAX Region collides with the need to keep allocations
bounded by tag.

I agree with Jonathan that unless and until the allocation scheme is
updated to be tag aware then there is no reason for allocate by tag to
exist in the interface.

That said, the next question, "is DCD enabling considered a toy until
the ability to allocate by tag is present?" I think yes, to the point
where old daxctl binaries should be made fail to create device instances
by forcing a tag to be selected at allocation time for Sparse DAX
Regions.

The last question is whether *writable* tags are needed to allow for
repurposing memory allocated to a host without needing to round trip it
through the FM to get it re-tagged. While that is something the host and
orchestrator can figure out on their own, it looks like a nice to have
until the above questions are answered.

> > > > > @@ -1400,8 +1507,10 @@ struct dev_dax *devm_create_dev_dax(struct dev_dax_data *data)
> > > > >  	device_initialize(dev);
> > > > >  	dev_set_name(dev, "dax%d.%d", dax_region->id, dev_dax->id);
> > > > >  
> > > > > +	dev_WARN_ONCE(parent, is_dynamic(dax_region) && data->size,
> > > > > +		      "Dynamic DAX devices are created initially with 0 size");  
> > > > 
> > > > dev_info() maybe more appropriate?  
> > > 
> > > Unless I'm mistaken this can happen from userspace but only if something
> > > in the code changes later.  Because the dax layer is trying to support
> > > non-dynamic regions (which dynamic may be a bad name), I was worried that
> > > the creation with a size might slip through...
> > 
> > Fair enough - if strong chance userspace will control it at somepoitn then
> > ONCE seems fine.
> > 
> > > 
> > > > Is this common enough that we need the
> > > > _ONCE?  
> > > 
> > > once is because it could end up spamming a log later if something got
> > > coded up wrong.
> > 
> > I'm not sure I care about bugs spamming the log.   Only things that
> > are userspace controlled or likely hardware failures etc.
> > 
> 
> Understood.  Let me trace them again but I think these can be triggered by
> user space.  If not I'll remove the ONCE.

Unless this is an unequivocal kernel bug if it fires, and there is a
significant potential for active development to do the wrong thing,
don't leave a panic_on_warn land mine.

Powered by blists - more mailing lists