On Sat, Sep 19, 2020 at 05:16:57PM -0700, Hugh Dickins wrote: > On Sat, 19 Sep 2020, Matthew Wilcox wrote: > > How about this for a band-aid until we sort that out properly? Just mark > > the page as dirty before splitting it so subsequent iterations see the > > subpages as dirty. > > This is certainly much better than my original, and I've had no problem > in running with it (briefly); and it's what I'll now keep in my testing > tree - thanks. But it is more obviously a hack, or as you put it, a > band-aid: fit for Linus's tree? > > This issue has only come up with i915 and shmem_enabled "force", nobody > has been bleeding except me: but if it's something that impedes or may > impede your own testing, or that you feel should go in anyway, say so and > I'll push your new improved version to akpm (adding your signoff to mine). No, it's not impeding my testing; I don't have i915 even compiled into my kernel. > > Arguably, we should use set_page_dirty() instead of SetPageDirty, > > My position on that has changed down the years: I went through a > phase of replacing shmem.c's SetPageDirtys by set_page_dirtys, with > the idea that its "if (!PageDirty)" is good to avoid setting cacheline > dirty. Then Spectre changed the balance, so now I'd rather avoid the > indirect function call, and go with your SetPageDirty. But the bdi > flags are such that they do the same thing, and if they ever diverge, > then we make the necessary changes at that time. That makes sense. > > but I don't think i915 cares. In particular, it uses > > an untagged iteration instead of searching for PAGECACHE_TAG_DIRTY. > > PAGECACHE_TAG_DIRTY comes with bdi flags that shmem does not use: > with one exception, every shmem page is always dirty (since its swap > is freed as soon as the page is moved from swap cache to file cache); > unless I'm forgetting something (like the tails in my patch!), the > only exception is a page allocated to a hole on fault (after which > a write fault will soon set pte dirty). Ah, of course. I keep forgetting that shmem doesn't use the dirty/writeback bits. > So I didn't quite get what i915 was doing with its set_page_dirty()s: > but very probably being a good citizen, marking when it exposed the > page to changes itself, making no assumption of what shmem.c does. > > It would be good to have a conversation with i915 guys about huge pages > sometime (they forked off their own mount point in i915_gemfs_init(), > precisely in order to make use of huge pages, but couldn't get them > working, so removed the option to ask for them, but kept the separate > mount point. But not a conversation I'll be responsive on at present.) Yes, I have a strong feeling the i915 shmem code is cargo-culted. There are some very questionable constructs in there. It's a little unfair to expect graphics developers to suddenly become experts on the page cache, so I've taken this as impetus to clean up our APIs a little (eg moving find_lock_entry() to mm/internal.h)
