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 -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@xxxxxxxxx. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href
