On Sat, Mar 1, 2014 at 5:59 AM, Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx> wrote:
> On Fri, 28 Feb 2014 10:07:45 +0100 Peter Zijlstra <peterz@xxxxxxxxxxxxx> wrote:
>
>> On Fri, Feb 28, 2014 at 01:59:59AM +0200, Kirill A. Shutemov wrote:
>> > On Thu, Feb 27, 2014 at 03:48:08PM -0800, Andrew Morton wrote:
>> > > On Thu, 27 Feb 2014 21:26:40 +0800 Bob Liu <lliubbo@xxxxxxxxx> wrote:
>> > >
>> > > > --- a/mm/memory.c
>> > > > +++ b/mm/memory.c
>> > > > @@ -3419,6 +3419,7 @@ static int do_shared_fault(struct mm_struct *mm, struct vm_area_struct *vma,
>> > > > pgoff_t pgoff, unsigned int flags, pte_t orig_pte)
>> > > > {
>> > > > struct page *fault_page;
>> > > > + struct address_space *mapping;
>> > > > spinlock_t *ptl;
>> > > > pte_t *pte;
>> > > > int dirtied = 0;
>> > > > @@ -3454,13 +3455,14 @@ static int do_shared_fault(struct mm_struct *mm, struct vm_area_struct *vma,
>> > > >
>> > > > if (set_page_dirty(fault_page))
>> > > > dirtied = 1;
>> > > > + mapping = fault_page->mapping;
>> > > > unlock_page(fault_page);
>> > > > - if ((dirtied || vma->vm_ops->page_mkwrite) && fault_page->mapping) {
>> > > > + if ((dirtied || vma->vm_ops->page_mkwrite) && mapping) {
>> > > > /*
>> > > > * Some device drivers do not set page.mapping but still
>> > > > * dirty their pages
>> > > > */
>> > > > - balance_dirty_pages_ratelimited(fault_page->mapping);
>> > > > + balance_dirty_pages_ratelimited(mapping);
>> > > > }
>> > > >
>> > > > /* file_update_time outside page_lock */
>> > >
>> > > So from my reading of the email thread, this patch has issues: the
>> > > compiler can just undo what you did.
>> >
>> > If I read PeterZ correctly, we are fine with the fix since unlock_page()
>> > has release semantics.
>>
>> Yeah, the compiler should not re-read mapping after unlock_page(). That
>> said; it might be good to put a comment near there saying we actually
>> rely on this, and _why_ we rely on this.
>
> Like this?
>
> --- a/mm/memory.c~mm-introduce-do_shared_fault-and-drop-do_fault-fix-fix
> +++ a/mm/memory.c
> @@ -3476,6 +3476,12 @@ set_pte:
>
> if (set_page_dirty(fault_page))
> dirtied = 1;
> + /*
> + * Take a local copy of the address_space - page.mapping may be zeroed
> + * by truncate after unlock_page(). The address_space itself remains
> + * pinned by vma->vm_file's reference. We rely on unlock_page()'s
> + * release semantics to prevent the compiler from undoing this copying.
> + */
> mapping = fault_page->mapping;
> unlock_page(fault_page);
> if ((dirtied || vma->vm_ops->page_mkwrite) && mapping) {
>
> I don't actually know if that's true. What *does* protect ->mapping
> from reclaim, drop_caches, etc?
>
I also puzzled what can protect ->mapping. This patch just change the
handling of shared-write-pagefault back to the same way as it used to
be. Perhaps we should add if(mapping==NULL) into
balance_dirty_pages_ratelimited() and balance_dirty_pages()?
BTW: Sasha, could you please have a test with this patch?
--
Regards,
--Bob
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxx. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>