On Thu, 24 Oct 2024, Zi Yan wrote: > On 24 Oct 2024, at 0:10, Hugh Dickins wrote: > > > The new unlocked list_del_init() in deferred_split_scan() is buggy. > > I gave bad advice, it looks plausible since that's a local on-stack > > list, but the fact is that it can race with a third party freeing or > > migrating the preceding folio (properly unqueueing it with refcount 0 > > while holding split_queue_lock), thereby corrupting the list linkage. > > > > The obvious answer would be to take split_queue_lock there: but it has > > a long history of contention, so I'm reluctant to add to that. Instead, > > make sure that there is always one safe (raised refcount) folio before, > > by delaying its folio_put(). (And of course I was wrong to suggest > > updating split_queue_len without the lock: leave that until the splice.) > > I feel like this is not the right approach, since it breaks the existing > condition of changing folio->_deferred_list, namely taking > ds_queue->split_queue_lock for serialization. The contention might not be > as high as you think, since if a folio were split, the split_queue_lock > needed to be taken during split anyway. So the worse case is the same > as all folios are split. Do you see significant perf degradation due to > taking the lock when doing list_del_init()? > > I am afraid if we take this route, we might hit hard-to-debug bugs > in the future when someone touches the code. You have a good point: I am adding another element of trickiness to that already-tricky local-but-not-quite list - which has tripped us up a few times in the past. I do still feel that this solution is right in the spirit of that list; but I've certainly not done any performance measurement to justify it, nor would I ever trust my skill to do so. I just tried to solve the corruptions in what I thought was the best way. (To be honest, I found this solution to the corruptions first, and thought the bug went back to the original implemention: that its put_page() at the end of the loop was premature all along. It was only when writing the commit message two days ago, that I came to realize that even put_page() or folio_put() would be safely using the lock to unqueue: that it is only this new list_del_init() which is the exception which introduces the bug.) Looking at vmstats, I'm coming to believe that the performance advantage of this way is likely to be in the noise: that mTHPs alone, and the !partially_mapped case on top, are greatly increasing the split_deferred stats: and may give rise to renewed complaints of lock contention, with or without this optimization. While I still prefer to stick with what's posted and most tested, I am giving the locked version a run overnight. Thanks a lot for the reviews and acks everyone: at present Zi Yan is in the minority preferring a locked version, but please feel free to change your vote if you wish. Hugh
