linux-kernel - Re: [PATCH 2/7] fs/locks: Replace lg

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-next>] [day] [month] [year] [list]

Date:   Tue, 6 Sep 2016 03:58:00 +0200
From:   Andreas Mohr <andi@...as.de>
To:     Peter Zijlstra <peterz@...radead.org>
Cc:     der.herr@...r.at, Al Viro <viro@...IV.linux.org.uk>,
        mingo@...hat.com, torvalds@...ux-foundation.org, dave@...olabs.net,
        Oleg Nesterov <oleg@...hat.com>, riel@...hat.com,
        tj@...nel.org, paulmck@...ux.vnet.ibm.com,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/7] fs/locks: Replace lg_global with a percpu-rwsem

Hi,

[no properly binding reference via In-Reply-To: available thus manually re-creating, sorry]

https://lkml.org/lkml/2016/9/5/832


Two thoughts:

***multiple locks

Don't have much insight into this
(didn't spend much thinking on this),
but of course it's unfortunate
that two lock types need to be serviced
rather than having the atomic handling exchange area/section
be sufficiently guarded by one lock only
(the question here could possibly be:
what kind of currently existing
structural disadvantage/layer distribution
prevents us from being able to
have things simply serviced
within one granular lock area only?)


***lock handling

> +	percpu_down_read(&file_rwsem);
>  	spin_lock(&ctx->flc_lock);


>  	spin_unlock(&ctx->flc_lock);
> +	percpu_up_read(&file_rwsem);


These are repeated multiple times in this commit, thus error-prone.

A possibly good way to commit-micro-manage this would be:
1. commit shoves things into a newly created encapsulation/wrapper helper
   stuff_lock(&flc_lock);  /* <---- naming surely can be improved here */
2. [this commit]
   extend encapsulation/wrapper helper
   to be servicing file_rwsem, too:
   stuff_lock(&flc_lock, &file_rwsem);

That way it is pretty much guaranteed that:
a) neither one nor the other lock type
   will get forgotten later, at a specific use site
b) lock order will be correctly maintained at all times
   (AB-BA deadlock......)


[or, IOW, you get some symmetry/consistency malus points
for having provided
a relatively large amount of LOC extensions
which put certain amounts of stress on
code implementation quality/maintainability ;)]

HTH,

Andreas Mohr