On 8/11/23 6:32 PM, Michał Mirosław wrote:
> On Fri, Aug 11, 2023 at 05:02:44PM +0500, Muhammad Usama Anjum wrote:
>> On 8/10/23 10:32 PM, Michał Mirosław wrote:
>>> On Wed, 9 Aug 2023 at 08:16, Muhammad Usama Anjum
>>> <usama.anjum@xxxxxxxxxxxxx> wrote:
> [...]
>>>> --- a/fs/proc/task_mmu.c
>>>> +++ b/fs/proc/task_mmu.c
>>> [...]
>>>> +#ifdef CONFIG_TRANSPARENT_HUGEPAGE
>>>> +static unsigned long pagemap_thp_category(pmd_t pmd)
>>>> +{
>>>> + unsigned long categories = PAGE_IS_HUGE;
>>>> +
>>>> + if (pmd_present(pmd)) {
>>>> + categories |= PAGE_IS_PRESENT;
>>>> + if (!pmd_uffd_wp(pmd))
>>>> + categories |= PAGE_IS_WRITTEN;
>>>> + if (is_zero_pfn(pmd_pfn(pmd)))
>>>> + categories |= PAGE_IS_PFNZERO;
>>>> + } else if (is_swap_pmd(pmd)) {
>>>> + categories |= PAGE_IS_SWAPPED;
>>>> + if (!pmd_swp_uffd_wp(pmd))
>>>> + categories |= PAGE_IS_WRITTEN;
>>>> + }
>>>> +
>>>> + return categories;
>>>> +}
>>> I guess THPs can't be file-backed currently, but can we somehow mark
>>> this assumption so it can be easily found if the capability arrives?
>> Yeah, THPs cannot be file backed. Lets not care for some feature which may
>> not arrive in several years or eternity.
>
> Yes, it might not arrive. But please add at least a comment, so that it
> is clearly visible that lack if PAGE_IS_FILE here is intentional.
I'll add a comment.
>
>>>> +#endif /* CONFIG_TRANSPARENT_HUGEPAGE */
>>>> +
>>>> +#ifdef CONFIG_HUGETLB_PAGE
>>>> +static unsigned long pagemap_hugetlb_category(pte_t pte)
>>>> +{
>>>> + unsigned long categories = PAGE_IS_HUGE;
>>>> +
>>>> + if (pte_present(pte)) {
>>>> + categories |= PAGE_IS_PRESENT;
>>>> + if (!huge_pte_uffd_wp(pte))
>>>> + categories |= PAGE_IS_WRITTEN;
>>>> + if (!PageAnon(pte_page(pte)))
>>>> + categories |= PAGE_IS_FILE;
>>>> + if (is_zero_pfn(pte_pfn(pte)))
>>>> + categories |= PAGE_IS_PFNZERO;
>>>> + } else if (is_swap_pte(pte)) {
>>>> + categories |= PAGE_IS_SWAPPED;
>>>> + if (!pte_swp_uffd_wp_any(pte))
>>>> + categories |= PAGE_IS_WRITTEN;
>>>> + }
>>>
>>> BTW, can a HugeTLB page be file-backed and swapped out?
>> Accourding to pagemap_hugetlb_range(), file-backed HugeTLB page cannot be
>> swapped.
>
> Here too a comment that leaving out this case is intentional would be useful.
Sure.
>
>>> [...]
>>>> + walk_start = p.arg.start;
>>>> + for (; walk_start < p.arg.end; walk_start = p.arg.walk_end) {
> [...[
>>>> + ret = mmap_read_lock_killable(mm);
>>>> + if (ret)
>>>> + break;
>>>> + ret = walk_page_range(mm, walk_start, p.arg.end,
>>>> + &pagemap_scan_ops, &p);
>>>> + mmap_read_unlock(mm);
> [...]
>>>> + if (ret != -ENOSPC || p.arg.vec_len - 1 == 0 ||
>>>> + p.found_pages == p.arg.max_pages)
>>>> + break;
>>>
>>> The second condition is equivalent to `p.arg.vec_len == 1`, but why is
>>> that an ending condition? Isn't the last entry enough to gather one
>>> more range? (The walk could have returned -ENOSPC due to buffer full
>>> and after flushing it could continue with the last free entry.)
>> Now we are walking the entire range walk_page_range(). We don't break loop
>> when we get -ENOSPC as this error may only mean that the temporary buffer
>> is full. So we need check if max pages have been found or output buffer is
>> full or ret is 0 or any other error. When p.arg.vec_len = 1 is end
>> condition as the last entry is in cur. As we have walked over the entire
>> range, cur must be full after which the walk returned.
>>
>> So current condition is necessary. I've double checked it. I'll change it
>> to `p.arg.vec_len == 1`.
>
> If we have walked the whole range, then the loop will end anyway due to
> `walk_start < walk_end` not held in the `for()`'s condition.
Sorry, for not explaining to-the-point.
Why would we walk the entire range when we should recognize that the output
buffer is full and break the loop?
I've test cases written for this case. If I remove `p.arg.vec_len == 1`
check, there is infinite loop for walking. So we are doing correct thing here.
>
> [...]
>>>> +/*
>>>> + * struct pm_scan_arg - Pagemap ioctl argument
>>>> + * @size: Size of the structure
>>>> + * @flags: Flags for the IOCTL
>>>> + * @start: Starting address of the region
>>>> + * @end: Ending address of the region
>>>> + * @walk_end Address where the scan stopped (written by kernel).
>>>> + * walk_end == end informs that the scan completed on entire range.
>>>
>>> Can we ensure this holds also for the tagged pointers?
>> No, we cannot.
>
> So this need explanation in the comment here. (Though I'd still like to
> know how the address tags are supposed to be used from someone that
> knows them.)
I've looked at some documentations (presentations/talks) about tags. Tags
is more like userspace feature. Kernel should just ignore them for our use
case. I'll add comment.
>
> Best Regards
> Michał Mirosław
--
BR,
Muhammad Usama Anjum
