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 A4CAEC433F5 for ; Tue, 8 Mar 2022 13:13:46 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 3E3E88D0002; Tue, 8 Mar 2022 08:13:46 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 36B448D0001; Tue, 8 Mar 2022 08:13:46 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 2336C8D0002; Tue, 8 Mar 2022 08:13:46 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0201.hostedemail.com [216.40.44.201]) by kanga.kvack.org (Postfix) with ESMTP id 0F0F88D0001 for ; Tue, 8 Mar 2022 08:13:46 -0500 (EST) Received: from smtpin20.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay04.hostedemail.com (Postfix) with ESMTP id B896AA89AB for ; Tue, 8 Mar 2022 13:13:45 +0000 (UTC) X-FDA: 79221261210.20.E040CBF Received: from casper.infradead.org (casper.infradead.org [90.155.50.34]) by imf22.hostedemail.com (Postfix) with ESMTP id BC42FC000D for ; Tue, 8 Mar 2022 13:13:44 +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=tEFtZlf/CYCLotgQg1WbOr0w3bRkJyHUcLCksKccShQ=; b=t/0M4XvfLdjnZ+Ca2Y4EH0mUgs baFhg0bQSNWs8ZeovRSODrHPtrUfSpAJpFK6y4SnMTMb+7QpfKfGE58MeT87XJiJvxzneCm9Cg7gt fNPB3f0up4PLb8lsCg+S5Bsrd4VeNgu7gvI6xZNj1wmog3TZm3lPuG2iFp4JpxxdVD6p3e2kOQa9E VrBB6v9d6xte5V1i9Z3bcsQZj+CtnyD1givVlb35LOVhXZwXlLdYYhyz6CVQTeHeqolqNoIBEkK+H GpiIffB7WFV0LjuGTPU59Kru3rgW+72Kjf6L8R8y/Nd3L91I/P/k69kexqEuMWUR3k96VxSTqSdgV mhyARhNg==; Received: from willy by casper.infradead.org with local (Exim 4.94.2 #2 (Red Hat Linux)) id 1nRZeo-00GBdp-HT; Tue, 08 Mar 2022 13:13:42 +0000 Date: Tue, 8 Mar 2022 13:13:42 +0000 From: Matthew Wilcox To: =?utf-8?B?0JDQvdCw0L3QtNCwINCR0LDQtNC80LDQtdCy?= 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> <719621646713798@mail.yandex.ru> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable In-Reply-To: <719621646713798@mail.yandex.ru> X-Rspamd-Server: rspam09 X-Rspamd-Queue-Id: BC42FC000D X-Stat-Signature: 6cu9sq4cxe79x13w7bccgqu1nqujmi7c Authentication-Results: imf22.hostedemail.com; dkim=pass header.d=infradead.org header.s=casper.20170209 header.b="t/0M4Xvf"; spf=none (imf22.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-Rspam-User: X-HE-Tag: 1646745224-762800 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: This is unreadable. Please fix your email client. On Tue, Mar 08, 2022 at 08:32:54AM +0300, =D0=90=D0=BD=D0=B0=D0=BD=D0=B4=D0= =B0 =D0=91=D0=B0=D0=B4=D0=BC=D0=B0=D0=B5=D0=B2 wrote: >
07.03.2022, 18:08, "Matthew Wilcox" <willy@infradead.org>:

On Mon, Mar 07, 2022 at 05:27:24PM +0300, Ananda wrote:

=C2=A0+/*****************
=C2=A0+ * Structures
=C2= =A0+ *****************/


You don't need this. I can see= they're structures.
=C2=A0

=C2=A0+/**
=C2=A0+ * s= truct ztree_block - block metadata
=C2=A0+ * Block consists of several= (1/2/4/8) pages and contains fixed
=C2=A0+ * integer number of slots = for allocating compressed pages.
=C2=A0+ * @block_node: links block in= to the relevant tree in the pool
=C2=A0+ * @slot_info: contains data a= bout free/occupied slots
=C2=A0+ * @compressed_data: pointer to the me= mory block
=C2=A0+ * @block_index: unique for each ztree_block in the = tree
=C2=A0+ * @free_slots: number of free slots in the block
=C2= =A0+ * @coeff: to switch between blocks
=C2=A0+ * @under_reclaim: if t= rue shows that block is being evicted
=C2=A0+ */


= 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?

It will be removed in the next version.
=C2=A0+ /* 1 page blocks with 11 slots */
=C2=A0+ [1= ] =3D { PAGE_SIZE / (11 * sizeof(long)) * sizeof(long), 0xB, 0 },


Hm. So 368 bytes on 64-bit, but 372 bytes on 32-bit? Is thatintentional? Why 11?

Yes, 'slot_size' and 'slo= ts_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.
=C2=A0+/*
=C2=A0+ * allocate new block and add it to corresponding bl= ock tree
=C2=A0+ */
=C2=A0+static struct ztree_block *alloc_block= (struct ztree_pool *pool,
=C2=A0+ int block_type, gfp_t gfp)


You have some very strange indentation (throughout).

I was trying to limit the length of lines.

=C2=A0

=C2=A0+ block =3D kmem_cache_alloc(pool->= ;block_cache,
=C2=A0+ (gfp & ~(__GFP_HIGHMEM | __GFP_MOVABLE)));=C2=A0+ if (!block)
=C2=A0+ return NULL;
=C2=A0+
=C2=A0+= block->compressed_data =3D (void *)__get_free_pages(gfp, tree_desc[bloc= k_type].order);


It makes no sense to mask out __GFP_HI= GHMEM and __GFP_MOVABLE for the call
to slab and then not mask them ou= t here. Either they shouldn't ever be
passed in, in which case that co= uld either be asserted or made true in
your own code. Or they can be p= assed in, and should always be masked.
Or you genuinely want to be abl= e to use highmem & movable memory for
these data blocks, in which = case you're missing calls to kmap() and
memory notifiers to let you mo= ve the memory around.

This smacks of "I tried something, and sla= b warned, so I fixed the
warning" instead of thinking about what the w= arning meant.

It seems that these flags should be= masked out in alloc_block().
=C2=A0+ sp= in_lock(&tree->lock);
=C2=A0+ /* check if there are free slots = in the current and the last added blocks */
=C2=A0+ if (tree->curre= nt_block && tree->current_block->free_slots > 0) {=
=C2=A0+ block =3D tree->current_block;
=C2=A0+ goto found;=C2=A0+ }
=C2=A0+ if (tree->last_block && tree->last_= block->free_slots > 0) {
=C2=A0+ block =3D tree->last= _block;
=C2=A0+ goto found;
=C2=A0+ }
=C2=A0+ spin_unlock(&a= mp;tree->lock);
=C2=A0+
=C2=A0+ /* not found block with free s= lots try to allocate new empty block */
=C2=A0+ block =3D alloc_block(= pool, block_type, gfp);
=C2=A0+ spin_lock(&tree->lock);
= =C2=A0+ if (block) {
=C2=A0+ tree->current_block =3D block;=
=C2=A0+ goto found;
=C2=A0+ }


Another place= that looks like "I fixed the warning instead of thinking
about it". W= hat 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.