linux-kernel - Re: [PATCH 1/3] Support generic I/O requests

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]

Message-ID: <4C040C98.8000105@vflare.org>
Date:	Tue, 01 Jun 2010 00:53:04 +0530
From:	Nitin Gupta <ngupta@...are.org>
To:	Minchan Kim <minchan.kim@...il.com>
CC:	Greg KH <greg@...ah.com>, Pekka Enberg <penberg@...helsinki.fi>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Hugh Dickins <hugh.dickins@...cali.co.uk>,
	Cyp <cyp561@...il.com>, driverdev <devel@...verdev.osuosl.org>,
	linux-kernel <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/3] Support generic I/O requests

Hi Minchan,

On 05/31/2010 09:11 PM, Minchan Kim wrote:
> 
> On Mon, 2010-05-24 at 19:48 +0530, Nitin Gupta wrote:
<snip>
>>
>>  - Scalability: There is only one (per-device) de/compression
>> buffer stats. This can lead to significant contention, especially
>> when used for generic (non-swap) purposes.
> 
> Later, we could enhance it by per-cpu counter. 
> 

Yes, currently we effectively use just 1 core due to a single
compression buffer. So, per-cpu buffers and counters should
really help.
  
>> -static int handle_zero_page(struct bio *bio)
>> +static void handle_zero_page(struct page *page, u32 index)
> 
> It doesn't use index. 
>

Ok, I will remove this argument.
 
>>  
>> -	/* Page is stored uncompressed since it's incompressible */
>> -	if (unlikely(rzs_test_flag(rzs, index, RZS_UNCOMPRESSED)))
>> -		return handle_uncompressed_page(rzs, bio);
>> +		/* Requested page is not present in compressed area */
>> +		if (unlikely(!rzs->table[index].page)) {
>> +			pr_debug("Read before write on swap device: "
>                                                          ^^^^
> It's not ramzswap any more. It is zram. :)
>

I will correct this :)

 
>> +		/*
>> +		 * Page is incompressible. Store it as-is (uncompressed)
>> +		 * since we do not want to return too many swap write
>> +		 * errors which has side effect of hanging the system.
>> +		 */
>> +		if (unlikely(clen > max_zpage_size)) {
>> +			clen = PAGE_SIZE;
>> +			page_store = alloc_page(GFP_NOIO | __GFP_HIGHMEM);
>> +			if (unlikely(!page_store)) {
>> +				mutex_unlock(&rzs->lock);
> 
> What's purpose of rzs->lock?
> Please, Document it. 
>


Ok. I intentionally deferred much of the documentation for later patches
to reduce "side noise" for these patches. Anyways, I will document this
lock in "v2" patches.
 
> And please, take care of "swap" mentioned in comments. 
> 

Old habits die hard :)  I will clean them up.

> I think code doesn't have big problem. 
> But my feel is we can clean up some portion of codes. But it's not a big
> deal. It doesn't have any problem to work. 
> So I want to add zram into linux-next, too. 
> 

I will be sending patch series with code commentary, cleanups and minor
fixes once this much enters linux-next.

Thanks for the review.
Nitin
--
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/