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;
MIME-Version: 1.0
References: <20230414175444.1837474-1-surenb@google.com> <ZDnJ1dOU2tpK6l68@x1n>
 <CAJuCfpFc2SohkkJnEFqZD-uCpSS9sUzToPcQXOR6dHTTE0Ty5w@mail.gmail.com>
 <ZDnVETTFus3BFLxH@x1n> <CAJuCfpH278ozvhz=z4W0d6TxY6HhrORbsBPFM5d4x1M=EM23WQ@mail.gmail.com>
In-Reply-To: <CAJuCfpH278ozvhz=z4W0d6TxY6HhrORbsBPFM5d4x1M=EM23WQ@mail.gmail.com>
From:   Suren Baghdasaryan <surenb@google.com>
Date:   Fri, 14 Apr 2023 17:11:25 -0700
Message-ID: <CAJuCfpExSdUgB6YV=k7UECnfgFJwQi-CHK6sRmJyc3OGx3GFwQ@mail.gmail.com>
Subject: Re: [PATCH 1/1] mm: do not increment pgfault stats when page fault
 handler retries
To:     Peter Xu <peterx@redhat.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
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Fri, Apr 14, 2023 at 4:49=E2=80=AFPM Suren Baghdasaryan <surenb@google.c=
om> wrote:
>
> On Fri, Apr 14, 2023 at 3:35=E2=80=AFPM Peter Xu <peterx@redhat.com> wrot=
e:
> >
> > 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) - basicall=
y
> > > we need to retry but no errors happened? If so then this condition
> > > would double-count pagefaults in such cases.
> >
> > If ret=3D=3DVM_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
>
> After looking some more into mm_account_fault(), I think it would be
> fine to count the faults which produced errors. IIUC these counters
> represent the total number of faults, not the number of valid and
> successful faults. If so then I think simply using VM_FAULT_RETRY
> should be ok without considering all possible combinations. WDYT?

I posted v2 at https://lore.kernel.org/all/20230415000818.1955007-1-surenb@=
google.com/
Hopefully it's closer to what we want it to be.

>
> >
> > I don't think ERROR & RETRY can even be set at the same time so I assum=
e
> > there's no option 4) - a RETRY should imply no ERROR already, even thou=
gh
> > it's still incomplete so need another attempt.
> >
> > Thanks,
> >
> > --
> > Peter Xu
> >