| Message ID | 20241103032111.333282-9-kanchana.p.sridhar@intel.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | zswap IAA compress batching | expand |
Hi Kanchana,
kernel test robot noticed the following build warnings:
[auto build test WARNING on 5c4cf96cd70230100b5d396d45a5c9a332539d19]
url: https://github.com/intel-lab-lkp/linux/commits/Kanchana-P-Sridhar/crypto-acomp-Define-two-new-interfaces-for-compress-decompress-batching/20241103-112337
base: 5c4cf96cd70230100b5d396d45a5c9a332539d19
patch link: https://lore.kernel.org/r/20241103032111.333282-9-kanchana.p.sridhar%40intel.com
patch subject: [PATCH v2 08/13] mm: zswap: acomp_ctx mutex lock/unlock optimizations.
config: hexagon-allmodconfig (https://download.01.org/0day-ci/archive/20241103/202411031829.7KQDfJy1-lkp@intel.com/config)
compiler: clang version 20.0.0git (https://github.com/llvm/llvm-project 639a7ac648f1e50ccd2556e17d401c04f9cce625)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241103/202411031829.7KQDfJy1-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202411031829.7KQDfJy1-lkp@intel.com/
All warnings (new ones prefixed by >>):
In file included from mm/zswap.c:18:
In file included from include/linux/highmem.h:10:
In file included from include/linux/mm.h:2211:
include/linux/vmstat.h:518:36: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]
518 | return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_"
| ~~~~~~~~~~~ ^ ~~~
In file included from mm/zswap.c:18:
In file included from include/linux/highmem.h:12:
In file included from include/linux/hardirq.h:11:
In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
In file included from include/asm-generic/hardirq.h:17:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:14:
In file included from arch/hexagon/include/asm/io.h:328:
include/asm-generic/io.h:548:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
548 | val = __raw_readb(PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:561:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
561 | val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
| ~~~~~~~~~~ ^
include/uapi/linux/byteorder/little_endian.h:37:51: note: expanded from macro '__le16_to_cpu'
37 | #define __le16_to_cpu(x) ((__force __u16)(__le16)(x))
| ^
In file included from mm/zswap.c:18:
In file included from include/linux/highmem.h:12:
In file included from include/linux/hardirq.h:11:
In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
In file included from include/asm-generic/hardirq.h:17:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:14:
In file included from arch/hexagon/include/asm/io.h:328:
include/asm-generic/io.h:574:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
574 | val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
| ~~~~~~~~~~ ^
include/uapi/linux/byteorder/little_endian.h:35:51: note: expanded from macro '__le32_to_cpu'
35 | #define __le32_to_cpu(x) ((__force __u32)(__le32)(x))
| ^
In file included from mm/zswap.c:18:
In file included from include/linux/highmem.h:12:
In file included from include/linux/hardirq.h:11:
In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
In file included from include/asm-generic/hardirq.h:17:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:14:
In file included from arch/hexagon/include/asm/io.h:328:
include/asm-generic/io.h:585:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
585 | __raw_writeb(value, PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:595:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
595 | __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:605:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
605 | __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
In file included from mm/zswap.c:40:
In file included from mm/internal.h:13:
include/linux/mm_inline.h:47:41: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]
47 | __mod_lruvec_state(lruvec, NR_LRU_BASE + lru, nr_pages);
| ~~~~~~~~~~~ ^ ~~~
include/linux/mm_inline.h:49:22: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]
49 | NR_ZONE_LRU_BASE + lru, nr_pages);
| ~~~~~~~~~~~~~~~~ ^ ~~~
>> mm/zswap.c:1529:7: warning: variable 'acomp_ctx' is used uninitialized whenever 'if' condition is true [-Wsometimes-uninitialized]
1529 | if (memcg_list_lru_alloc(memcg, &zswap_list_lru, GFP_KERNEL)) {
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
mm/zswap.c:1560:16: note: uninitialized use occurs here
1560 | mutex_unlock(&acomp_ctx->mutex);
| ^~~~~~~~~
mm/zswap.c:1529:3: note: remove the 'if' if its condition is always false
1529 | if (memcg_list_lru_alloc(memcg, &zswap_list_lru, GFP_KERNEL)) {
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1530 | mem_cgroup_put(memcg);
| ~~~~~~~~~~~~~~~~~~~~~~
1531 | goto put_pool;
| ~~~~~~~~~~~~~~
1532 | }
| ~
mm/zswap.c:1496:36: note: initialize the variable 'acomp_ctx' to silence this warning
1496 | struct crypto_acomp_ctx *acomp_ctx;
| ^
| = NULL
10 warnings generated.
vim +1529 mm/zswap.c
ccacfbe8c6963d Kanchana P Sridhar 2024-09-30 1491
ccacfbe8c6963d Kanchana P Sridhar 2024-09-30 1492 bool zswap_store(struct folio *folio)
ccacfbe8c6963d Kanchana P Sridhar 2024-09-30 1493 {
ccacfbe8c6963d Kanchana P Sridhar 2024-09-30 1494 long nr_pages = folio_nr_pages(folio);
ccacfbe8c6963d Kanchana P Sridhar 2024-09-30 1495 swp_entry_t swp = folio->swap;
68dac3aa759e32 Kanchana P Sridhar 2024-11-02 1496 struct crypto_acomp_ctx *acomp_ctx;
ccacfbe8c6963d Kanchana P Sridhar 2024-09-30 1497 struct obj_cgroup *objcg = NULL;
ccacfbe8c6963d Kanchana P Sridhar 2024-09-30 1498 struct mem_cgroup *memcg = NULL;
ccacfbe8c6963d Kanchana P Sridhar 2024-09-30 1499 struct zswap_pool *pool;
ccacfbe8c6963d Kanchana P Sridhar 2024-09-30 1500 size_t compressed_bytes = 0;
ccacfbe8c6963d Kanchana P Sridhar 2024-09-30 1501 bool ret = false;
ccacfbe8c6963d Kanchana P Sridhar 2024-09-30 1502 long index;
ccacfbe8c6963d Kanchana P Sridhar 2024-09-30 1503
ccacfbe8c6963d Kanchana P Sridhar 2024-09-30 1504 VM_WARN_ON_ONCE(!folio_test_locked(folio));
ccacfbe8c6963d Kanchana P Sridhar 2024-09-30 1505 VM_WARN_ON_ONCE(!folio_test_swapcache(folio));
ccacfbe8c6963d Kanchana P Sridhar 2024-09-30 1506
ccacfbe8c6963d Kanchana P Sridhar 2024-09-30 1507 if (!zswap_enabled)
ccacfbe8c6963d Kanchana P Sridhar 2024-09-30 1508 goto check_old;
ccacfbe8c6963d Kanchana P Sridhar 2024-09-30 1509
ccacfbe8c6963d Kanchana P Sridhar 2024-09-30 1510 objcg = get_obj_cgroup_from_folio(folio);
ccacfbe8c6963d Kanchana P Sridhar 2024-09-30 1511 if (objcg && !obj_cgroup_may_zswap(objcg)) {
ccacfbe8c6963d Kanchana P Sridhar 2024-09-30 1512 memcg = get_mem_cgroup_from_objcg(objcg);
ccacfbe8c6963d Kanchana P Sridhar 2024-09-30 1513 if (shrink_memcg(memcg)) {
ccacfbe8c6963d Kanchana P Sridhar 2024-09-30 1514 mem_cgroup_put(memcg);
ccacfbe8c6963d Kanchana P Sridhar 2024-09-30 1515 goto put_objcg;
ccacfbe8c6963d Kanchana P Sridhar 2024-09-30 1516 }
ccacfbe8c6963d Kanchana P Sridhar 2024-09-30 1517 mem_cgroup_put(memcg);
ccacfbe8c6963d Kanchana P Sridhar 2024-09-30 1518 }
ccacfbe8c6963d Kanchana P Sridhar 2024-09-30 1519
ccacfbe8c6963d Kanchana P Sridhar 2024-09-30 1520 if (zswap_check_limits())
ccacfbe8c6963d Kanchana P Sridhar 2024-09-30 1521 goto put_objcg;
ccacfbe8c6963d Kanchana P Sridhar 2024-09-30 1522
ccacfbe8c6963d Kanchana P Sridhar 2024-09-30 1523 pool = zswap_pool_current_get();
ccacfbe8c6963d Kanchana P Sridhar 2024-09-30 1524 if (!pool)
ccacfbe8c6963d Kanchana P Sridhar 2024-09-30 1525 goto put_objcg;
ccacfbe8c6963d Kanchana P Sridhar 2024-09-30 1526
ccacfbe8c6963d Kanchana P Sridhar 2024-09-30 1527 if (objcg) {
ccacfbe8c6963d Kanchana P Sridhar 2024-09-30 1528 memcg = get_mem_cgroup_from_objcg(objcg);
ccacfbe8c6963d Kanchana P Sridhar 2024-09-30 @1529 if (memcg_list_lru_alloc(memcg, &zswap_list_lru, GFP_KERNEL)) {
ccacfbe8c6963d Kanchana P Sridhar 2024-09-30 1530 mem_cgroup_put(memcg);
ccacfbe8c6963d Kanchana P Sridhar 2024-09-30 1531 goto put_pool;
ccacfbe8c6963d Kanchana P Sridhar 2024-09-30 1532 }
ccacfbe8c6963d Kanchana P Sridhar 2024-09-30 1533 mem_cgroup_put(memcg);
ccacfbe8c6963d Kanchana P Sridhar 2024-09-30 1534 }
ccacfbe8c6963d Kanchana P Sridhar 2024-09-30 1535
68dac3aa759e32 Kanchana P Sridhar 2024-11-02 1536 acomp_ctx = raw_cpu_ptr(pool->acomp_ctx);
68dac3aa759e32 Kanchana P Sridhar 2024-11-02 1537 mutex_lock(&acomp_ctx->mutex);
68dac3aa759e32 Kanchana P Sridhar 2024-11-02 1538
ccacfbe8c6963d Kanchana P Sridhar 2024-09-30 1539 for (index = 0; index < nr_pages; ++index) {
ccacfbe8c6963d Kanchana P Sridhar 2024-09-30 1540 struct page *page = folio_page(folio, index);
ccacfbe8c6963d Kanchana P Sridhar 2024-09-30 1541 ssize_t bytes;
ccacfbe8c6963d Kanchana P Sridhar 2024-09-30 1542
ccacfbe8c6963d Kanchana P Sridhar 2024-09-30 1543 bytes = zswap_store_page(page, objcg, pool);
ccacfbe8c6963d Kanchana P Sridhar 2024-09-30 1544 if (bytes < 0)
ccacfbe8c6963d Kanchana P Sridhar 2024-09-30 1545 goto put_pool;
ccacfbe8c6963d Kanchana P Sridhar 2024-09-30 1546 compressed_bytes += bytes;
ccacfbe8c6963d Kanchana P Sridhar 2024-09-30 1547 }
ccacfbe8c6963d Kanchana P Sridhar 2024-09-30 1548
ccacfbe8c6963d Kanchana P Sridhar 2024-09-30 1549 if (objcg) {
ccacfbe8c6963d Kanchana P Sridhar 2024-09-30 1550 obj_cgroup_charge_zswap(objcg, compressed_bytes);
ccacfbe8c6963d Kanchana P Sridhar 2024-09-30 1551 count_objcg_events(objcg, ZSWPOUT, nr_pages);
ccacfbe8c6963d Kanchana P Sridhar 2024-09-30 1552 }
ccacfbe8c6963d Kanchana P Sridhar 2024-09-30 1553
ccacfbe8c6963d Kanchana P Sridhar 2024-09-30 1554 atomic_long_add(nr_pages, &zswap_stored_pages);
ccacfbe8c6963d Kanchana P Sridhar 2024-09-30 1555 count_vm_events(ZSWPOUT, nr_pages);
ccacfbe8c6963d Kanchana P Sridhar 2024-09-30 1556
ccacfbe8c6963d Kanchana P Sridhar 2024-09-30 1557 ret = true;
ccacfbe8c6963d Kanchana P Sridhar 2024-09-30 1558
ccacfbe8c6963d Kanchana P Sridhar 2024-09-30 1559 put_pool:
68dac3aa759e32 Kanchana P Sridhar 2024-11-02 1560 mutex_unlock(&acomp_ctx->mutex);
ccacfbe8c6963d Kanchana P Sridhar 2024-09-30 1561 zswap_pool_put(pool);
ccacfbe8c6963d Kanchana P Sridhar 2024-09-30 1562 put_objcg:
f4840ccfca25db Johannes Weiner 2022-05-19 1563 obj_cgroup_put(objcg);
ccacfbe8c6963d Kanchana P Sridhar 2024-09-30 1564 if (!ret && zswap_pool_reached_full)
4ea3fa9dd2e9c5 Yosry Ahmed 2024-04-13 1565 queue_work(shrink_wq, &zswap_shrink_work);
f576a1e80c3a97 Chengming Zhou 2024-02-09 1566 check_old:
f576a1e80c3a97 Chengming Zhou 2024-02-09 1567 /*
ccacfbe8c6963d Kanchana P Sridhar 2024-09-30 1568 * If the zswap store fails or zswap is disabled, we must invalidate
ccacfbe8c6963d Kanchana P Sridhar 2024-09-30 1569 * the possibly stale entries which were previously stored at the
ccacfbe8c6963d Kanchana P Sridhar 2024-09-30 1570 * offsets corresponding to each page of the folio. Otherwise,
ccacfbe8c6963d Kanchana P Sridhar 2024-09-30 1571 * writeback could overwrite the new data in the swapfile.
f576a1e80c3a97 Chengming Zhou 2024-02-09 1572 */
ccacfbe8c6963d Kanchana P Sridhar 2024-09-30 1573 if (!ret) {
ccacfbe8c6963d Kanchana P Sridhar 2024-09-30 1574 unsigned type = swp_type(swp);
ccacfbe8c6963d Kanchana P Sridhar 2024-09-30 1575 pgoff_t offset = swp_offset(swp);
ccacfbe8c6963d Kanchana P Sridhar 2024-09-30 1576 struct zswap_entry *entry;
ccacfbe8c6963d Kanchana P Sridhar 2024-09-30 1577 struct xarray *tree;
ccacfbe8c6963d Kanchana P Sridhar 2024-09-30 1578
ccacfbe8c6963d Kanchana P Sridhar 2024-09-30 1579 for (index = 0; index < nr_pages; ++index) {
ccacfbe8c6963d Kanchana P Sridhar 2024-09-30 1580 tree = swap_zswap_tree(swp_entry(type, offset + index));
ccacfbe8c6963d Kanchana P Sridhar 2024-09-30 1581 entry = xa_erase(tree, offset + index);
f576a1e80c3a97 Chengming Zhou 2024-02-09 1582 if (entry)
796c2c23e14e75 Chris Li 2024-03-26 1583 zswap_entry_free(entry);
ccacfbe8c6963d Kanchana P Sridhar 2024-09-30 1584 }
ccacfbe8c6963d Kanchana P Sridhar 2024-09-30 1585 }
ccacfbe8c6963d Kanchana P Sridhar 2024-09-30 1586
ccacfbe8c6963d Kanchana P Sridhar 2024-09-30 1587 return ret;
2b2811178e8555 Seth Jennings 2013-07-10 1588 }
2b2811178e8555 Seth Jennings 2013-07-10 1589
diff --git a/mm/zswap.c b/mm/zswap.c index f6316b66fb23..3e899fa61445 100644 --- a/mm/zswap.c +++ b/mm/zswap.c @@ -880,6 +880,9 @@ static int zswap_cpu_comp_dead(unsigned int cpu, struct hlist_node *node) return 0; } +/* + * The acomp_ctx->mutex must be locked/unlocked in the calling procedure. + */ static bool zswap_compress(struct page *page, struct zswap_entry *entry, struct zswap_pool *pool) { @@ -895,8 +898,6 @@ static bool zswap_compress(struct page *page, struct zswap_entry *entry, acomp_ctx = raw_cpu_ptr(pool->acomp_ctx); - mutex_lock(&acomp_ctx->mutex); - dst = acomp_ctx->buffer; sg_init_table(&input, 1); sg_set_page(&input, page, PAGE_SIZE, 0); @@ -949,7 +950,6 @@ static bool zswap_compress(struct page *page, struct zswap_entry *entry, else if (alloc_ret) zswap_reject_alloc_fail++; - mutex_unlock(&acomp_ctx->mutex); return comp_ret == 0 && alloc_ret == 0; } @@ -986,10 +986,16 @@ static void zswap_decompress(struct zswap_entry *entry, struct folio *folio) acomp_request_set_params(acomp_ctx->req, &input, &output, entry->length, PAGE_SIZE); BUG_ON(crypto_wait_req(crypto_acomp_decompress(acomp_ctx->req), &acomp_ctx->wait)); BUG_ON(acomp_ctx->req->dlen != PAGE_SIZE); - mutex_unlock(&acomp_ctx->mutex); if (src != acomp_ctx->buffer) zpool_unmap_handle(zpool, entry->handle); + + /* + * It is safer to unlock the mutex after the check for + * "src != acomp_ctx->buffer" so that the value of "src" + * does not change. + */ + mutex_unlock(&acomp_ctx->mutex); } /********************************* @@ -1487,6 +1493,7 @@ bool zswap_store(struct folio *folio) { long nr_pages = folio_nr_pages(folio); swp_entry_t swp = folio->swap; + struct crypto_acomp_ctx *acomp_ctx; struct obj_cgroup *objcg = NULL; struct mem_cgroup *memcg = NULL; struct zswap_pool *pool; @@ -1526,6 +1533,9 @@ bool zswap_store(struct folio *folio) mem_cgroup_put(memcg); } + acomp_ctx = raw_cpu_ptr(pool->acomp_ctx); + mutex_lock(&acomp_ctx->mutex); + for (index = 0; index < nr_pages; ++index) { struct page *page = folio_page(folio, index); ssize_t bytes; @@ -1547,6 +1557,7 @@ bool zswap_store(struct folio *folio) ret = true; put_pool: + mutex_unlock(&acomp_ctx->mutex); zswap_pool_put(pool); put_objcg: obj_cgroup_put(objcg);
This patch implements two changes with respect to the acomp_ctx mutex lock: 1) The mutex lock is not acquired/released in zswap_compress(). Instead, zswap_store() acquires the mutex lock once before compressing each page in a large folio, and releases the lock once all pages in the folio have been compressed. This should reduce some compute cycles in case of large folio stores. 2) In zswap_decompress(), the mutex lock is released after the conditional zpool_unmap_handle() based on "src != acomp_ctx->buffer" rather than before. This ensures that the value of "src" obtained earlier does not change. If the mutex lock is released before the comparison of "src" it is possible that another call to reclaim by the same process could obtain the mutex lock and over-write the value of "src". Signed-off-by: Kanchana P Sridhar <kanchana.p.sridhar@intel.com> --- mm/zswap.c | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-)