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:	Thu, 06 Jun 2013 00:21:51 +0800
From:	Jiang Liu <liuj97@...il.com>
To:	Jerome Marchand <jmarchan@...hat.com>
CC:	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Nitin Gupta <ngupta@...are.org>,
	Minchan Kim <minchan@...nel.org>,
	Yijing Wang <wangyijing@...wei.com>,
	Jiang Liu <jiang.liu@...wei.com>, devel@...verdev.osuosl.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 10/10] zram: use atomic64_xxx() to replace zram_stat64_xxx()

On Wed 05 Jun 2013 08:02:12 PM CST, Jerome Marchand wrote:
> On 06/04/2013 06:06 PM, Jiang Liu wrote:
>> Use atomic64_xxx() to replace open-coded zram_stat64_xxx().
>> Some architectures have native support of atomic64 operations,
>> so we can get rid of the spin_lock() in zram_stat64_xxx().
>> On the other hand, for platforms use generic version of atomic64
>> implement, it may cause an extra save/restore of the interrupt
>> flag.  So it's a tradeoff.
>>
>> Signed-off-by: Jiang Liu <jiang.liu@...wei.com>
>
> Before optimizing stats, I'd like to make sure that they're correct.
> What makes 64 bits fields so different that they need atomicity while
> 32 bits wouldn't? Actually all of them save compr_size only increase,
> which would make a race less critical than for 32 bits fields that all
> can go up and down (if a decrement overwrites a increment, the counter
> can wrap around zero).
>
> Jerome
>
Hi Jerome,
          I'm not sure about the design decision, but I could give a 
guess here.
1) All 32-bit counters are only modified by 
zram_bvec_write()/zram_page_free()
and is/should be protected by down_write(&zram->lock).
2) __zram_make_request() modifies some 64-bit counters without 
protection.
3) zram_bvec_write() modifies some 64-bit counters and it's protected 
with
     down_read(&zram->lock).
4) It's always safe for sysfs handler to read 32bit counters.
5) It's unsafe for sysfs handler to read 64bit counters on 32bit 
platforms.

So it does work with current design, but very hard to understand.
Suggest to use atomic_t for 32bit counters too for maintainability,
though may be a little slower.
Any suggestion?
Regards!
Gerry

--
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/

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ