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.
