On Tue, Jun 7, 2022 at 11:43 AM Tim Chen <tim.c.chen@xxxxxxxxxxxxxxx> wrote: > > On Fri, 2022-06-03 at 19:12 +0530, Aneesh Kumar K.V wrote: > > > > > > The nodes which are part of a specific memory tier can be listed > > via > > /sys/devices/system/memtier/memtierN/nodelist > > > > "Rank" is an opaque value. Its absolute value doesn't have any > > special meaning. But the rank values of different memtiers can be > > compared with each other to determine the memory tier order. > > > > For example, if we have 3 memtiers: memtier0, memtier1, memiter2, and > > their rank values are 300, 200, 100, then the memory tier order is: > > memtier0 -> memtier2 -> memtier1, > > Why is memtier2 (rank 100) higher than memtier1 (rank 200)? Seems like > the order should be memtier0 -> memtier1 -> memtier2? > (rank 300) (rank 200) (rank 100) I think this is a copy-and-modify typo from my original memory tiering kernel interface RFC (v4, https://lore.kernel.org/linux-mm/CAAPL-u9Wv+nH1VOZTj=9p9S70Y3Qz3+63EkqncRDdHfubsrjfw@xxxxxxxxxxxxxx/T/): where the rank values are 100, 10, 50 (i.e the rank of memtier2 is higher than memtier1). > > where memtier0 is the highest tier > > and memtier1 is the lowest tier. > > I think memtier2 is the lowest as it has the lowest rank value. > > > > The rank value of each memtier should be unique. > > > > > > + > > +static void memory_tier_device_release(struct device *dev) > > +{ > > + struct memory_tier *tier = to_memory_tier(dev); > > + > > Do we need some ref counts on memory_tier? > If there is another device still using the same memtier, > free below could cause problem. > > > + kfree(tier); > > +} > > + > > > ... > > +static struct memory_tier *register_memory_tier(unsigned int tier) > > +{ > > + int error; > > + struct memory_tier *memtier; > > + > > + if (tier >= MAX_MEMORY_TIERS) > > + return NULL; > > + > > + memtier = kzalloc(sizeof(struct memory_tier), GFP_KERNEL); > > + if (!memtier) > > + return NULL; > > + > > + memtier->dev.id = tier; > > + memtier->rank = get_rank_from_tier(tier); > > + memtier->dev.bus = &memory_tier_subsys; > > + memtier->dev.release = memory_tier_device_release; > > + memtier->dev.groups = memory_tier_dev_groups; > > + > > Should you take the mem_tier_lock before you insert to > memtier-list? > > > + insert_memory_tier(memtier); > > + > > + error = device_register(&memtier->dev); > > + if (error) { > > + list_del(&memtier->list); > > + put_device(&memtier->dev); > > + return NULL; > > + } > > + return memtier; > > +} > > + > > +__maybe_unused // temporay to prevent warnings during bisects > > +static void unregister_memory_tier(struct memory_tier *memtier) > > +{ > > I think we should take mem_tier_lock before modifying memtier->list. > > > + list_del(&memtier->list); > > + device_unregister(&memtier->dev); > > +} > > + > > > > Thanks. > > Tim > >
