| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Mon, 27 Jul 2009 16:42:09 -0700
From: Moussa Ba <moussa.a.ba@...il.com>
To: David Rientjes <rientjes@...gle.com>
Cc: linux-kernel@...r.kernel.org, xiyou.wangcong@...il.com,
Andrew Morton <akpm@...ux-foundation.org>,
Alexey Dobriyan <adobriyan@...il.com>,
Matt Mackall <mpm@...enic.com>, Mel Gorman <mel@....ul.ie>,
Ying Han <yinghan@...gle.com>, Nick Piggin <npiggin@...e.de>,
jaredeh@...il.com
Subject: Re: [PATCH 1/1] pagemap clear_refs: modify to specify anon or mapped
vma clearing
On Mon, Jul 27, 2009 at 4:38 PM, Moussa Ba<moussa.a.ba@...il.com> wrote:
> On Mon, Jul 27, 2009 at 3:49 PM, David Rientjes<rientjes@...gle.com> wrote:
>> On Mon, 27 Jul 2009, Moussa Ba wrote:
>>
>>> > clear_refs currently accepts any non-zero value, so it's possible that
>>> > this will break user scripts that, for whatever reason, write '2' or '3'.
>>> > I think that's acceptable, but it would be helpful to make all other
>>> > values a no-op similar to drop_caches at this point to avoid the potential
>>> > for breakage if this is ever extended any further.
>>> >
>>
>> In your latest post, I see you implemented this in
>> clear_refs_walk_vma_area() by checking for
>> CLEAR_REFS_ALL > type > CLEAR_REFS_MAPPED. Thanks! Don't forget to
>> update Documentation/filesystems/proc.txt to specify that.
>>
>>> >> Selective clearing the pages has a measurable impact on performance as it
>>> >> limits the number of page walks. We have been using this interface and this
>>> >> adds flexibility to the user user space application implementing the reference
>>> >> clearing.
>>> >>
>>> >> Signed-off-by: Jared Hulbert (jaredeh@...il.com)
>>> >> Signed-off-by: Moussa A. Ba (moussa.a.ba@...il.com)
>>> >
>>> > Email addresses in < > braces, please.
>>> >
>>> > The first sign-off line normally indicates who wrote the patch, but your
>>> > submission lacks a From: line, so git would indicate you wrote it. If
>>> > that's incorrect, please add a From: line as described in
>>> > Documentation/SubmittingPatches. If it's correct, please reorder your
>>> > sign-off lines.
>>> >
>>> I will reorder the sign-off lines
>>
>> Ok, that removes the ambiguity concerning authorship, thanks.
>>
>>> >> -------
>>> >> Documentation/filesystems/proc.txt | 7 +++++++
>>> >> fs/proc/task_mmu.c | 29 +++++++++++++++++++++++++----
>>> >> 2 files changed, 32 insertions(+), 4 deletions(-)
>>> >>
>>> >> --- a/fs/proc/task_mmu.c 2009-07-21 14:30:01.000000000 -0700
>>> >> +++ b/fs/proc/task_mmu.c 2009-07-27 11:46:05.000000000 -0700
>>> >> @@ -462,6 +462,27 @@
>>> >> return 0;
>>> >> }
>>> >>
>>> >> +static void walk_vma_area(struct mm_walk *this_walk,
>>> >> + struct vm_area_struct *vma, int type)
>>> >> +{
>>> >
>>> > This is a very generic name for something that is only applicable to
>>> > clear_refs, so please name it accordingly. This will also avoid having to
>>> > pass the struct mm_walk * in since its only user is clear_refs_walk.
>>> >
>>> Done.
>>> >> +
>>> >> + /* Writing 2 to /proc/pid/clear_refs will clear all Anonymous
>>> >> + * pages.
>>> >> + *
>>> >> + * Writing 3 to /proc/pid/clear_refs will clear all file mapped
>>> >> + * pages.
>>> >> + *
>>> >> + * Writing any other value including 1 will clear all pages
>>> >> + */
>>> >
>>> > Documentation/CodingStyle would suggest this format:
>>> >
>>> > /*
>>> > * Multi-line kernel comments always start ..
>>> > * with an empty first line.
>>> > */
>>> >
>>> Done.
>>> >> + if (is_vm_hugetlb_page(vma))
>>> >> + return;
>>> >> + if (type == 2 && vma->vm_file)
>>> >> + return;
>>> >> + if (type == 3 && !vma->vm_file)
>>> >> + return;
>>> >> + walk_page_range(vma->vm_start, vma->vm_end, this_walk);
>>> >> +}
>>> >
>>> > K&R would suggest #define's (or enums) for those hard-coded values. I
>>> > think that's already been suggested for this patch, actually.
>>> >
>>>
>>> Would this be acceptable?
>>>
>>> enum clear_refs_walk_type {
>>> CLEAR_REFS_ALL = 1,
>>> CLEAR_REFS_ANON = 2,
>>> CLEAR_REFS_MAPPED = 3
>>> };
>>>
>>
>> #define seems more appropriate for this particular use case. We simply
>> try to avoid hard-coded integers in source code (rationale: K&R).
>>
>>> static void clear_refs_walk_vma_area(struct mm_walk *this_walk,
>>> struct vm_area_struct *vma, enum clear_refs_walk_type type)
>>> {
>>
>> Ddi you consider my suggestion of dropping the struct mm_walk * formal
>> since this is now a clear_refs-specific function? walk_page_range() will
>> always take clear_refs_walk, there's no need to pass it in.
>>
>
> Well, I did but I would have to either pass clear_refs_walk or mm and
> build the mm_walk entry inside clear_refs_walk_vma. Simply passing
> the clear_refs_walk structure seemed simpler and more logical than
> having to rebuild it inside the function every time it is called.
>
> The other alternative would be to just forgo the additional function
> clear_refs_walk_vma and rewrite the for loop as:
>
> for (vma = mm->mmap; vma; vma = vma->vm_next) {
> clear_refs_walk.private = vma;
> if (type < CLEAR_REFS_ALL || type > CLEAR_REFS_MAPPED)
> continue;
> if (is_vm_hugetlb_page(vma))
> continue;
> if (type == CLEAR_REFS_ANON && vma->vm_file)
> continue;
> if (type == CLEAR_REFS_MAPPED && !vma->vm_file)
> continue;
> walk_page_range(vma->vm_start, vma->vm_end, this_walk);
> }
>
>
Apologies the checking of the range of values should be outside the for loop.
if (type < CLEAR_REFS_ALL || type > CLEAR_REFS_MAPPED)
return -EINVAl;
for (vma = mm->mmap; vma; vma = vma->vm_next) {
clear_refs_walk.private = vma;
if (is_vm_hugetlb_page(vma))
continue;
if (type == CLEAR_REFS_ANON && vma->vm_file)
continue;
if (type == CLEAR_REFS_MAPPED && !vma->vm_file)
continue;
walk_page_range(vma->vm_start, vma->vm_end, this_walk);
}
>
>>>
>>> /*
>>> * Writing 1 to /proc/pid/clear_refs clears all pages.
>>> * Writing 2 to /proc/pid/clear_refs clears Anonymous pages.
>>> * Writing 3 to /proc/pid/clear_refs clears file mapped pages.
>>> */
>>> if (type < CLEAR_REFS_ALL || type > CLEAR_REFS_MAPPED)
>>> return;
>>> if (is_vm_hugetlb_page(vma))
>>> return;
>>> if (type == CLEAR_REFS_ANON && vma->vm_file)
>>> return;
>>> if (type == CLEAR_REFS_MAPPED && !vma->vm_file)
>>> return;
>>> walk_page_range(vma->vm_start, vma->vm_end, this_walk);
>>> }
>>>
>>>
>>> >> +
>>> >> static ssize_t clear_refs_write(struct file *file, const char __user * buf,
>>> >> size_t count, loff_t * ppos)
>>> >> {
>>> >> @@ -469,13 +490,15 @@
>>> >> char buffer[PROC_NUMBUF], *end;
>>> >> struct mm_struct *mm;
>>> >> struct vm_area_struct *vma;
>>> >> + int type;
>>> >>
>>> >> memset(buffer, 0, sizeof(buffer));
>>> >> if (count > sizeof(buffer) - 1)
>>> >> count = sizeof(buffer) - 1;
>>> >> if (copy_from_user(buffer, buf, count))
>>> >> return -EFAULT;
>>> >> - if (!simple_strtol(buffer, &end, 0))
>>> >> + type = strict_strtol(buffer, &end, 0);
>>> >> + if (!type)
>>> >> return -EINVAL;
>>> >> if (*end == '\n')
>>> >> end++;
>>> >> @@ -491,9 +514,7 @@
>>> >> down_read(&mm->mmap_sem);
>>> >> for (vma = mm->mmap; vma; vma = vma->vm_next) {
>>> >> clear_refs_walk.private = vma;
>>> >> - if (!is_vm_hugetlb_page(vma))
>>> >> - walk_page_range(vma->vm_start, vma->vm_end,
>>> >> - &clear_refs_walk);
>>> >> + walk_vma_area(&clear_refs_walk, vma, type);
>>> >> }
>>> >> flush_tlb_mm(mm);
>>> >> up_read(&mm->mmap_sem);
>>> >> --- a/Documentation/filesystems/proc.txt 2009-07-20 17:29:11.000000000
>>> >> -0700
>>> >> +++ b/Documentation/filesystems/proc.txt 2009-07-27 12:08:34.000000000
>>> >> -0700
>>> >> @@ -375,6 +375,13 @@
>>> >> This file is only present if the CONFIG_MMU kernel configuration option is
>>> >> enabled.
>>> >>
>>> >> +The clear_refs entry is used to reset the Referenced bits on virtual and physical
>>> >> +pages associated with a process.
>>> >> +echo 1 > /proc/PID/clear_refs clears all pages associated with the process
>>> >> +echo 2 > /proc/PID/clear_refs clears anonymous pages only
>>> >> +echo 3 > /proc/PID/clear_refs clears file mapped pages only
>>> >> +Any other value written to the proc entry will clear all pages.
>>> >> +
>>> >
>>> > Please follow the format in this document for how other /proc/PID/*
>>> > entries are described.
>>> >
>>> > That format could really be improved here, perhaps you could clean
>>> > proc.txt up a little bit while you're here?
>>> >
>>> >
>>>
>>> I am not sure what you mean by "clean" proc.txt, I did not detect much
>>> formatting in the PID proc enries description, beyond what I rewrote
>>> below:
>>>
>>
>> Right, the file is pretty sloppy, so I was wondering if you wanted to take
>> the time to clean it up a little so there's a more consistent style.
>>
>>>
>>> The /proc/PID/clear_refs is used to reset the Referenced bits on virtual and
>>
>> Probably better to say PG_referenced instead of Referenced.
>
> Okay, though it would have to also state that the pte bits are cleared as well.
>>
>>> physical pages associated with a process.
>>> To clear all pages associated with the process
>>> > echo 1 > /proc/PID/clear_refs
>>>
>>> To clear all anonymous pages associated with the process
>>> > echo 2 > /proc/PID/clear_refs
>>>
>>> To clear all file mapped pages associated with the process
>>> > echo 3 > /proc/PID/clear_refs
>>> Any other value written to /proc/PID/clear_refs will have no effect.
>>>
>>
>> You should probably say these all clear the bit instead of saying they
>> "clear pages," which doesn't make a lot of sense.
>
> You are correct.
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists