Re: [PATCH RESEND 1/4] memory tiering: add abstract distance calculation algorithms management

[Huang, Ying]

Alistair Popple <apopple@xxxxxxxxxx> writes:

> "Huang, Ying" <ying.huang@xxxxxxxxx> writes:
>
>> Alistair Popple <apopple@xxxxxxxxxx> writes:
>>
>>> "Huang, Ying" <ying.huang@xxxxxxxxx> writes:
>>>
>>>> Alistair Popple <apopple@xxxxxxxxxx> writes:
>>>>
>>>>> "Huang, Ying" <ying.huang@xxxxxxxxx> writes:
>>>>>
>>>>>> Hi, Alistair,
>>>>>>
>>>>>> Sorry for late response.  Just come back from vacation.
>>>>>
>>>>> Ditto for this response :-)
>>>>>
>>>>> I see Andrew has taken this into mm-unstable though, so my bad for not
>>>>> getting around to following all this up sooner.
>>>>>
>>>>>> Alistair Popple <apopple@xxxxxxxxxx> writes:
>>>>>>
>>>>>>> "Huang, Ying" <ying.huang@xxxxxxxxx> writes:
>>>>>>>
>>>>>>>> Alistair Popple <apopple@xxxxxxxxxx> writes:
>>>>>>>>
>>>>>>>>> "Huang, Ying" <ying.huang@xxxxxxxxx> writes:
>>>>>>>>>
>>>>>>>>>> Alistair Popple <apopple@xxxxxxxxxx> writes:
>>>>>>>>>>
>>>>>>>>>>>>>> While other memory device drivers can use the general notifier chain
>>>>>>>>>>>>>> interface at the same time.
>>>>>>>>>>>
>>>>>>>>>>> How would that work in practice though? The abstract distance as far as
>>>>>>>>>>> I can tell doesn't have any meaning other than establishing preferences
>>>>>>>>>>> for memory demotion order. Therefore all calculations are relative to
>>>>>>>>>>> the rest of the calculations on the system. So if a driver does it's own
>>>>>>>>>>> thing how does it choose a sensible distance? IHMO the value here is in
>>>>>>>>>>> coordinating all that through a standard interface, whether that is HMAT
>>>>>>>>>>> or something else.
>>>>>>>>>>
>>>>>>>>>> Only if different algorithms follow the same basic principle.  For
>>>>>>>>>> example, the abstract distance of default DRAM nodes are fixed
>>>>>>>>>> (MEMTIER_ADISTANCE_DRAM).  The abstract distance of the memory device is
>>>>>>>>>> in linear direct proportion to the memory latency and inversely
>>>>>>>>>> proportional to the memory bandwidth.  Use the memory latency and
>>>>>>>>>> bandwidth of default DRAM nodes as base.
>>>>>>>>>>
>>>>>>>>>> HMAT and CDAT report the raw memory latency and bandwidth.  If there are
>>>>>>>>>> some other methods to report the raw memory latency and bandwidth, we
>>>>>>>>>> can use them too.
>>>>>>>>>
>>>>>>>>> Argh! So we could address my concerns by having drivers feed
>>>>>>>>> latency/bandwidth numbers into a standard calculation algorithm right?
>>>>>>>>> Ie. Rather than having drivers calculate abstract distance themselves we
>>>>>>>>> have the notifier chains return the raw performance data from which the
>>>>>>>>> abstract distance is derived.
>>>>>>>>
>>>>>>>> Now, memory device drivers only need a general interface to get the
>>>>>>>> abstract distance from the NUMA node ID.  In the future, if they need
>>>>>>>> more interfaces, we can add them.  For example, the interface you
>>>>>>>> suggested above.
>>>>>>>
>>>>>>> Huh? Memory device drivers (ie. dax/kmem.c) don't care about abstract
>>>>>>> distance, it's a meaningless number. The only reason they care about it
>>>>>>> is so they can pass it to alloc_memory_type():
>>>>>>>
>>>>>>> struct memory_dev_type *alloc_memory_type(int adistance)
>>>>>>>
>>>>>>> Instead alloc_memory_type() should be taking bandwidth/latency numbers
>>>>>>> and the calculation of abstract distance should be done there. That
>>>>>>> resovles the issues about how drivers are supposed to devine adistance
>>>>>>> and also means that when CDAT is added we don't have to duplicate the
>>>>>>> calculation code.
>>>>>>
>>>>>> In the current design, the abstract distance is the key concept of
>>>>>> memory types and memory tiers.  And it is used as interface to allocate
>>>>>> memory types.  This provides more flexibility than some other interfaces
>>>>>> (e.g. read/write bandwidth/latency).  For example, in current
>>>>>> dax/kmem.c, if HMAT isn't available in the system, the default abstract
>>>>>> distance: MEMTIER_DEFAULT_DAX_ADISTANCE is used.  This is still useful
>>>>>> to support some systems now.  On a system without HMAT/CDAT, it's
>>>>>> possible to calculate abstract distance from ACPI SLIT, although this is
>>>>>> quite limited.  I'm not sure whether all systems will provide read/write
>>>>>> bandwith/latency data for all memory devices.
>>>>>>
>>>>>> HMAT and CDAT or some other mechanisms may provide the read/write
>>>>>> bandwidth/latency data to be used to calculate abstract distance.  For
>>>>>> them, we can provide a shared implementation in mm/memory-tiers.c to map
>>>>>> from read/write bandwith/latency to the abstract distance.  Can this
>>>>>> solve your concerns about the consistency among algorithms?  If so, we
>>>>>> can do that when we add the second algorithm that needs that.
>>>>>
>>>>> I guess it would address my concerns if we did that now. I don't see why
>>>>> we need to wait for a second implementation for that though - the whole
>>>>> series seems to be built around adding a framework for supporting
>>>>> multiple algorithms even though only one exists. So I think we should
>>>>> support that fully, or simplfy the whole thing and just assume the only
>>>>> thing that exists is HMAT and get rid of the general interface until a
>>>>> second algorithm comes along.
>>>>
>>>> We will need a general interface even for one algorithm implementation.
>>>> Because it's not good to make a dax subsystem driver (dax/kmem) to
>>>> depend on a ACPI subsystem driver (acpi/hmat).  We need some general
>>>> interface at subsystem level (memory tier here) between them.
>>>
>>> I don't understand this argument. For a single algorithm it would be
>>> simpler to just define acpi_hmat_calculate_adistance() and a static
>>> inline version of it that returns -ENOENT when !CONFIG_ACPI than adding
>>> a layer of indirection through notifier blocks. That breaks any
>>> dependency on ACPI and there's plenty of precedent for this approach in
>>> the kernel already.
>>
>> ACPI is a subsystem, so it's OK for dax/kmem to depends on CONFIG_ACPI.
>> But HMAT is a driver of ACPI subsystem (controlled via
>> CONFIG_ACPI_HMAT).  It's not good for a driver of DAX subsystem
>> (dax/kmem) to depend on a *driver* of ACPI subsystem.
>>
>> Yes.  Technically, there's no hard wall to prevent this.  But I think
>> that a good design should make drivers depends on subsystems or drivers
>> of the same subsystem, NOT drivers of other subsystems.
>
> Thanks, I wasn't really thinking of HMAT as an ACPI driver. I understand
> where you're coming from but I really don't see the problem with using a
> static inline. It doesn't create dependencies (you could still use
> dax/kmem without ACPI) and results in smaller and easier to follow code.
>
> IMHO it's far more obvious that a call to acpi_hmat_calcaulte_adist()
> returns either a default if ACPI HMAT isn't configured or a calculated
> value than it is to figure out what notifiers may or may not be
> registered at runtime and what priority they may be called in from
> mt_calc_adistance().
>
> It appears you think that is a bad design, but I don't understand
> why. What does this approach give us that a simpler approach wouldn't?

Think about all these again.  Finally I admit you are right.  The
general interface is better mainly if there are multiple implementations
of the interface.

In this series, we provide just one implementation: HMAT.  And, the
second one: CDAT will be implemented soon.  And, CDAT will use the same
method to translate from read/write bandwidth/latency to adistance.  So,
I suggest to:

- Keep the general interface (and notifier chain), for HMAT and soon
  available CDAT

- Move the code to translate from read/write bandwidth/latency to
  adistance to memory-tiers.c.  Which is used by HMAT now and will be
  used by CDAT soon.  And it can be used by other drivers.

What do you think about that?

--
Best Regards,
Huang, Ying