Date:   Tue, 24 Jan 2023 14:49:18 -0500
From:   Peter Xu <peterx@redhat.com>
To:     Muhammad Usama Anjum <usama.anjum@collabora.com>
Cc:     Andrei Vagin <avagin@gmail.com>,
        Danylo Mocherniuk <mdanylo@google.com>,
        David Hildenbrand <david@redhat.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        =?utf-8?B?TWljaGHFgiBNaXJvc8WCYXc=?= <emmir@google.com>,
        Paul Gofman <pgofman@codeweavers.com>,
        Cyrill Gorcunov <gorcunov@gmail.com>,
        Alexander Viro <viro@zeniv.linux.org.uk>,
        Shuah Khan <shuah@kernel.org>,
        Christian Brauner <brauner@kernel.org>,
        Yang Shi <shy828301@gmail.com>,
        Vlastimil Babka <vbabka@suse.cz>,
        "Liam R . Howlett" <Liam.Howlett@oracle.com>,
        Yun Zhou <yun.zhou@windriver.com>,
        Suren Baghdasaryan <surenb@google.com>,
        Alex Sierra <alex.sierra@amd.com>,
        Matthew Wilcox <willy@infradead.org>,
        Pasha Tatashin <pasha.tatashin@soleen.com>,
        Mike Rapoport <rppt@kernel.org>, Nadav Amit <namit@vmware.com>,
        Axel Rasmussen <axelrasmussen@google.com>,
        "Gustavo A . R . Silva" <gustavoars@kernel.org>,
        Dan Williams <dan.j.williams@intel.com>,
        linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org,
        linux-mm@kvack.org, linux-kselftest@vger.kernel.org,
        Greg KH <gregkh@linuxfoundation.org>, kernel@collabora.com
Subject: Re: [PATCH v7 0/4] Implement IOCTL to get and/or the clear info
 about PTEs
Message-ID: <Y9A2PsCS7gfKWfaM@x1n>
References: <20230109064519.3555250-1-usama.anjum@collabora.com>
 <Y8hutCGec6je5toG@x1n>
 <0eb79bb3-7384-11c6-a380-c027f09305f2@collabora.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <0eb79bb3-7384-11c6-a380-c027f09305f2@collabora.com>
Precedence: bulk

On Mon, Jan 23, 2023 at 06:15:00PM +0500, Muhammad Usama Anjum wrote:
> > Firstly, doc update is more than welcomed to explain the new interface
> > first (before throwing the code..).  That can be done in pagemap.rst on
> > pagemap changes, or userfaultfd.rst on userfaultfd.
> Okay. I'll add the documentation in next version or after the series has
> been accepted. Initially I'd added the documentation. But the code kept on
> changing so much that I had to spend considerable time on updating the
> documentation. I know it is better to add documentation with the patches.
> I'll try to add it.

Yes, logically it should be the thing people start looking with.  It'll
help reviewers to understand how does it work in general if relevant
description is not in the cover letter, so it can matter even before the
series is merged.

> > There're four kinds of masks (required/anyof/excluded/return).  Are they
> > all needed?  Why this is a good interface design?
> Then, CRIU developers Andrea [1] and Danylo [2], asked to include all these
> different kinds of masks. I'd thought of these masks as fancy filter inside
> the kernel. But there wasn't anyone else to review. So I'd included them to
> move forward. Please let me know your thoughts after reading emails from [1].

The idea makes sense to me, thanks.  I just hope "moving it forward" is not
the only reason that you included it.

Please also consider to attach relevant links to your next cover letter so
new reviewers can be aware of why the interface is proposed like that.

IMHO it would be also great if the CRIU people can acknowledge the
interface at some point to make sure it satisfies the needs.  An POC would
be even better on CRIU, but maybe that's asking too much.

-- 
Peter Xu