On 9/26/19 10:06 PM, Yu Zhao wrote: > 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(). Good point, maybe there's a better place to do it... > >>> 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. > Documentation/memory-barriers.txt points out: INTERRUPT DISABLING FUNCTIONS ----------------------------- Functions that disable interrupts (ACQUIRE equivalent) and enable interrupts (RELEASE equivalent) will act as compiler barriers only. So if memory or I/O barriers are required in such a situation, they must be provided from some other means. btw, I'm really sorry I missed your responses over the last 3 or 4 days. I just tracked down something in our email system that was sometimes moving some emails to spam (just few enough to escape immediate attention, argghh!). I think I killed it off for good now. I wasn't ignoring you. :) thanks, -- John Hubbard NVIDIA
