Messages in this thread | | | Date | Wed, 11 Mar 2009 15:25:05 +0200 | Subject | Re: [PATCH 1/2] mm: use list.h for vma list | From | Daniel Lowengrub <> |
| |
On Wed, Mar 11, 2009 at 1:54 PM, Nick Piggin <nickpiggin@yahoo.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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |