lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <9e7b7ded-62ef-4a7c-992a-dda0e540cff8@arm.com>
Date: Fri, 9 Aug 2024 08:46:57 +0100
From: Ryan Roberts <ryan.roberts@....com>
To: Barry Song <baohua@...nel.org>
Cc: Andrew Morton <akpm@...ux-foundation.org>, Hugh Dickins
 <hughd@...gle.com>, "Matthew Wilcox (Oracle)" <willy@...radead.org>,
 David Hildenbrand <david@...hat.com>, Lance Yang <ioworker0@...il.com>,
 Baolin Wang <baolin.wang@...ux.alibaba.com>, Gavin Shan <gshan@...hat.com>,
 linux-kernel@...r.kernel.org, linux-mm@...ck.org
Subject: Re: [PATCH v3 2/2] mm: Tidy up shmem mTHP controls and stats

On 08/08/2024 23:11, Barry Song wrote:
> On Thu, Aug 8, 2024 at 11:19 PM Ryan Roberts <ryan.roberts@....com> wrote:
>>
>> Previously we had a situation where shmem mTHP controls and stats were
>> not exposed for some supported sizes and were exposed for some
>> unsupported sizes. So let's clean that up.
>>
>> Anon mTHP can support all large orders [2, PMD_ORDER]. But shmem can
>> support all large orders [1, MAX_PAGECACHE_ORDER]. However, per-size
>> shmem controls and stats were previously being exposed for all the anon
>> mTHP orders, meaning order-1 was not present, and for arm64 64K base
>> pages, orders 12 and 13 were exposed but were not supported internally.
>>
>> Tidy this all up by defining ctrl and stats attribute groups for anon
>> and file separately. Anon ctrl and stats groups are populated for all
>> orders in THP_ORDERS_ALL_ANON and file ctrl and stats groups are
>> populated for all orders in THP_ORDERS_ALL_FILE_DEFAULT.
>>
>> Additionally, create "any" ctrl and stats attribute groups which are
>> populated for all orders in (THP_ORDERS_ALL_ANON |
>> THP_ORDERS_ALL_FILE_DEFAULT). swpout stats use this since they apply to
>> anon and shmem.
> 
> This passage is a bit confusing to me. I'm not sure whether it's about
> creating any control and stats for both file and anon, or creating them
> separately depending on the situation. However, the previous passage
> mentions that file and anon create control and stats separately based
> on different orders, and the orders they create might differ.

Yes, I'll admit that I added this paragraph for the change I made since the last
version, and it's not the clearest thing I've ever written on reflection.

The idea is that there are 3 buckets; "anon", "file" and "any". controls/stats
in the "anon" bucket are populated for all orders supported by anon memory. Same
for file. controls/stats in the "any" bucket are populated for all orders
supported by either anon or file.

> 
> But after running your patches, I understood it. For instance, I'm now
> seeing 8kB folder that didn't exist before:
> 
>            /sys/kernel/mm/transparent_hugepage # ls -l
>            drwxr-xr-x 3 root root    0 Aug  8 22:01 hugepages-1024kB
>             ...
>            drwxr-xr-x 3 root root    0 Aug  8 22:01 hugepages-8kB
>            drwxr-xr-x 2 root root    0 Aug  8 22:01 khugepaged
>            ...
> 
> Then, when I entered the 8kB folder, I found 'shmem_enabled'
> but not 'enabled' for anon:
>             /sys/kernel/mm/transparent_hugepage/hugepages-8kB # ls
>             shmem_enabled  stats
> 
> However, in the 16kB folder, I found both:
>             /sys/kernel/mm/transparent_hugepage/hugepages-16kB # ls
>             enabled  shmem_enabled stats
> 
> In the 8kB 'stats,' I see 'shmem_alloc' but not 'anon_alloc.' Since
> both shmem and anon require swapout, I am seeing 'swpout' and
> 'swpout_fallback':
> 
>             /sys/kernel/mm/transparent_hugepage/hugepages-8kB/stats # ls
>             shmem_alloc  shmem_fallback  shmem_fallback_charge  swpout
>  swpout_fallback
> 
> Everything observed seems to meet expectations, so:

Yes, what you observe is exactly as intended. It gets a bit tricky if
CONFIG_SHMEM is disabled; In this case, some ifdeffery ensures that the swpout
stats are declared in the "anon" bucket instead of the "any" bucket.

> 
> Tested-by: Barry Song <baohua@...nel.org>

Thanks!

> 
>>
>> The side-effect of all this is that different hugepage-*kB directories
>> contain different sets of controls and stats, depending on which memory
>> types support that size. This approach is preferred over the
>> alternative, which is to populate dummy controls and stats for memory
>> types that do not support a given size.
>>
>> Signed-off-by: Ryan Roberts <ryan.roberts@....com>
>> ---
>>  mm/huge_memory.c | 144 +++++++++++++++++++++++++++++++++++++----------
>>  1 file changed, 114 insertions(+), 30 deletions(-)
>>
>> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
>> index 0c3075ee00012..082d86b7c6c2f 100644
>> --- a/mm/huge_memory.c
>> +++ b/mm/huge_memory.c
>> @@ -482,8 +482,8 @@ static void thpsize_release(struct kobject *kobj);
>>  static DEFINE_SPINLOCK(huge_anon_orders_lock);
>>  static LIST_HEAD(thpsize_list);
>>
>> -static ssize_t thpsize_enabled_show(struct kobject *kobj,
>> -                                   struct kobj_attribute *attr, char *buf)
>> +static ssize_t anon_enabled_show(struct kobject *kobj,
>> +                                struct kobj_attribute *attr, char *buf)
>>  {
>>         int order = to_thpsize(kobj)->order;
>>         const char *output;
>> @@ -500,9 +500,9 @@ static ssize_t thpsize_enabled_show(struct kobject *kobj,
>>         return sysfs_emit(buf, "%s\n", output);
>>  }
>>
>> -static ssize_t thpsize_enabled_store(struct kobject *kobj,
>> -                                    struct kobj_attribute *attr,
>> -                                    const char *buf, size_t count)
>> +static ssize_t anon_enabled_store(struct kobject *kobj,
>> +                                 struct kobj_attribute *attr,
>> +                                 const char *buf, size_t count)
>>  {
>>         int order = to_thpsize(kobj)->order;
>>         ssize_t ret = count;
>> @@ -544,19 +544,35 @@ static ssize_t thpsize_enabled_store(struct kobject *kobj,
>>         return ret;
>>  }
>>
>> -static struct kobj_attribute thpsize_enabled_attr =
>> -       __ATTR(enabled, 0644, thpsize_enabled_show, thpsize_enabled_store);
>> +static struct kobj_attribute anon_enabled_attr =
>> +       __ATTR(enabled, 0644, anon_enabled_show, anon_enabled_store);
>>
>> -static struct attribute *thpsize_attrs[] = {
>> -       &thpsize_enabled_attr.attr,
>> +static struct attribute *anon_ctrl_attrs[] = {
>> +       &anon_enabled_attr.attr,
>> +       NULL,
>> +};
>> +
>> +static const struct attribute_group anon_ctrl_attr_grp = {
>> +       .attrs = anon_ctrl_attrs,
>> +};
>> +
>> +static struct attribute *file_ctrl_attrs[] = {
>>  #ifdef CONFIG_SHMEM
>>         &thpsize_shmem_enabled_attr.attr,
>>  #endif
>>         NULL,
>>  };
>>
>> -static const struct attribute_group thpsize_attr_group = {
>> -       .attrs = thpsize_attrs,
>> +static const struct attribute_group file_ctrl_attr_grp = {
>> +       .attrs = file_ctrl_attrs,
>> +};
>> +
>> +static struct attribute *any_ctrl_attrs[] = {
>> +       NULL,
>> +};
>> +
>> +static const struct attribute_group any_ctrl_attr_grp = {
>> +       .attrs = any_ctrl_attrs,
>>  };
>>
>>  static const struct kobj_type thpsize_ktype = {
>> @@ -595,64 +611,132 @@ DEFINE_MTHP_STAT_ATTR(anon_fault_fallback, MTHP_STAT_ANON_FAULT_FALLBACK);
>>  DEFINE_MTHP_STAT_ATTR(anon_fault_fallback_charge, MTHP_STAT_ANON_FAULT_FALLBACK_CHARGE);
>>  DEFINE_MTHP_STAT_ATTR(swpout, MTHP_STAT_SWPOUT);
>>  DEFINE_MTHP_STAT_ATTR(swpout_fallback, MTHP_STAT_SWPOUT_FALLBACK);
>> +#ifdef CONFIG_SHMEM
>>  DEFINE_MTHP_STAT_ATTR(shmem_alloc, MTHP_STAT_SHMEM_ALLOC);
>>  DEFINE_MTHP_STAT_ATTR(shmem_fallback, MTHP_STAT_SHMEM_FALLBACK);
>>  DEFINE_MTHP_STAT_ATTR(shmem_fallback_charge, MTHP_STAT_SHMEM_FALLBACK_CHARGE);
>> +#endif
>>  DEFINE_MTHP_STAT_ATTR(split, MTHP_STAT_SPLIT);
>>  DEFINE_MTHP_STAT_ATTR(split_failed, MTHP_STAT_SPLIT_FAILED);
>>  DEFINE_MTHP_STAT_ATTR(split_deferred, MTHP_STAT_SPLIT_DEFERRED);
>>
>> -static struct attribute *stats_attrs[] = {
>> +static struct attribute *anon_stats_attrs[] = {
>>         &anon_fault_alloc_attr.attr,
>>         &anon_fault_fallback_attr.attr,
>>         &anon_fault_fallback_charge_attr.attr,
>> +#ifndef CONFIG_SHMEM
>>         &swpout_attr.attr,
>>         &swpout_fallback_attr.attr,
>> -       &shmem_alloc_attr.attr,
>> -       &shmem_fallback_attr.attr,
>> -       &shmem_fallback_charge_attr.attr,
>> +#endif
>>         &split_attr.attr,
>>         &split_failed_attr.attr,
>>         &split_deferred_attr.attr,
>>         NULL,
>>  };
>>
>> -static struct attribute_group stats_attr_group = {
>> +static struct attribute_group anon_stats_attr_grp = {
>> +       .name = "stats",
>> +       .attrs = anon_stats_attrs,
>> +};
>> +
>> +static struct attribute *file_stats_attrs[] = {
>> +#ifdef CONFIG_SHMEM
>> +       &shmem_alloc_attr.attr,
>> +       &shmem_fallback_attr.attr,
>> +       &shmem_fallback_charge_attr.attr,
>> +#endif
>> +       NULL,
>> +};
>> +
>> +static struct attribute_group file_stats_attr_grp = {
>> +       .name = "stats",
>> +       .attrs = file_stats_attrs,
>> +};
>> +
>> +static struct attribute *any_stats_attrs[] = {
>> +#ifdef CONFIG_SHMEM
>> +       &swpout_attr.attr,
>> +       &swpout_fallback_attr.attr,
>> +#endif
>> +       NULL,
>> +};
>> +
>> +static struct attribute_group any_stats_attr_grp = {
>>         .name = "stats",
>> -       .attrs = stats_attrs,
>> +       .attrs = any_stats_attrs,
>>  };
>>
>> +static int sysfs_add_group(struct kobject *kobj,
>> +                          const struct attribute_group *grp)
>> +{
>> +       int ret = -ENOENT;
>> +
>> +       /*
>> +        * If the group is named, try to merge first, assuming the subdirectory
>> +        * was already created. This avoids the warning emitted by
>> +        * sysfs_create_group() if the directory already exists.
>> +        */
>> +       if (grp->name)
>> +               ret = sysfs_merge_group(kobj, grp);
>> +       if (ret)
>> +               ret = sysfs_create_group(kobj, grp);
>> +
>> +       return ret;
>> +}
>> +
>>  static struct thpsize *thpsize_create(int order, struct kobject *parent)
>>  {
>>         unsigned long size = (PAGE_SIZE << order) / SZ_1K;
>>         struct thpsize *thpsize;
>> -       int ret;
>> +       int ret = -ENOMEM;
>>
>>         thpsize = kzalloc(sizeof(*thpsize), GFP_KERNEL);
>>         if (!thpsize)
>> -               return ERR_PTR(-ENOMEM);
>> +               goto err;
>> +
>> +       thpsize->order = order;
>>
>>         ret = kobject_init_and_add(&thpsize->kobj, &thpsize_ktype, parent,
>>                                    "hugepages-%lukB", size);
>>         if (ret) {
>>                 kfree(thpsize);
>> -               return ERR_PTR(ret);
>> +               goto err;
>>         }
>>
>> -       ret = sysfs_create_group(&thpsize->kobj, &thpsize_attr_group);
>> -       if (ret) {
>> -               kobject_put(&thpsize->kobj);
>> -               return ERR_PTR(ret);
>> +
>> +       ret = sysfs_add_group(&thpsize->kobj, &any_ctrl_attr_grp);
>> +       if (ret)
>> +               goto err_put;
>> +
>> +       ret = sysfs_add_group(&thpsize->kobj, &any_stats_attr_grp);
>> +       if (ret)
>> +               goto err_put;
>> +
>> +       if (BIT(order) & THP_ORDERS_ALL_ANON) {
>> +               ret = sysfs_add_group(&thpsize->kobj, &anon_ctrl_attr_grp);
>> +               if (ret)
>> +                       goto err_put;
>> +
>> +               ret = sysfs_add_group(&thpsize->kobj, &anon_stats_attr_grp);
>> +               if (ret)
>> +                       goto err_put;
>>         }
>>
>> -       ret = sysfs_create_group(&thpsize->kobj, &stats_attr_group);
>> -       if (ret) {
>> -               kobject_put(&thpsize->kobj);
>> -               return ERR_PTR(ret);
>> +       if (BIT(order) & THP_ORDERS_ALL_FILE_DEFAULT) {
>> +               ret = sysfs_add_group(&thpsize->kobj, &file_ctrl_attr_grp);
>> +               if (ret)
>> +                       goto err_put;
>> +
>> +               ret = sysfs_add_group(&thpsize->kobj, &file_stats_attr_grp);
>> +               if (ret)
>> +                       goto err_put;
>>         }
>>
>> -       thpsize->order = order;
>>         return thpsize;
>> +err_put:
>> +       kobject_put(&thpsize->kobj);
>> +err:
>> +       return ERR_PTR(ret);
>>  }
>>
>>  static void thpsize_release(struct kobject *kobj)
>> @@ -692,7 +776,7 @@ static int __init hugepage_init_sysfs(struct kobject **hugepage_kobj)
>>                 goto remove_hp_group;
>>         }
>>
>> -       orders = THP_ORDERS_ALL_ANON;
>> +       orders = THP_ORDERS_ALL_ANON | THP_ORDERS_ALL_FILE_DEFAULT;
>>         order = highest_order(orders);
>>         while (orders) {
>>                 thpsize = thpsize_create(order, *hugepage_kobj);
>> --
>> 2.43.0
>>
> 
> Thanks
> Barry


Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ