| 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
| ||
|
Message-ID: <CAMgjq7BJE9ALFG4N8wb-hdkC+b-8d1+ckXL9D6pbbfgiXfuzPA@mail.gmail.com>
Date: Thu, 12 Jun 2025 19:14:20 +0800
From: Kairui Song <ryncsn@...il.com>
To: youngjun.park@....com
Cc: linux-mm@...ck.org, akpm@...ux-foundation.org, hannes@...xchg.org,
mhocko@...nel.org, roman.gushchin@...ux.dev, shakeel.butt@...ux.dev,
cgroups@...r.kernel.org, linux-kernel@...r.kernel.org,
shikemeng@...weicloud.com, nphamcs@...il.com, bhe@...hat.com,
baohua@...nel.org, chrisl@...nel.org, muchun.song@...ux.dev,
iamjoonsoo.kim@....com, taejoon.song@....com, gunho.lee@....com
Subject: Re: [RFC PATCH 2/2] mm: swap: apply per cgroup swap priority
mechansim on swap layer
On Thu, Jun 12, 2025 at 6:43 PM <youngjun.park@....com> wrote:
>
> From: "youngjun.park" <youngjun.park@....com>
>
Hi, Youngjun,
Thanks for sharing this series.
> This patch implements swap device selection and swap on/off propagation
> when a cgroup-specific swap priority is set.
>
> There is one workaround to this implementation as follows.
> Current per-cpu swap cluster enforces swap device selection based solely
> on CPU locality, overriding the swap cgroup's configured priorities.
I've been thinking about this, we can switch to a per-cgroup-per-cpu
next cluster selector, the problem with current code is that swap
allocator is not designed with folio / cgroup in mind at all, so it's
really ugly to implement, which is why I have following two patches in
the swap table series:
https://lore.kernel.org/linux-mm/20250514201729.48420-18-ryncsn@gmail.com/
https://lore.kernel.org/linux-mm/20250514201729.48420-22-ryncsn@gmail.com/
The first one makes all swap allocation starts with a folio, the
second one makes the allocator always folio aware. So you can know
which cgroup is doing the allocation at anytime inside the allocator
(and it reduced the number of argument, also improving performance :)
)
So the allocator can just use cgroup's swap info if available, plist,
percpu cluster, and fallback to global locality in a very natural way.
> Therefore, when a swap cgroup priority is assigned, we fall back to
> using per-CPU clusters per swap device, similar to the previous behavior.
>
> A proper fix for this workaround will be evaluated in the next patch.
Hmm, but this is already the last patch in the series?
>
> Signed-off-by: Youngjun park <youngjun.park@....com>
> ---
> include/linux/swap.h | 8 +++
> mm/swap.h | 8 +++
> mm/swap_cgroup_priority.c | 133 ++++++++++++++++++++++++++++++++++++++
> mm/swapfile.c | 125 ++++++++++++++++++++++++-----------
> 4 files changed, 238 insertions(+), 36 deletions(-)
>
> diff --git a/include/linux/swap.h b/include/linux/swap.h
> index 49b73911c1bd..d158b0d5c997 100644
> --- a/include/linux/swap.h
> +++ b/include/linux/swap.h
> @@ -283,6 +283,13 @@ enum swap_cluster_flags {
> #define SWAP_NR_ORDERS 1
> #endif
>
> +#ifdef CONFIG_SWAP_CGROUP_PRIORITY
> +struct percpu_cluster {
> + local_lock_t lock; /* Protect the percpu_cluster above */
> + unsigned int next[SWAP_NR_ORDERS]; /* Likely next allocation offset */
> +};
> +#endif
> +
> /*
> * We keep using same cluster for rotational device so IO will be sequential.
> * The purpose is to optimize SWAP throughput on these device.
> @@ -341,6 +348,7 @@ struct swap_info_struct {
> struct list_head discard_clusters; /* discard clusters list */
> #ifdef CONFIG_SWAP_CGROUP_PRIORITY
> int unique_id;
> + struct percpu_cluster __percpu *percpu_cluster; /* per cpu's swap location */
> #endif
> struct plist_node avail_lists[]; /*
> * entries in swap_avail_heads, one
> diff --git a/mm/swap.h b/mm/swap.h
> index cd2649c632ed..cb6d653fe3f1 100644
> --- a/mm/swap.h
> +++ b/mm/swap.h
> @@ -113,7 +113,15 @@ void delete_swap_cgroup_priority(struct mem_cgroup *memcg);
> void show_swap_device_unique_id(struct seq_file *m);
> #else
> static inline void delete_swap_cgroup_priority(struct mem_cgroup *memcg) {}
> +static inline void activate_swap_cgroup_priority_pnode(struct swap_info_struct *swp, bool swapon) {}
> +static inline void deactivate_swap_cgroup_priority_pnode(struct swap_info_struct *swp, bool swapoff){}
> static inline void get_swap_unique_id(struct swap_info_struct *si) {}
> +static inline bool swap_alloc_cgroup_priority(struct mem_cgroup *memcg,
> + swp_entry_t *entry, int order)
> +{
> + return false;
> +}
> +
> #endif
>
> #else /* CONFIG_SWAP */
> diff --git a/mm/swap_cgroup_priority.c b/mm/swap_cgroup_priority.c
> index b3e20b676680..bb18cb251f60 100644
> --- a/mm/swap_cgroup_priority.c
> +++ b/mm/swap_cgroup_priority.c
> @@ -54,6 +54,132 @@ static void get_swap_unique_id(struct swap_info_struct *si)
> si->unique_id = atomic_add_return(1, &swap_unique_id_counter);
> }
>
> +static bool swap_alloc_cgroup_priority(struct mem_cgroup *memcg,
> + swp_entry_t *entry, int order)
> +{
> + struct swap_cgroup_priority *swap_priority;
> + struct swap_cgroup_priority_pnode *pnode, *next;
> + unsigned long offset;
> + int node;
> +
> + if (!memcg)
> + return false;
> +
> + spin_lock(&swap_avail_lock);
> +priority_check:
> + swap_priority = memcg->swap_priority;
> + if (!swap_priority) {
> + spin_unlock(&swap_avail_lock);
> + return false;
> + }
> +
> + node = numa_node_id();
> +start_over:
> + plist_for_each_entry_safe(pnode, next, &swap_priority->plist[node],
> + avail_lists[node]) {
> + struct swap_info_struct *si = pnode->swap;
> + plist_requeue(&pnode->avail_lists[node],
> + &swap_priority->plist[node]);
> + spin_unlock(&swap_avail_lock);
> +
> + if (get_swap_device_info(si)) {
> + offset = cluster_alloc_swap_entry(si,
> + order, SWAP_HAS_CACHE, true);
> + put_swap_device(si);
> + if (offset) {
> + *entry = swp_entry(si->type, offset);
> + return true;
> + }
> + if (order)
> + return false;
> + }
> +
> + spin_lock(&swap_avail_lock);
> +
> + /* swap_priority is remove or changed under us. */
> + if (swap_priority != memcg->swap_priority)
> + goto priority_check;
> +
> + if (plist_node_empty(&next->avail_lists[node]))
> + goto start_over;
> + }
> + spin_unlock(&swap_avail_lock);
> +
> + return false;
> +}
> +
> +/* add_to_avail_list (swapon / swapusage > 0) */
> +static void activate_swap_cgroup_priority_pnode(struct swap_info_struct *swp,
> + bool swapon)
> +{
> + struct swap_cgroup_priority *swap_priority;
> + int i;
> +
> + list_for_each_entry(swap_priority, &swap_cgroup_priority_list, link) {
> + struct swap_cgroup_priority_pnode *pnode
> + = swap_priority->pnode[swp->type];
> +
> + if (swapon) {
> + pnode->swap = swp;
> + pnode->prio = swp->prio;
> + }
> +
> + /* NUMA priority handling */
> + for_each_node(i) {
> + if (swapon) {
> + if (swap_node(swp) == i) {
> + plist_node_init(
> + &pnode->avail_lists[i],
> + 1);
> + } else {
> + plist_node_init(
> + &pnode->avail_lists[i],
> + -pnode->prio);
> + }
> + }
> +
> + plist_add(&pnode->avail_lists[i],
> + &swap_priority->plist[i]);
> + }
> + }
> +}
> +
> +/* del_from_avail_list (swapoff / swap usage <= 0) */
> +static void deactivate_swap_cgroup_priority_pnode(struct swap_info_struct *swp,
> + bool swapoff)
> +{
> + struct swap_cgroup_priority *swap_priority;
> + int nid, i;
> +
> + list_for_each_entry(swap_priority, &swap_cgroup_priority_list, link) {
> + struct swap_cgroup_priority_pnode *pnode;
> +
> + if (swapoff && swp->prio < 0) {
> + /*
> + * NUMA priority handling
> + * mimic swapoff prio adjustment without plist
> + */
> + for (int i = 0; i < MAX_SWAPFILES; i++) {
> + pnode = swap_priority->pnode[i];
> + if (pnode->prio > swp->prio ||
> + pnode->swap == swp)
> + continue;
> +
> + pnode->prio++;
> + for_each_node(nid) {
> + if (pnode->avail_lists[nid].prio != 1)
> + pnode->avail_lists[nid].prio--;
> + }
> + }
> + }
> +
> + pnode = swap_priority->pnode[swp->type];
> + for_each_node(i)
> + plist_del(&pnode->avail_lists[i],
> + &swap_priority->plist[i]);
> + }
> +}
> +
> int create_swap_cgroup_priority(struct mem_cgroup *memcg,
> int unique[], int prio[], int nr)
> {
> @@ -183,6 +309,12 @@ void delete_swap_cgroup_priority(struct mem_cgroup *memcg)
> {
> struct swap_cgroup_priority *swap_priority;
>
> + /*
> + * XXX: Possible RCU wait? No. Cannot protect priority list addition.
> + * swap_avail_lock gives protection.
> + * Think about other object protection mechanism
> + * might be solve it and better. (e.g object reference)
> + */
> spin_lock(&swap_avail_lock);
> swap_priority = memcg->swap_priority;
> if (!swap_priority) {
> @@ -198,5 +330,6 @@ void delete_swap_cgroup_priority(struct mem_cgroup *memcg)
>
> for (int i = 0; i < MAX_SWAPFILES; i++)
> kvfree(swap_priority->pnode[i]);
> +
> kvfree(swap_priority);
> }
> diff --git a/mm/swapfile.c b/mm/swapfile.c
> index f8e48dd2381e..28afe4ec0504 100644
> --- a/mm/swapfile.c
> +++ b/mm/swapfile.c
> @@ -126,8 +126,12 @@ static DEFINE_PER_CPU(struct percpu_swap_cluster, percpu_swap_cluster) = {
> .offset = { SWAP_ENTRY_INVALID },
> .lock = INIT_LOCAL_LOCK(),
> };
> -/* TODO: better choice? */
> +/* TODO: better arrangement */
> #ifdef CONFIG_SWAP_CGROUP_PRIORITY
> +static bool get_swap_device_info(struct swap_info_struct *si);
> +static unsigned long cluster_alloc_swap_entry(struct swap_info_struct *si, int order,
> + unsigned char usage, bool is_cgroup_priority);
> +static int swap_node(struct swap_info_struct *si);
> #include "swap_cgroup_priority.c"
> #endif
>
> @@ -776,7 +780,8 @@ static unsigned int alloc_swap_scan_cluster(struct swap_info_struct *si,
> struct swap_cluster_info *ci,
> unsigned long offset,
> unsigned int order,
> - unsigned char usage)
> + unsigned char usage,
> + bool is_cgroup_priority)
> {
> unsigned int next = SWAP_ENTRY_INVALID, found = SWAP_ENTRY_INVALID;
> unsigned long start = ALIGN_DOWN(offset, SWAPFILE_CLUSTER);
> @@ -820,12 +825,19 @@ static unsigned int alloc_swap_scan_cluster(struct swap_info_struct *si,
> out:
> relocate_cluster(si, ci);
> unlock_cluster(ci);
> +
> if (si->flags & SWP_SOLIDSTATE) {
> - this_cpu_write(percpu_swap_cluster.offset[order], next);
> - this_cpu_write(percpu_swap_cluster.si[order], si);
> - } else {
> + if (!is_cgroup_priority) {
> + this_cpu_write(percpu_swap_cluster.offset[order], next);
> + this_cpu_write(percpu_swap_cluster.si[order], si);
> + } else {
> +#ifdef CONFIG_SWAP_CGROUP_PRIORITY
> + __this_cpu_write(si->percpu_cluster->next[order], next);
> +#endif
> + }
> + } else
> si->global_cluster->next[order] = next;
> - }
> +
> return found;
> }
>
> @@ -883,7 +895,7 @@ static void swap_reclaim_work(struct work_struct *work)
> * cluster for current CPU too.
> */
> static unsigned long cluster_alloc_swap_entry(struct swap_info_struct *si, int order,
> - unsigned char usage)
> + unsigned char usage, bool is_cgroup_priority)
> {
> struct swap_cluster_info *ci;
> unsigned int offset = SWAP_ENTRY_INVALID, found = SWAP_ENTRY_INVALID;
> @@ -895,32 +907,38 @@ static unsigned long cluster_alloc_swap_entry(struct swap_info_struct *si, int o
> if (order && !(si->flags & SWP_BLKDEV))
> return 0;
>
> - if (!(si->flags & SWP_SOLIDSTATE)) {
> + if (si->flags & SWP_SOLIDSTATE) {
> +#ifdef CONFIG_SWAP_CGROUP_PRIORITY
> + local_lock(&si->percpu_cluster->lock);
> + offset = __this_cpu_read(si->percpu_cluster->next[order]);
> +#endif
> + } else {
> /* Serialize HDD SWAP allocation for each device. */
> spin_lock(&si->global_cluster_lock);
> offset = si->global_cluster->next[order];
> - if (offset == SWAP_ENTRY_INVALID)
> - goto new_cluster;
> + }
>
> - ci = lock_cluster(si, offset);
> - /* Cluster could have been used by another order */
> - if (cluster_is_usable(ci, order)) {
> - if (cluster_is_empty(ci))
> - offset = cluster_offset(si, ci);
> - found = alloc_swap_scan_cluster(si, ci, offset,
> - order, usage);
> - } else {
> - unlock_cluster(ci);
> - }
> - if (found)
> - goto done;
> + if (offset == SWAP_ENTRY_INVALID)
> + goto new_cluster;
> +
> + ci = lock_cluster(si, offset);
> + /* Cluster could have been used by another order */
> + if (cluster_is_usable(ci, order)) {
> + if (cluster_is_empty(ci))
> + offset = cluster_offset(si, ci);
> + found = alloc_swap_scan_cluster(si, ci, offset,
> + order, usage, is_cgroup_priority);
> + } else {
> + unlock_cluster(ci);
> }
> + if (found)
> + goto done;
>
> new_cluster:
> ci = isolate_lock_cluster(si, &si->free_clusters);
> if (ci) {
> found = alloc_swap_scan_cluster(si, ci, cluster_offset(si, ci),
> - order, usage);
> + order, usage, is_cgroup_priority);
> if (found)
> goto done;
> }
> @@ -934,7 +952,7 @@ static unsigned long cluster_alloc_swap_entry(struct swap_info_struct *si, int o
>
> while ((ci = isolate_lock_cluster(si, &si->nonfull_clusters[order]))) {
> found = alloc_swap_scan_cluster(si, ci, cluster_offset(si, ci),
> - order, usage);
> + order, usage, is_cgroup_priority);
> if (found)
> goto done;
> /* Clusters failed to allocate are moved to frag_clusters */
> @@ -952,7 +970,7 @@ static unsigned long cluster_alloc_swap_entry(struct swap_info_struct *si, int o
> * reclaimable (eg. lazy-freed swap cache) slots.
> */
> found = alloc_swap_scan_cluster(si, ci, cluster_offset(si, ci),
> - order, usage);
> + order, usage, is_cgroup_priority);
> if (found)
> goto done;
> frags++;
> @@ -979,21 +997,27 @@ static unsigned long cluster_alloc_swap_entry(struct swap_info_struct *si, int o
> while ((ci = isolate_lock_cluster(si, &si->frag_clusters[o]))) {
> atomic_long_dec(&si->frag_cluster_nr[o]);
> found = alloc_swap_scan_cluster(si, ci, cluster_offset(si, ci),
> - 0, usage);
> + 0, usage, is_cgroup_priority);
> if (found)
> goto done;
> }
>
> while ((ci = isolate_lock_cluster(si, &si->nonfull_clusters[o]))) {
> found = alloc_swap_scan_cluster(si, ci, cluster_offset(si, ci),
> - 0, usage);
> + 0, usage, is_cgroup_priority);
> if (found)
> goto done;
> }
> }
> done:
> - if (!(si->flags & SWP_SOLIDSTATE))
> + if (si->flags & SWP_SOLIDSTATE) {
> +#ifdef CONFIG_SWAP_CGROUP_PRIORITY
> + local_unlock(&si->percpu_cluster->lock);
> +#endif
> + } else {
> spin_unlock(&si->global_cluster_lock);
> + }
> +
> return found;
> }
>
> @@ -1032,6 +1056,7 @@ static void del_from_avail_list(struct swap_info_struct *si, bool swapoff)
> for_each_node(nid)
> plist_del(&si->avail_lists[nid], &swap_avail_heads[nid]);
>
> + deactivate_swap_cgroup_priority_pnode(si, swapoff);
> skip:
> spin_unlock(&swap_avail_lock);
> }
> @@ -1075,6 +1100,7 @@ static void add_to_avail_list(struct swap_info_struct *si, bool swapon)
> for_each_node(nid)
> plist_add(&si->avail_lists[nid], &swap_avail_heads[nid]);
>
> + activate_swap_cgroup_priority_pnode(si, swapon);
> skip:
> spin_unlock(&swap_avail_lock);
> }
> @@ -1200,7 +1226,8 @@ static bool swap_alloc_fast(swp_entry_t *entry,
> if (cluster_is_usable(ci, order)) {
> if (cluster_is_empty(ci))
> offset = cluster_offset(si, ci);
> - found = alloc_swap_scan_cluster(si, ci, offset, order, SWAP_HAS_CACHE);
> + found = alloc_swap_scan_cluster(si, ci, offset, order,
> + SWAP_HAS_CACHE, false);
> if (found)
> *entry = swp_entry(si->type, found);
> } else {
> @@ -1227,7 +1254,7 @@ static bool swap_alloc_slow(swp_entry_t *entry,
> plist_requeue(&si->avail_lists[node], &swap_avail_heads[node]);
> spin_unlock(&swap_avail_lock);
> if (get_swap_device_info(si)) {
> - offset = cluster_alloc_swap_entry(si, order, SWAP_HAS_CACHE);
> + offset = cluster_alloc_swap_entry(si, order, SWAP_HAS_CACHE, false);
> put_swap_device(si);
> if (offset) {
> *entry = swp_entry(si->type, offset);
> @@ -1294,10 +1321,12 @@ int folio_alloc_swap(struct folio *folio, gfp_t gfp)
> }
> }
>
> - local_lock(&percpu_swap_cluster.lock);
> - if (!swap_alloc_fast(&entry, order))
> - swap_alloc_slow(&entry, order);
> - local_unlock(&percpu_swap_cluster.lock);
> + if (!swap_alloc_cgroup_priority(folio_memcg(folio), &entry, order)) {
> + local_lock(&percpu_swap_cluster.lock);
> + if (!swap_alloc_fast(&entry, order))
> + swap_alloc_slow(&entry, order);
> + local_unlock(&percpu_swap_cluster.lock);
> + }
>
> /* Need to call this even if allocation failed, for MEMCG_SWAP_FAIL. */
> if (mem_cgroup_try_charge_swap(folio, entry))
> @@ -1870,7 +1899,7 @@ swp_entry_t get_swap_page_of_type(int type)
> /* This is called for allocating swap entry, not cache */
> if (get_swap_device_info(si)) {
> if (si->flags & SWP_WRITEOK) {
> - offset = cluster_alloc_swap_entry(si, 0, 1);
> + offset = cluster_alloc_swap_entry(si, 0, 1, false);
> if (offset) {
> entry = swp_entry(si->type, offset);
> atomic_long_dec(&nr_swap_pages);
> @@ -2800,6 +2829,10 @@ SYSCALL_DEFINE1(swapoff, const char __user *, specialfile)
> arch_swap_invalidate_area(p->type);
> zswap_swapoff(p->type);
> mutex_unlock(&swapon_mutex);
> +#ifdef CONFIG_SWAP_CGROUP_PRIORITY
> + free_percpu(p->percpu_cluster);
> + p->percpu_cluster = NULL;
> +#endif
> kfree(p->global_cluster);
> p->global_cluster = NULL;
> vfree(swap_map);
> @@ -3207,7 +3240,23 @@ static struct swap_cluster_info *setup_clusters(struct swap_info_struct *si,
> for (i = 0; i < nr_clusters; i++)
> spin_lock_init(&cluster_info[i].lock);
>
> - if (!(si->flags & SWP_SOLIDSTATE)) {
> + if (si->flags & SWP_SOLIDSTATE) {
> +#ifdef CONFIG_SWAP_CGROUP_PRIORITY
> + si->percpu_cluster = alloc_percpu(struct percpu_cluster);
> + if (!si->percpu_cluster)
> + goto err_free;
> +
> + int cpu;
> + for_each_possible_cpu(cpu) {
> + struct percpu_cluster *cluster;
> +
> + cluster = per_cpu_ptr(si->percpu_cluster, cpu);
> + for (i = 0; i < SWAP_NR_ORDERS; i++)
> + cluster->next[i] = SWAP_ENTRY_INVALID;
> + local_lock_init(&cluster->lock);
> + }
> +#endif
> + } else {
> si->global_cluster = kmalloc(sizeof(*si->global_cluster),
> GFP_KERNEL);
> if (!si->global_cluster)
> @@ -3495,6 +3544,10 @@ SYSCALL_DEFINE2(swapon, const char __user *, specialfile, int, swap_flags)
> bad_swap_unlock_inode:
> inode_unlock(inode);
> bad_swap:
> +#ifdef CONFIG_SWAP_CGROUP_PRIORITY
> + free_percpu(si->percpu_cluster);
> + si->percpu_cluster = NULL;
> +#endif
> kfree(si->global_cluster);
> si->global_cluster = NULL;
> inode = NULL;
> --
> 2.34.1
>
>
Powered by blists - more mailing lists