Re: [External] Re: [PATCH v6 2/2] memory tier: create CPUless memory tiers after obtaining HMAT info

2024-03-28 Thread Ho-Ren (Jack) Chuang
On Wed, Mar 27, 2024 at 6:37 PM Huang, Ying  wrote:
>
> "Ho-Ren (Jack) Chuang"  writes:
>
> [snip]
>
> > @@ -655,6 +672,34 @@ void mt_put_memory_types(struct list_head 
> > *memory_types)
> >  }
> >  EXPORT_SYMBOL_GPL(mt_put_memory_types);
> >
> > +/*
> > + * This is invoked via `late_initcall()` to initialize memory tiers for
> > + * CPU-less memory nodes after driver initialization, which is
> > + * expected to provide `adistance` algorithms.
> > + */
> > +static int __init memory_tier_late_init(void)
> > +{
> > + int nid;
> > +
> > + mutex_lock(_tier_lock);
> > + for_each_node_state(nid, N_MEMORY)
> > + if (!node_state(nid, N_CPU) &&
> > + node_memory_types[nid].memtype == NULL)
>
> Think about this again.  It seems that it is better to check
> "node_memory_types[nid].memtype == NULL" only here.  Because for all
> node with N_CPU in memory_tier_init(), "node_memory_types[nid].memtype"
> will be !NULL.  And it's possible (in theory) that some nodes becomes
> "node_state(nid, N_CPU) == true" between memory_tier_init() and
> memory_tier_late_init().
>
> Otherwise, Looks good to me.  Feel free to add
>
> Reviewed-by: "Huang, Ying" 
>
> in the future version.
>

Thank you Huang, Ying for your endorsement and
the feedback you've been giving!

> > + /*
> > +  * Some device drivers may have initialized memory 
> > tiers
> > +  * between `memory_tier_init()` and 
> > `memory_tier_late_init()`,
> > +  * potentially bringing online memory nodes and
> > +  * configuring memory tiers. Exclude them here.
> > +  */
> > + set_node_memory_tier(nid);
> > +
> > + establish_demotion_targets();
> > + mutex_unlock(_tier_lock);
> > +
> > + return 0;
> > +}
> > +late_initcall(memory_tier_late_init);
> > +
>
> [snip]
>
> --
> Best Regards,
> Huang, Ying



-- 
Best regards,
Ho-Ren (Jack) Chuang
莊賀任



Re: [PATCH v6 2/2] memory tier: create CPUless memory tiers after obtaining HMAT info

2024-03-27 Thread Huang, Ying
"Ho-Ren (Jack) Chuang"  writes:

[snip]

> @@ -655,6 +672,34 @@ void mt_put_memory_types(struct list_head *memory_types)
>  }
>  EXPORT_SYMBOL_GPL(mt_put_memory_types);
>  
> +/*
> + * This is invoked via `late_initcall()` to initialize memory tiers for
> + * CPU-less memory nodes after driver initialization, which is
> + * expected to provide `adistance` algorithms.
> + */
> +static int __init memory_tier_late_init(void)
> +{
> + int nid;
> +
> + mutex_lock(_tier_lock);
> + for_each_node_state(nid, N_MEMORY)
> + if (!node_state(nid, N_CPU) &&
> + node_memory_types[nid].memtype == NULL)

Think about this again.  It seems that it is better to check
"node_memory_types[nid].memtype == NULL" only here.  Because for all
node with N_CPU in memory_tier_init(), "node_memory_types[nid].memtype"
will be !NULL.  And it's possible (in theory) that some nodes becomes
"node_state(nid, N_CPU) == true" between memory_tier_init() and
memory_tier_late_init().

Otherwise, Looks good to me.  Feel free to add

Reviewed-by: "Huang, Ying" 

in the future version.

> + /*
> +  * Some device drivers may have initialized memory tiers
> +  * between `memory_tier_init()` and 
> `memory_tier_late_init()`,
> +  * potentially bringing online memory nodes and
> +  * configuring memory tiers. Exclude them here.
> +  */
> + set_node_memory_tier(nid);
> +
> + establish_demotion_targets();
> + mutex_unlock(_tier_lock);
> +
> + return 0;
> +}
> +late_initcall(memory_tier_late_init);
> +

[snip]

--
Best Regards,
Huang, Ying