Date:   Mon, 30 Jan 2023 16:34:49 -0500
From:   Peter Xu <peterx@redhat.com>
To:     Muhammad Usama Anjum <usama.anjum@collabora.com>
Cc:     David Hildenbrand <david@redhat.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        =?utf-8?B?TWljaGHFgiBNaXJvc8WCYXc=?= <emmir@google.com>,
        Andrei Vagin <avagin@gmail.com>,
        Danylo Mocherniuk <mdanylo@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 v8 3/4] fs/proc/task_mmu: Implement IOCTL to get and/or
 the clear info about PTEs
Message-ID: <Y9g3+RdfYIH3N47i@x1n>
References: <20230124084323.1363825-1-usama.anjum@collabora.com>
 <20230124084323.1363825-4-usama.anjum@collabora.com>
 <Y9QLqY0mQ6eD6Zm+@x1n>
 <01bc1428-5340-5a75-6876-417194edba39@collabora.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <01bc1428-5340-5a75-6876-417194edba39@collabora.com>
Precedence: bulk

On Mon, Jan 30, 2023 at 04:12:25PM +0500, Muhammad Usama Anjum wrote:

[...]

> > 
> >> +#define IS_WP_ENGAGE_OP(a)	(a->flags & PAGEMAP_WP_ENGAGE)
> >> +#define IS_GET_OP(a)		(a->vec)
> > 
> > Having a->vec to imply the GET is fine, but IMHO not as clean as having
> > each OP a bit in the flags.
> > 
> > How about:
> > 
> > #define  PAGEMAP_OP_GET       (1UL << 0)
> > #define  PAGEMAP_OP_WP        (1UL << 1)
> > #define  PAGEMAP_OP_MASK      (PAGEMAP_OP_GET | PAGEMAP_OP_WP)
> > 
> > ?
> > 
> > Then a->vec requried for PAGEMAP_OP_GET.
> I had something like PAGEMAP_OP_GET and PAGEMAP_OP_WP flags once until I
> was asked to only keep WP flag and vec != NULL will imply GET flag in a
> previous review. I'm ready to change again if there are more notes on it?

Sorry to know that; that's somewhat frustrating when you need to go back
and forth on subjective comments like mine.  So if you still think your
original way is better then at least you have two votes now. :) Your call
to choose any, I have a preference as I said but not that strong.

[...]

> > 
> >> +			     struct pagemap_scan_private *p, unsigned long addr, unsigned int len)
> >> +{
> >> +	unsigned long bitmap, cur = PAGEMAP_SCAN_BITMAP(wt, file, pres, swap);
> >> +	bool cpy = true;
> >> +	struct page_region *prev = &p->prev;
> >> +
> >> +	if (HAS_NO_SPACE(p))
> >> +		return -ENOSPC;
> > 
> > This can be moved to below [1], we should stop scanning immediately if the
> > condition met.
> It would be definately possible. Wouldn't it be strange to return error
> when the operation was successful? A function should return error only when
> the current execution is unsuccessful. So I'm returning error only when the
> space is full and there is no space left in the buffer anymore.

I would expect the user to always provide some more space than they expect
because the merging of page_regions are kind of unpredictable from the
user's POV.

But yeah, maybe you're right.  I'm fine to keep that as is.

Thanks,

-- 
Peter Xu