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. > > 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. Leonardo Bras's recent patchset brought this to my attention. Here, he's recommending adding atomic counting inc/dec before and after the gup_fast() irq disable/enable points: https://lore.kernel.org/r/20190920195047.7703-4-leonardo@xxxxxxxxxxxxx ...and that lead to noticing a general lack of barriers there. thanks, -- John Hubbard NVIDIA
