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: <Y0T2l3HaH2MU8M9m@gmail.com> <20221014134802.1361436-1-mdanylo@google.com>
 <474513c0-4ff9-7978-9d77-839fe775d04c@collabora.com> <CABb0KFGCm=K2X3-O=y3BJN85sT2C-y+XZRtLxnuabuOg+OrHwQ@mail.gmail.com>
 <17d7d6f5-21dc-37e1-6843-29c77a0e14b6@collabora.com>
In-Reply-To: <17d7d6f5-21dc-37e1-6843-29c77a0e14b6@collabora.com>
From:   =?UTF-8?B?TWljaGHFgiBNaXJvc8WCYXc=?= <emmir@google.com>
Date:   Tue, 18 Oct 2022 19:17:36 +0200
Message-ID: <CABb0KFFGRgy9D212skxxFMsHV5n3qjqUP9d-bQaWLUmtH46H3A@mail.gmail.com>
Subject: Re: [PATCH v3 0/4] Implement IOCTL to get and clear soft dirty PTE
To:     Muhammad Usama Anjum <usama.anjum@collabora.com>
Cc:     Danylo Mocherniuk <mdanylo@google.com>, avagin@gmail.com,
        linux-mm@kvack.org, akpm@linux-foundation.org,
        gregkh@linuxfoundation.org, corbet@lwn.net, david@redhat.com,
        kernel@collabora.com, krisman@collabora.com,
        linux-doc@vger.kernel.org, linux-fsdevel@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-kselftest@vger.kernel.org,
        peter.enderborg@sony.com, shuah@kernel.org,
        viro@zeniv.linux.org.uk, willy@infradead.org, figiel@google.com,
        kyurtsever@google.com, Paul Gofman <pgofman@codeweavers.com>,
        surenb@google.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Tue, 18 Oct 2022 at 15:23, Muhammad Usama Anjum
<usama.anjum@collabora.com> wrote:
>
> On 10/18/22 4:11 PM, Micha=C5=82 Miros=C5=82aw wrote:
> > On Tue, 18 Oct 2022 at 12:36, Muhammad Usama Anjum
> > <usama.anjum@collabora.com> wrote:
[...]
> >>    * @start:             Starting address
> >>    * @len:               Length of the region
> >>    * @vec:               Output page_region struct array
> >>    * @vec_len:           Length of the page_region struct array
> >>    * @max_out_page:      Optional max output pages (It must be less th=
an
> >> vec_len if specified)
> >
> > Why is it required to be less than vec_len? vec_len effectively
> > specifies max number of ranges to find, and this new additional field
> > counts pages, I suppose?
> > BTW, if we count pages, then what size of them? Maybe using bytes
> > (matching start/len fields) would be more consistent?
> Yes, it if for counting pages. As the regions can have multiple pages,
> user cannot specify through the number of regions that how many pages
> does he need. Page size is used here as well like the start and len.
> This is optional argument as this is only needed to emulate the Windows
> syscall getWriteWatch.

I'm wondering about the condition that max_out_page < vec_len. Since
both count different things (pages vs ranges) I would expect there is
no strict relation between them and information returned is as much as
fits both (IOW: at most vec_len ranges spanning not more than
max_out_page pages). The field's name and description I'd suggest
improving: maybe 'max_pages' with a comment that 0 =3D unlimited?

[...]
> >> /* Special flags */
> >> #define PAGEMAP_NO_REUSED_REGIONS       0x1
> >
> > What does this flag do?
> Some non-dirty pages get marked as dirty because of the kernel's
> internal activity. The dirty bit of the pages is stored in the VMA flags
> and in the per page flags. If any of these two bits are set, the page is
> considered to be dirty. Suppose you have cleared the dirty bit of half
> of VMA which will be done by splitting the VMA and clearing dirty flag
> in the half VMA and the pages in it. Now kernel may decide to merge the
> VMAs again as dirty bit of VMAs isn't considered if the VMAs should be
> merged. So the half VMA becomes dirty again. This splitting/merging
> costs performance. The application receives a lot of pages which aren't
> dirty in reality but marked as dirty. Performance is lost again here.
>
> This PAGEMAP_NO_REUSED_REGIONS flag is used to don't depend on the dirty
> flag in the VMA flags. It only depends on the individual page dirty bit.
> With doing this, the new memory regions which are just created, doesn't
> look like dirty when seen with the IOCTL, but look dirty when seen from
> pagemap. This seems okay as the user of this flag know the implication
> of using it.

Thanks for explaining! Could you include this as a comment in the patch?

Best Regards
Micha=C5=82 Miros=C5=82aw