"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?
