linux-kernel - Re: [locks] 6d390e4b5d: will-it-scale.per_process

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:   Sat, 14 Mar 2020 08:58:04 -0700
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     NeilBrown <neilb@...e.de>
Cc:     Jeff Layton <jlayton@...nel.org>, yangerkun <yangerkun@...wei.com>,
        kernel test robot <rong.a.chen@...el.com>,
        LKML <linux-kernel@...r.kernel.org>, lkp@...ts.01.org,
        Bruce Fields <bfields@...ldses.org>,
        Al Viro <viro@...iv.linux.org.uk>
Subject: Re: [locks] 6d390e4b5d: will-it-scale.per_process_ops -96.6% regression

On Fri, Mar 13, 2020 at 7:31 PM NeilBrown <neilb@...e.de> wrote:
>
> The idea of list_del_init_release() and list_empty_acquire() is growing
> on me though.  See below.

This does look like a promising approach.

However:

> +       if (waiter->fl_blocker == NULL &&
> +           list_empty(&waiter->fl_blocked_requests) &&
> +           list_empty_acquire(&waiter->fl_blocked_member))
> +               return status;

This does not seem sensible to me.

The thing is, the whole point about "acquire" semantics is that it
should happen _first_ - because a load-with-acquire only orders things
_after_ it.

So testing some other non-locked state before testing the load-acquire
state makes little sense: it means that the other tests you do are
fundamentally unordered and nonsensical in an unlocked model.

So _if_ those other tests matter (do they?), then they should be after
the acquire test (because they test things that on the writer side are
set before the "store-release"). Otherwise you're testing random
state.

And if they don't matter, then they shouldn't exist at all.

IOW, if you depend on ordering, then the _only_ ordering that exists is:

 - writer side: writes done _before_ the smp_store_release() are visible

 - to the reader side done _after_ the smp_load_acquire()

and absolutely no other ordering exists or makes sense to test for.

That limited ordering guarantee is why a store-release -> load-acquire
is fundamentally cheaper than any other serialization.

So the optimistic "I don't need to do anything" case should start ouf with

        if (list_empty_acquire(&waiter->fl_blocked_member)) {

and go from there. Does it actually need to do anything else at all?
But if it does need to check the other fields, they should be checked
after that acquire.

Also, it worries me that the comment talks about "if fl_blocker is
NULL". But it realy now is that fl_blocked_member list being empty
that is the real serialization test, adn that's the one that the
comment should primarily talk about.

                Linus