Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 14 Apr 2023 18:34:57 -0400
From:   Peter Xu <peterx@redhat.com>
To:     Suren Baghdasaryan <surenb@google.com>
Cc:     akpm@linux-foundation.org, willy@infradead.org, hannes@cmpxchg.org,
        mhocko@suse.com, josef@toxicpanda.com, jack@suse.cz,
        ldufour@linux.ibm.com, laurent.dufour@fr.ibm.com,
        michel@lespinasse.org, liam.howlett@oracle.com, jglisse@google.com,
        vbabka@suse.cz, minchan@google.com, dave@stgolabs.net,
        punit.agrawal@bytedance.com, lstoakes@gmail.com,
        linux-mm@kvack.org, linux-kernel@vger.kernel.org,
        kernel-team@android.com
Subject: Re: [PATCH 1/1] mm: do not increment pgfault stats when page fault
 handler retries
Message-ID: <ZDnVETTFus3BFLxH@x1n>
References: <20230414175444.1837474-1-surenb@google.com>
 <ZDnJ1dOU2tpK6l68@x1n>
 <CAJuCfpFc2SohkkJnEFqZD-uCpSS9sUzToPcQXOR6dHTTE0Ty5w@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <CAJuCfpFc2SohkkJnEFqZD-uCpSS9sUzToPcQXOR6dHTTE0Ty5w@mail.gmail.com>
Precedence: bulk

Hi, Suren,

On Fri, Apr 14, 2023 at 03:14:23PM -0700, Suren Baghdasaryan wrote:
> > It also already ignores invalid faults:
> >
> >         if (ret & (VM_FAULT_ERROR | VM_FAULT_RETRY))
> >                 return;
> 
> Can there be a case of (!VM_FAULT_ERROR && VM_FAULT_RETRY) - basically
> we need to retry but no errors happened? If so then this condition
> would double-count pagefaults in such cases.

If ret==VM_FAULT_RETRY it should return here already, so I assume
mm_account_fault() itself is fine regarding fault retries?

Note that I think "ret & (VM_FAULT_ERROR | VM_FAULT_RETRY)" above means
"either ERROR or RETRY we'll skip the accounting".

IMHO we should have 3 cases here:

  - ERROR && !RETRY
    error triggered of any kind

  - RETRY && !ERROR
    we need to try one more time

  - !RETRY && !ERROR
    we finished the fault

I don't think ERROR & RETRY can even be set at the same time so I assume
there's no option 4) - a RETRY should imply no ERROR already, even though
it's still incomplete so need another attempt.

Thanks,

-- 
Peter Xu