linux-kernel - Re: [PATCH 1/2] mm: use list.h for vma list

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <8c5a844a0903110625y416e7a3ft448a44b1bf70c990@mail.gmail.com>
Date:	Wed, 11 Mar 2009 15:25:05 +0200
From:	Daniel Lowengrub <lowdanie@...il.com>
To:	Nick Piggin <nickpiggin@...oo.com.au>
Cc:	Ingo Molnar <mingo@...e.hu>, linux-kernel@...r.kernel.org,
	linux-mm@...ck.org
Subject: Re: [PATCH 1/2] mm: use list.h for vma list

On Wed, Mar 11, 2009 at 1:54 PM, Nick Piggin <nickpiggin@...oo.com.au> wrote:
> On Wednesday 11 March 2009 20:55:48 Daniel Lowengrub wrote:
>> diff -uNr linux-2.6.28.7.vanilla/arch/arm/mm/mmap.c
>> linux-2.6.28.7/arch/arm/mm/mmap.c
>>.....
>> -     for (vma = find_vma(mm, addr); ; vma = vma->vm_next) {
>> +     for (vma = find_vma(mm, addr); ; vma = vma->vma_next(vma)) {
>>               /* At this point:  (!vma || addr < vma->vm_end). */
>>               if (TASK_SIZE - len < addr) {
>>                       /*
>
> Careful with your replacements. I'd suggest a mechanical search &
> replace might be less error prone.

Thanks for pointing that out.  The code compiled and ran on my x86 machine
so I'll take an extra look at the other architectures.

>> linux-2.6.28.7/include/linux/mm.h
>> --- linux-2.6.28.7.vanilla/include/linux/mm.h 2009-03-06
>>...
>> +/* Interface for the list_head prev and next pointers.  They
>> + * don't let you wrap around the vm_list.
>> + */
>
> Hmm, I don't think these are really appropriate replacements for
> vma->vm_next. 2 branches and a lot of extra icache.
>
> A non circular list like hlist might work better, but I suspect if
> callers are converted properly to have conditions ensuring that it
> doesn't wrap and doesn't get NULL vmas passed in, then it could
> avoid both those branches and just be a wrapper around
> list_entry(vma->vm_list.next)
>
The main place I can think of where "list_entry(vma->vm_list.next)"
can be used without the extra conditionals is inside a loop where
we're going through every vma in the
list.  This is usually done  with "list_for_each_entry" which uses
"list_entry(...)" anyway.
But in all the places that we start from some point inside the list
(usually with a find_vma)
a regular "for" list is used with "vma_next" as the last parameter.
In this case it would
probably be better to use "list_for_each_entry_continue" which would
lower the amount of pointless calls to "vma_next".
The first condition in vma_next also does away with the excessive use
of the ternary operator in the mmap.c file.  Where else in the code
would it be faster to use
"list_entry(...)" together with conditionals?
I'll look through the code again with all this in mind and see if
calls to the vma_next function can be minimized to the point of
removing it like
you said.

>>  struct mm_struct {
>> -     struct vm_area_struct * mmap;           /* list of VMAs */
>> +     struct list_head mm_vmas;               /* list of VMAs */

>.... like this nice name change ;)
This and other parts of the patch are based on a previous attempt by
Paul Zijlstra.

>> @@ -988,7 +989,8 @@
>>       lru_add_drain();
>>       tlb = tlb_gather_mmu(mm, 0);
>>       update_hiwater_rss(mm);
>> -     end = unmap_vmas(&tlb, vma, address, end, &nr_accounted, details);
>> +     end = unmap_vmas(&tlb, &mm->mm_vmas, vma, address, end,
>> +                     &nr_accounted, details);
>
> Why do you change this if the caller knows where the list head is
> anyway, and extracts it from the mm? I'd prefer to keep changes to
> calling convention to a minimum (and I hope with the changes to
> vma_next I suggested then it wouldn't be needed to carry the list
> head around everywhere anyway).
>
The unmap_vmas was changed because sometimes (in exit_mmap for example)
"unmap_vmas" is used right after "detach_vmas_to_be_unmapped" which
now returns a list of the vmas we want to unmap.  Now that we already
have this list for free it seems like a good idea to be able to pass
it to "unmap_vmas".  Do you think that this causes
more damage than it's worth?
After reading what you said before, it looks like we could take better
advantage of this
if we use "list_entry(...) in unmap_vmas's main loop instead of a
regular for loop
with __vma_next.
Thank you for the helpful suggestions.
--
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/