Re: [PATCH v2 1/1] memory tier: consolidate the initialization of memory tiers

[Jonathan Cameron]

On Wed, 03 Jul 2024 16:51:40 +0800
"Huang, Ying" <ying.huang@intel.com> wrote:

> Jonathan Cameron <Jonathan.Cameron@Huawei.com> writes:
> 
> > On Fri, 28 Jun 2024 06:09:23 +0000
> > "Ho-Ren (Jack) Chuang" <horen.chuang@linux.dev> wrote:  
> 
> [snip]
> 
> >> @@ -875,8 +886,7 @@ static int __meminit memtier_hotplug_callback(struct 
> >> notifier_block *self,
> >>  
> >>  static int __init memory_tier_init(void)
> >>  {
> >> -  int ret, node;
> >> -  struct memory_tier *memtier;
> >> +  int ret;
> >>  
> >>    ret = subsys_virtual_register(&memory_tier_subsys, NULL);
> >>    if (ret)
> >> @@ -887,7 +897,8 @@ static int __init memory_tier_init(void)
> >>                            GFP_KERNEL);
> >>    WARN_ON(!node_demotion);
> >>  #endif
> >> -  mutex_lock(&memory_tier_lock);
> >> +
> >> +  guard(mutex)(&memory_tier_lock);  
> >
> > If this was safe to do without the rest of the change (I think so)
> > then better to pull that out as a trivial precursor so less noise
> > in here.
> >  
> >>    /*
> >>     * For now we can have 4 faster memory tiers with smaller adistance
> >>     * than default DRAM tier.
> >> @@ -897,29 +908,9 @@ static int __init memory_tier_init(void)
> >>    if (IS_ERR(default_dram_type))
> >>            panic("%s() failed to allocate default DRAM tier\n", __func__);
> >>  
> >> -  /*
> >> -   * Look at all the existing N_MEMORY nodes and add them to
> >> -   * default memory tier or to a tier if we already have memory
> >> -   * types assigned.
> >> -   */
> >> -  for_each_node_state(node, N_MEMORY) {
> >> -          if (!node_state(node, N_CPU))
> >> -                  /*
> >> -                   * Defer memory tier initialization on
> >> -                   * CPUless numa nodes. These will be initialized
> >> -                   * after firmware and devices are initialized.
> >> -                   */
> >> -                  continue;
> >> -
> >> -          memtier = set_node_memory_tier(node);
> >> -          if (IS_ERR(memtier))
> >> -                  /*
> >> -                   * Continue with memtiers we are able to setup
> >> -                   */
> >> -                  break;
> >> -  }
> >> -  establish_demotion_targets();
> >> -  mutex_unlock(&memory_tier_lock);
> >> +  /* Record nodes with memory and CPU to set default DRAM performance. */
> >> +  nodes_and(default_dram_nodes, node_states[N_MEMORY],
> >> +            node_states[N_CPU]);  
> >
> > There are systems where (for various esoteric reasons, such as describing an
> > association with some other memory that isn't DRAM where the granularity
> > doesn't match) the CPU nodes contain no DRAM but rather it's one node away.
> > Handling that can be a job for another day though.
> >
> > Why does this need to be computed here?  Why not do it in
> > hmat_set_default_dram_perf? Doesn't seem to be used anywhere else.  
> 
> IMO, which node is default dram node is a general concept instead of
> HMAT specific.  So, I think that it's better to decide that in the
> general code (memory-tiers.c).

That makes sense given I'd imagine this will spread to other firmware
types in time.

Thanks,

Jonathan

> 
> >>  
> >>    hotplug_memory_notifier(memtier_hotplug_callback, MEMTIER_HOTPLUG_PRI);
> >>    return 0;  
> 
> --
> Best Regards,
> Huang, Ying
>