linux-kernel - Re: [PATCHv6 4/8] zswap: add to mm/

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Mon, 25 Feb 2013 11:21:16 -0600
From:	Seth Jennings <sjenning@...ux.vnet.ibm.com>
To:	Joonsoo Kim <iamjoonsoo.kim@....com>
CC:	Andrew Morton <akpm@...ux-foundation.org>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Nitin Gupta <ngupta@...are.org>,
	Minchan Kim <minchan@...nel.org>,
	Konrad Rzeszutek Wilk <konrad.wilk@...cle.com>,
	Dan Magenheimer <dan.magenheimer@...cle.com>,
	Robert Jennings <rcj@...ux.vnet.ibm.com>,
	Jenifer Hopper <jhopper@...ibm.com>,
	Mel Gorman <mgorman@...e.de>,
	Johannes Weiner <jweiner@...hat.com>,
	Rik van Riel <riel@...hat.com>,
	Larry Woodman <lwoodman@...hat.com>,
	Benjamin Herrenschmidt <benh@...nel.crashing.org>,
	Dave Hansen <dave@...ux.vnet.ibm.com>,
	Joe Perches <joe@...ches.com>,
	Cody P Schafer <cody@...ux.vnet.ibm.com>, linux-mm@...ck.org,
	linux-kernel@...r.kernel.org, devel@...verdev.osuosl.org
Subject: Re: [PATCHv6 4/8] zswap: add to mm/

On 02/24/2013 10:35 PM, Joonsoo Kim wrote:
> Hello, Seth.
> Here comes minor comments.
> 
<snip>
>> +static int __zswap_cpu_notifier(unsigned long action, unsigned long cpu)
>> +{
>> +	struct crypto_comp *tfm;
>> +	u8 *dst;
>> +
>> +	switch (action) {
>> +	case CPU_UP_PREPARE:
>> +		tfm = crypto_alloc_comp(zswap_compressor, 0, 0);
>> +		if (IS_ERR(tfm)) {
>> +			pr_err("can't allocate compressor transform\n");
>> +			return NOTIFY_BAD;
>> +		}
>> +		*per_cpu_ptr(zswap_comp_pcpu_tfms, cpu) = tfm;
>> +		dst = (u8 *)__get_free_pages(GFP_KERNEL, 1);
> 
> Order 1 is really needed?
> Following code uses only PAGE_SIZE, not 2 * PAGE_SIZE.

Yes, probably should add a comment here.

Some compression modules in the kernel, notably LZO, do not guard
against buffer overrun during compression.  In cases where LZO tries
to compress a page with high entropy (e.g. a page containing already
compressed data like JPEG), the compressed result can actually be
larger than the original data.  In this case, if the compression
buffer is only one page, we overrun.  I actually encountered this
during development.

> 
>> +		if (!dst) {
>> +			pr_err("can't allocate compressor buffer\n");
>> +			crypto_free_comp(tfm);
>> +			*per_cpu_ptr(zswap_comp_pcpu_tfms, cpu) = NULL;
>> +			return NOTIFY_BAD;
>> +		}
<snip>
>> +	buf = zs_map_object(tree->pool, handle, ZS_MM_WO);
>> +	memcpy(buf, dst, dlen);
>> +	zs_unmap_object(tree->pool, handle);
>> +	put_cpu_var(zswap_dstmem);
>> +
>> +	/* allocate entry */
>> +	entry = zswap_entry_cache_alloc(GFP_KERNEL);
>> +	if (!entry) {
>> +		zs_free(tree->pool, handle);
>> +		zswap_reject_kmemcache_fail++;
>> +		ret = -ENOMEM;
>> +		goto reject;
>> +	}
> 
> How about moving up zswap_entry_cache_alloc()?
> It can save compression processing time
> if zswap_entry_cache_alloc() is failed.

Will do.

> 
>> +
>> +	/* populate entry */
>> +	entry->type = type;
>> +	entry->offset = offset;
>> +	entry->handle = handle;
>> +	entry->length = dlen;
>> +
<snip>
>> +/* invalidates all pages for the given swap type */
>> +static void zswap_frontswap_invalidate_area(unsigned type)
>> +{
>> +	struct zswap_tree *tree = zswap_trees[type];
>> +	struct rb_node *node;
>> +	struct zswap_entry *entry;
>> +
>> +	if (!tree)
>> +		return;
>> +
>> +	/* walk the tree and free everything */
>> +	spin_lock(&tree->lock);
>> +	/*
>> +	 * TODO: Even though this code should not be executed because
>> +	 * the try_to_unuse() in swapoff should have emptied the tree,
>> +	 * it is very wasteful to rebalance the tree after every
>> +	 * removal when we are freeing the whole tree.
>> +	 *
>> +	 * If post-order traversal code is ever added to the rbtree
>> +	 * implementation, it should be used here.
>> +	 */
>> +	while ((node = rb_first(&tree->rbroot))) {
>> +		entry = rb_entry(node, struct zswap_entry, rbnode);
>> +		rb_erase(&entry->rbnode, &tree->rbroot);
>> +		zs_free(tree->pool, entry->handle);
>> +		zswap_entry_cache_free(entry);
>> +	}
> 
> You should decrease zswap_stored_pages in while loop.

Yes. Will do.

Thanks,
Seth

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/