On Tue, Nov 29, 2022 at 11:33 PM Hugh Dickins <hughd@xxxxxxxxxx> wrote: > > On Tue, 29 Nov 2022, Johannes Weiner wrote: > > On Mon, Nov 28, 2022 at 11:59:53AM -0500, Johannes Weiner wrote: > > > On Wed, Nov 23, 2022 at 10:03:00PM -0800, Hugh Dickins wrote: > > > The swapcache/pagecache bit was a brainfart. We acquire the folio lock > > > in move_account(), which would lock out concurrent faults. If it's not > > > mapped, I don't see how it could become mapped behind our backs. But > > > we do need to be prepared for it to be unmapped. > > > > Welp, that doesn't protect us from the inverse, where the page is > > mapped elsewhere and the other ptes are going away. So this won't be > > enough, unfortunately. > > > > > > Does that mean that we just have to reinstate the folio_mapped() checks > > > > in mm/memcontrol.c i.e. revert all mm/memcontrol.c changes from the > > > > commit? Or does it invalidate the whole project to remove > > > > lock_page_memcg() from mm/rmap.c? > > > > Short of further restricting the pages that can be moved, I don't see > > how we can get rid of the cgroup locks in rmap after all. :( > > > > We can try limiting move candidates to present ptes. But maybe it's > > indeed time to deprecate the legacy charge moving altogether, and get > > rid of the entire complication. > > > > Hugh, Shakeel, Michal, what do you think? > > I'm certainly not against deprecating it - it's a largish body of odd > code, which poses signficant problems, yet is very seldom used; but I > feel that we'd all like to see it gone from rmap quicker that it can > be fully deprecated out of existence. > > I do wonder if any user would notice, if we quietly removed its > operation on non-present ptes; certainly there *might* be users > relying on that behaviour, but I doubt that many would. > > Alternatively (although I think Linus's objection to it in rmap is on > both aesthetic and performance grounds, and retaining any trace of it > in rmap.c still fails the aesthetic), can there be some static-keying > done, to eliminate (un)lock_page_memcg() overhead for all but those few > who actually indulge in moving memcg charge at immigrate? (But I think > you would have already done that if it were possible.) > My preference would be going with the removal of non-present ptes over static-key in [un]lock_page_memcg(). How about the following steps: 1. Add warning in memory.move_charge_at_immigrate now (6.1/6.2) that this is going away and also backport it to the stable kernels. 2. For 6.2 (or 6.3), remove the non-present pte migration with some additional text in the warning and do the rmap cleanup. 3. After 3 or 4 releases (and hopefully finding no real users), we deprecate this completely. Step 3 can be delayed if there are some users depending on it. However we need to be firm that this is going away irrespective. Shakeel
