On Thu, Sep 26, 2019 at 08:26:46PM -0700, John Hubbard wrote: > On 9/26/19 3:20 AM, Kirill A. Shutemov wrote: > > On Wed, Sep 25, 2019 at 04:26:54PM -0600, Yu Zhao wrote: > >> On Wed, Sep 25, 2019 at 10:25:30AM +0200, Peter Zijlstra wrote: > >>> On Tue, Sep 24, 2019 at 05:24:58PM -0600, Yu Zhao wrote: > ... > >>> I'm thinking this patch make stuff rather fragile.. Should we instead > >>> stick the barrier in set_p*d_at() instead? Or rather, make that store a > >>> store-release? > >> > >> I prefer it this way too, but I suspected the majority would be > >> concerned with the performance implications, especially those > >> looping set_pte_at()s in mm/huge_memory.c. > > > > We can rename current set_pte_at() to __set_pte_at() or something and > > leave it in places where barrier is not needed. The new set_pte_at()( will > > be used in the rest of the places with the barrier inside. > > +1, sounds nice. I was unhappy about the wide-ranging changes that would have > to be maintained. So this seems much better. Just to be clear that doing so will add unnecessary barriers to one of the two paths that share set_pte_at(). > > BTW, have you looked at other levels of page table hierarchy. Do we have > > the same issue for PMD/PUD/... pages? > > > > Along the lines of "what other memory barriers might be missing for > get_user_pages_fast(), I'm also concerned that the synchronization between > get_user_pages_fast() and freeing the page tables might be technically broken, > due to missing memory barriers on the get_user_pages_fast() side. Details: > > gup_fast() disables interrupts, but I think it also needs some sort of > memory barrier(s), in order to prevent reads of the page table (gup_pgd_range, > etc) from speculatively happening before the interrupts are disabled. I was under impression switching back from interrupt context is a full barrier (otherwise wouldn't we be vulnerable to some side channel attacks?), so the reader side wouldn't need explicit rmb.
