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 EA0E2C433F5 for ; Tue, 8 Mar 2022 15:40:36 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 5F2B58D0002; Tue, 8 Mar 2022 10:40:36 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 5A2448D0001; Tue, 8 Mar 2022 10:40:36 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 4914E8D0002; Tue, 8 Mar 2022 10:40:36 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (relay.hostedemail.com [64.99.140.28]) by kanga.kvack.org (Postfix) with ESMTP id 3AE2F8D0001 for ; Tue, 8 Mar 2022 10:40:36 -0500 (EST) Received: from smtpin02.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay11.hostedemail.com (Postfix) with ESMTP id 1CDB481FA9 for ; Tue, 8 Mar 2022 15:40:36 +0000 (UTC) X-FDA: 79221631272.02.704A12B Received: from casper.infradead.org (casper.infradead.org [90.155.50.34]) by imf29.hostedemail.com (Postfix) with ESMTP id 3748A120005 for ; Tue, 8 Mar 2022 15:40:35 +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-Transfer-Encoding: Content-Type:MIME-Version:References:Message-ID:Subject:Cc:To:From:Date: Sender:Reply-To:Content-ID:Content-Description; bh=2ze43B38kALPJoqbpAdj2UQXWKKTuuLnsAGytaHXHmk=; b=jN2lkHkKyeppcn3xCzGSw6FTZi RFy4zmXwe4Vo9g/9f+PLer2WskxS9o79w+o/zAa9mEyyBNMM1HJ1ENufyCKfEWs0fTxu46+YHnzjs bizZcrvWkiYmYjmkHnjrSZH1vOVRYGJrdG8j6MnVEzfndw4TqPwajDJPjj+B8fR1Ey0J+xcYe5Sz0 CRtNZLgyXdz/40ahLa3bOk6j3TbuX46mQo5gXsUU8DJI5em34Bdz9hxMpA9z27cw4rPTRX1DXXv4c diGQayH7qWWJfG5jrvmB2Ao2StiVs3bwIojb6s5YnseHJdYmotFlY0WQFznBmviMrj8R+Pc3zJYVY 4b7W+4NA==; Received: from willy by casper.infradead.org with local (Exim 4.94.2 #2 (Red Hat Linux)) id 1nRbwu-00GINy-8A; Tue, 08 Mar 2022 15:40:32 +0000 Date: Tue, 8 Mar 2022 15:40:32 +0000 From: Matthew Wilcox To: Ananda Badmaev 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> <8255c6882aa7f78e6855cf4adda66afe0c1df809.camel@clicknet.pro> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <8255c6882aa7f78e6855cf4adda66afe0c1df809.camel@clicknet.pro> X-Rspam-User: X-Rspamd-Server: rspam12 X-Rspamd-Queue-Id: 3748A120005 X-Stat-Signature: b57mtyp4dqmdr95cfw66mbfzuk74dbon Authentication-Results: imf29.hostedemail.com; dkim=pass header.d=infradead.org header.s=casper.20170209 header.b=jN2lkHkK; spf=none (imf29.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: 1646754035-411888 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 Tue, Mar 08, 2022 at 06:16:35PM +0300, Ananda Badmaev wrote: > > > +       /* 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? > > > Yes, 'slot_size' and 'slots_per_block' values are chosen so that in > general the range from 0 to PAGE_SIZE is split more or less evenly and > the size of the block is as close as possible to the power of 2. Also > 'slot_size' values are aligned to the size of long. Is it intrinsic to the design that the blocks are aligned to sizeof(long)? > > > +/* > > > + * 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). > > > I was trying to limit the length of lines. But you didn't achieve that. The _start_ of the line containing block_type and gfp was _more indented_ than the end of the previous line. Do you have unusual tab settings? > > > +       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. > > > It seems that these flags should be masked out in alloc_block(). Maybe? I don't know the > > > +       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; > > 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. > > > Probably I should pass GFP_ATOMIC flag to alloc_block() and execute > this entire section of code under single spinlock. I think that's a bad approach. Better would be: spin_lock(); if (free_slot_block) goto found; spin_unlock(); block = alloc_block(); spin_lock(); if (free_slot_block) { free_block(block); block = free_slot_block; } else { free_slot_block = block; } found: ... spin_unlock();