Re: [PATCH RFC v2 11/18] cxl/region: Expose DC extents on region driver load

[Ira Weiny]

Jonathan Cameron wrote:
> On Mon, 28 Aug 2023 22:21:02 -0700
> Ira Weiny <ira.weiny@xxxxxxxxx> wrote:
> 
> > Ultimately user space must associate Dynamic Capacity (DC) extents with
> > DAX devices.  Remember also that DCD extents may have been accepted
> > previous to regions being created and must have references held until
> > all higher level regions and DAX devices are done with the memory.
> > 
> > On CXL region driver load scan existing device extents and create CXL
> > DAX region extents as needed.
> > 
> > Create abstractions for the extents to be used in DAX region.  This
> > includes a generic interface to take proper references on the lower
> > level CXL region extents.
> > 
> > Also maintain separate objects for the DAX region extent device vs the
> > DAX region extent.  The DAX region extent device has a shorter life span
> > which corresponds to the removal of an extent while a DAX device is
> > still using it.  In this case an extent continues to exist whilst the
> > ability to create new DAX devices on that extent is prevented.
> > 
> > NOTE: Without interleaving; the device, CXL region, and DAX region
> > extents have a 1:1:1 relationship.  Future support for interleaving will
> > maintain a 1:N relationship between CXL region extents and the hardware
> > extents.
> > 
> > While the ability to create DAX devices on an extent exists; expose the
> > necessary details of DAX region extents by creating a device with the
> > following sysfs entries.
> > 
> > /sys/bus/cxl/devices/dax_regionX/extentY
> > /sys/bus/cxl/devices/dax_regionX/extentY/length
> > /sys/bus/cxl/devices/dax_regionX/extentY/label
> > 
> > Label is a rough analogy to the DC extent tag.  As such the DC extent
> > tag is used to initially populate the label.  However, the label is made
> > writeable so that it can be adjusted in the future when forming a DAX
> > device.
> > 
> > Signed-off-by: Navneet Singh <navneet.singh@xxxxxxxxx>
> > Co-developed-by: Navneet Singh <navneet.singh@xxxxxxxxx>
> > Signed-off-by: Ira Weiny <ira.weiny@xxxxxxxxx>
> > 
> 
> Trivial stuff inline.
> 
> 
> 
> > diff --git a/drivers/dax/dax-private.h b/drivers/dax/dax-private.h
> > index 27cf2daaaa79..4dab52496c3f 100644
> > --- a/drivers/dax/dax-private.h
> > +++ b/drivers/dax/dax-private.h
> > @@ -5,6 +5,7 @@
> >  #ifndef __DAX_PRIVATE_H__
> >  #define __DAX_PRIVATE_H__
> >  
> > +#include <linux/pgtable.h>
> >  #include <linux/device.h>
> >  #include <linux/cdev.h>
> >  #include <linux/idr.h>
> > @@ -40,6 +41,58 @@ struct dax_region {
> >  	struct device *youngest;
> >  };
> >  
> > +/*
> /**
> 
> as it's valid kernel doc so no disadvantage really.

Sure. Done.

> 
> > + * struct dax_region_extent - extent data defined by the low level region
> > + * driver.
> > + * @private_data: lower level region driver data
> > + * @ref: track number of dax devices which are using this extent
> > + * @get: get reference to low level data
> > + * @put: put reference to low level data
> 
> I'd like to understand when these are optional - perhaps comment on that?

They are not optional in this implementation.  I got a bit carried away in
extrapolating the dax_region away from the lower levels in thinking that
some other implementation may not need these.

I will still keep the helpers below though.

> 
> > + */
> > +struct dax_region_extent {
> > +	void *private_data;
> > +	struct kref ref;
> > +	void (*get)(struct dax_region_extent *dr_extent);
> > +	void (*put)(struct dax_region_extent *dr_extent);
> > +};
> > +
> > +static inline void dr_extent_get(struct dax_region_extent *dr_extent)
> > +{
> > +	if (dr_extent->get)
> > +		dr_extent->get(dr_extent);
> > +}
> > +
> > +static inline void dr_extent_put(struct dax_region_extent *dr_extent)
> > +{
> > +	if (dr_extent->put)
> > +		dr_extent->put(dr_extent);
> > +}
> > +
> > +#define DAX_EXTENT_LABEL_LEN 64
> 
> blank line here.

Sure.  Done

Ira