linux-kernel - Re: [PATCH] list_lru: Prefetch neighboring list entries before acquiring lock

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: <04d15b8d-d69f-660f-2196-a10aab2fefa6@redhat.com>
Date:   Thu, 30 Nov 2017 15:55:01 -0500
From:   Waiman Long <longman@...hat.com>
To:     Dave Chinner <david@...morbit.com>
Cc:     Andrew Morton <akpm@...ux-foundation.org>,
        Vladimir Davydov <vdavydov.dev@...il.com>,
        Johannes Weiner <hannes@...xchg.org>,
        linux-kernel@...r.kernel.org, linux-mm@...ck.org
Subject: Re: [PATCH] list_lru: Prefetch neighboring list entries before
 acquiring lock

On 11/30/2017 03:38 PM, Dave Chinner wrote:
> On Thu, Nov 30, 2017 at 08:54:04AM -0500, Waiman Long wrote:
>>
>> For the record, I add one more list_empty() check at the beginning of
>> list_lru_del() in the patch for 2 purpose:
>> 1. it allows the code to bail out early.
> Which is what I said was wrong. You haven't addressed why you think
> it's safe to add racy specualtive checks to this code in your quest
> for speed.
>
> Also, I'm curious about is how much of the gain is from the
> prefetching, and how much of the gain is from avoiding the lock
> altogether by the early bailout...

The early bailout doesn't improve the test at all. In the case of
dentries, there is a flag that indicates that the dentry is in the LRU
list. So list_lru_del is only called when it is in the LRU list.

>> 2. It make sure the cacheline of the list_head entry itself is loaded.
>>
>> Other than that, I only add a likely() qualifier to the existing
>> list_empty() check within the lock critical region.
> Yup, but in many cases programmers get the static branch prediction
> hints are wrong. In this case, you are supposing that nobody ever
> calls list_lru_del() on objects that aren't on the lru. That's not
> true - inodes that are being evicted may never have been on the LRU
> at all, but we still call through list_lru_del() so it can determine
> the LRU state correctly (e.g. cache cold rm -rf workloads)....
>
> IOWs, I'm pretty sure even just adding static branch prediction
> hints here is wrong....

In the case of dentries, the static branch is right. However it may not
be true for other users of list_lru, so I am OK to take them out. Thanks
for the explanation.

Cheers,
Longman