From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 5A0E5C433EF for ; Mon, 7 Mar 2022 15:08:11 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id E4A4E8D0007; Mon, 7 Mar 2022 10:08:10 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id DFA168D0001; Mon, 7 Mar 2022 10:08:10 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id C9C528D0007; Mon, 7 Mar 2022 10:08:10 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (relay.a.hostedemail.com [64.99.140.24]) by kanga.kvack.org (Postfix) with ESMTP id BAEB68D0001 for ; Mon, 7 Mar 2022 10:08:10 -0500 (EST) Received: from smtpin13.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay13.hostedemail.com (Postfix) with ESMTP id 9456E61739 for ; Mon, 7 Mar 2022 15:08:10 +0000 (UTC) X-FDA: 79217920740.13.088C3AB Received: from casper.infradead.org (casper.infradead.org [90.155.50.34]) by imf14.hostedemail.com (Postfix) with ESMTP id 8AED2100015 for ; Mon, 7 Mar 2022 15:08:09 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=infradead.org; s=casper.20170209; h=In-Reply-To:Content-Type:MIME-Version: References:Message-ID:Subject:Cc:To:From:Date:Sender:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description; bh=plJ8jvqLlfUhGwR69CkwoEy8s4m+ArARJ/Z+8bfWQNs=; b=JFyY5UWKWOO8lvVDL0l6hEHAaU fPcc3A5XoNftpX7H4i49b9YzT5H+S4AROxH9TtAv5E3GYc8pPB/nVI2RkorAvP885SfA2P9rBmWFl 4dOtWUDBYHoZXReuaJI/tPwoHYotw3ScDvO3CHevErZXGcn/3bzzj/1PhGYq8zPKbl8F1RLh+Jfql 9bUZTnhTLuVppiIEvDHk5cxOmvYQcMWNAZUZTqmAaa8UwPX4AqJeof53pliXTsvPXHVjue/hV27Ai j7RZcNP8TyTygdDc+NKr5gtgeSBBc4S0aoVkeeFxNzWndAu+B48o6DLqKzC97i/TaLMcjQEh6lOC7 yk26wMfA==; Received: from willy by casper.infradead.org with local (Exim 4.94.2 #2 (Red Hat Linux)) id 1nRExy-00FJxm-RZ; Mon, 07 Mar 2022 15:08:06 +0000 Date: Mon, 7 Mar 2022 15:08:06 +0000 From: Matthew Wilcox To: Ananda Cc: linux-mm@kvack.org, vitaly.wool@konsulko.com, vbabka@suse.cz, akpm@linux-foundation.org Subject: Re: [PATCH v3] mm: add ztree - new allocator for use via zpool API Message-ID: References: <20220307142724.14519-1-a.badmaev@clicknet.pro> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20220307142724.14519-1-a.badmaev@clicknet.pro> X-Rspam-User: X-Rspamd-Server: rspam12 X-Rspamd-Queue-Id: 8AED2100015 X-Stat-Signature: xyoeou9yddmubk9hzif5gsa1z161pauo Authentication-Results: imf14.hostedemail.com; dkim=pass header.d=infradead.org header.s=casper.20170209 header.b=JFyY5UWK; spf=none (imf14.hostedemail.com: domain of willy@infradead.org has no SPF policy when checking 90.155.50.34) smtp.mailfrom=willy@infradead.org; dmarc=none X-HE-Tag: 1646665689-72139 X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On Mon, Mar 07, 2022 at 05:27:24PM +0300, Ananda wrote: > +/***************** > + * Structures > + *****************/ You don't need this. I can see they're structures. > +/** > + * struct ztree_block - block metadata > + * Block consists of several (1/2/4/8) pages and contains fixed > + * integer number of slots for allocating compressed pages. > + * @block_node: links block into the relevant tree in the pool > + * @slot_info: contains data about free/occupied slots > + * @compressed_data: pointer to the memory block > + * @block_index: unique for each ztree_block in the tree > + * @free_slots: number of free slots in the block > + * @coeff: to switch between blocks > + * @under_reclaim: if true shows that block is being evicted > + */ Earlier in the file you say this exposes no API and is to be used only through zpool. So what's the point of marking this as kernel-doc? > + /* 1 page blocks with 11 slots */ > + [1] = { PAGE_SIZE / (11 * sizeof(long)) * sizeof(long), 0xB, 0 }, Hm. So 368 bytes on 64-bit, but 372 bytes on 32-bit? Is that intentional? Why 11? > +/* > + * allocate new block and add it to corresponding block tree > + */ > +static struct ztree_block *alloc_block(struct ztree_pool *pool, > + int block_type, gfp_t gfp) You have some very strange indentation (throughout). > + block = kmem_cache_alloc(pool->block_cache, > + (gfp & ~(__GFP_HIGHMEM | __GFP_MOVABLE))); > + if (!block) > + return NULL; > + > + block->compressed_data = (void *)__get_free_pages(gfp, tree_desc[block_type].order); It makes no sense to mask out __GFP_HIGHMEM and __GFP_MOVABLE for the call to slab and then not mask them out here. Either they shouldn't ever be passed in, in which case that could either be asserted or made true in your own code. Or they can be passed in, and should always be masked. Or you genuinely want to be able to use highmem & movable memory for these data blocks, in which case you're missing calls to kmap() and memory notifiers to let you move the memory around. This smacks of "I tried something, and slab warned, so I fixed the warning" instead of thinking about what the warning meant. > + spin_lock(&tree->lock); > + /* check if there are free slots in the current and the last added blocks */ > + if (tree->current_block && tree->current_block->free_slots > 0) { > + block = tree->current_block; > + goto found; > + } > + if (tree->last_block && tree->last_block->free_slots > 0) { > + block = tree->last_block; > + goto found; > + } > + spin_unlock(&tree->lock); > + > + /* not found block with free slots try to allocate new empty block */ > + block = alloc_block(pool, block_type, gfp); > + spin_lock(&tree->lock); > + if (block) { > + tree->current_block = block; > + goto found; > + } Another place that looks like "I fixed the warning instead of thinking about it". What if you have two threads that execute this path concurrently? Looks to me like you leak the memory allocated by the first thread.