Message-ID: <6501f1c29a2c4_31df46294ec@iweiny-mobl.notmuch>
Date:   Wed, 13 Sep 2023 10:30:42 -0700
From:   Ira Weiny <ira.weiny@...el.com>
To:     Dan Williams <dan.j.williams@...el.com>,
        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

Dan Williams wrote:
> 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.

Agreed.  And this is the way things work now.

Also your use of 'Sparse DAX region' seems better than the word 'dynamic'
I have used now.  I know that static regions mean something else but I
could not think of a better word.  I'll make adjustments to the
code/commit messages.

> 
> 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.

How does it collide?

My attempt here is to leave dax devices 'unlabeled'.  As such they will use
space on a 'first-available' search regardless of extent labels.

Effectively I have defined 'no label' as being 'any label'.  I apologize
for this detail being implicit and not explicit.

My envisioned path would be that older daxctl would continue to work like
this because the kernel would not restrict unlabeled dax device creation.

Newer daxctl could use dax device labels to control the extents used.  But
only when dax device labeling is introduced in a future kernel.  Use of a
newer daxctl on an older DCD kernel could continue to work sans label.

In this way I envisioned a path where the policy is completely dictated by
user space restricted only by the software available.

> 
> 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.

I will agree that it was perhaps premature to introduce labels on the
extents.  However, I did so to give tags a space to be informationally
surfaced.

IMO we must have a plan forward or wait until that plan is fully formed
and implemented.  The size of this set is rather large.  Therefore, I was
hoping that a plan would be enough to move forward.

> 
> 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.

Interesting.  I was not considering allocate by label to be a requirement
but rather an enhancement.  Labels IMO are a further refinement of the
memory space allocation.  I can see a very valid use case (not toy use
case) where all the DCD memory allocated to a node is dedicated to a
singular job and is done without tags or even ignoring tags.  Many HPC
sites run with singular jobs per host.

> 
> 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.

Needed?  No.  Of course not.  As you said the orchestrator software can
keep iterating with the FM until it gets what it wants.  It was you who
had the idea of a writable labels and I agreed.

"Seemed like a good idea at the time..."  ;-)

As I have reviewed and rewritten this message I worry that writable labels
are a bad idea.  Interleaving will most likely depend on grouping extent
tags into the CXL/DAX extent.  With this in mind adjusting extents is
potentially going to require an FM interaction to get things set up
anyway.

	[Again re-reading my message I thought of another issue.  What
	happens if the user decides to change the label on an extent after
	some dax device with the old label?  That seems like an additional
	complication which is best left out by not allowing extent labels
	to be writable.]

I think writable labels are orthogonal to the kernel behavior though.
Allowing labels to change after the fact is a policy matter which is not
something the kernel needs to manage.

The kernel does need to manage how it allocates a dax device across the
extents available.  Assigning a dax label and allocating to the extents
matching that label is very straight forward.  The real issue is how to
deal with the 'no label' case.

As a path forward, I made a couple of assumptions.  First was the idea of
'no dax device label' == 'any extent label'.  Second, was that current dax
device creation was done as 'no dax device label'.

In this way I did not see a requirement to fully implement label
restriction on dax devices.  Labels are simply a nice to have thing to
group extents later.  Also, if you want dax devices created with specific
extents you have to assign them a label.  Otherwise they are allocated
'first-available' like they have been in the past.

I see a few ways forward.

One is to define 'no dax device label' as 'any extent label' as I have it
now.  IMO this provides the most backwards compatible dax device creation.
The ndctl region code additions are minimal and there are no daxctl
modifications required at all.

A second is to define 'no dax device label' as 'no extent label' and go
forward with this series but add a restriction on dax device creation to
only extents without a label.  This is still pretty compatible but if tags
are used then some extents would not be available without additional
daxctl modifications.

A third way forward is to fully implement label enabled dax device
creation.  In this case I feel like the direction is to make 'no label' ==
'no label'.  This is not hard but will take a couple more weeks to get the
daxctl code and all the testing.

It warrants mentioning that tags are an optional feature.  I feel like
there is momentum in the community to not use tags initially.  And so I
was targeting an initial implementation which really did not need tags at
all.  Perhaps I am wrong in that assumption?  Or perhaps I was short
sighted (possibly because interleaving becomes more straight forward)?

To summarize I see the following fundamental questions.

	1) Do we require DCD support to require dax device label
	   management?
	2) What does 'no dax device label' mean?
		a) any extent label
		b) no extent label
	3) Should writable labels be allowed on extents?
		a) this is more flexible
		b) security issues?
		c) does it just confuse things with interleaving?
		d) nice to change the tag name to something easy to read?
		e) other issues?
	4) How should the available size for labels be communicated to the
	   user?
	   	a) currently available size reflects an 'any extent label'
		   behavior when there is no label on the dax device.
		b) this becomes an issue if labelless dax devices are
		   restricted to labelless extents.

My current view is:
	1) No.  Current dax devices can be defined as 'no label'
	2) I'm not sure.  I can see both ways having benefits.
	3) No I think the ROI is not worth it.
	4) The use of 'any extent label' in #2 means that available size
	   retains it's meaning for no label dax devices.  Labeled dax
	   devices would require a future enhancement to size information.

> 
> > > > > > @@ -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.

Indeed.  I forgot about those panic on warn users.  I'll remove the warn
altogether.

Thanks,
Ira

Powered by blists - more mailing lists