Re: [PATCH] dma-debug: fix debug_dma_assert_idle(), use rcu_read_lock()

[Hugh Dickins]

On Fri, 14 Aug 2020, Linus Torvalds wrote:
> On Thu, Aug 13, 2020 at 10:42 PM Christoph Hellwig <hch@xxxxxx> wrote:
> >
> > The whole thing predates my involvement with the code, but I defintively
> > think the patch from Hugh is a major improvement.  But I would also
> > have no problem with just removing it entirely.
> 
> I decided to just do both, since neither you nor Dan seemed to really object.
> 
> I applied Hugh's RCU read locking patch as a clear improvement, and
> then I did a second patch that just removed this function entirely.
> That sounds a bit odd, perhaps, but in case people decide to resurrect
> the debugging code, I didn't want us to lose sight of Hugh's
> improvement just because I then decided that we might as well go one
> step further and just remove it entirely.

That's ideal, thanks - exactly the sequence I was hoping for.

(Another shortcoming in debug_dma_assert_idle(), that I hadn't wanted
to distract us by mentioning, is that it assumed that the mapping is
contained within one small page, whereas I believe one or more of the
DMA mapping functions take a size_t argument, that could in theory span
small pages - I guess more plausible inside a compound page; yet it
looked like only an initial entry would be put into the radix-tree.)

> 
> And the only real reason I care is that this whole COW and page lock
> thing has showed up lately, and I like removing code.
> 
> I'm _very_ tempted to just apply my COW simplification patch that gets
> rid of all the complex try-to-share cases entirely (and would also
> obviate the whole forced-cow patch). I suspect it would effectively
> remove almost all of the [un[lock_page() bottlenecks entirely, but
> that code has decades of history and I suspect it's a bit too drastic
> wrt KSM and the swap cache pages.

Yes, you're right to hold back.

I'd been looking there too (but backed off while speeding up the
fork was causing the "Hugh load" to "fail": it's the exit that now
wants speeding up, to please that test).  I think it could well avoid
getting into page locking when mapcount is quickly seen to be high
(> 1? > 2? > bigger? I never did the logic), but the page locking
becomes important when mapcount looks low, yet swap might be involved.

We used to rely on page count there, and on trylock_page() only; but
there was at least one user whose app went wrong when occasionally we
COWed the page, just because something else momentarily took a reference
to it, or locked it.  Around 2006, bug report from 2004: I did look up
the history a week ago, but was interrupted before taking notes.

> 
> It would be lovely if the main source of page locking would really be
> about just IO, but the page lock has also become the thing that
> serializes almost everything related to page state. Which is why you
> find it in contexts that are really not IO-related at all (not just
> COW - page migration is the other one that has shown up a lot under
> "heavy CPU loads" without really necessarily any IO component to it).
> 
>                          Linus