On Sun, Jan 31, 2010 at 05:43:02PM +0500, Anton Maksimenkov wrote:
> Here is some cleanup of uvm_map.c code.
>
> At first, in uvm_map_advice() function.
> Let it looks as simple as uvm_map_inherit() - no need to lock/unlock map
> just to realize that sanity check fail. Also no need to do it in every loop.
The functional change is that a wrong advice on an empty range will now
return EINVAL instead of 0. I actually like that.
> Second, remove redundant "temp_entry" variable from both functions.
> One "entry" variable is enough to do the job.
I agree temp_entry is a redundant variable in both functions. I think
the compiler may already optimize that away (otoh, we are talking gcc
here...)
> $ cvs diff -Nup sys/uvm/uvm_map.c
> Index: sys/uvm/uvm_map.c
> ===================================================================
> RCS file: /cvs/src/sys/uvm/uvm_map.c,v
> retrieving revision 1.123
> diff -u -p -r1.123 uvm_map.c
> --- sys/uvm/uvm_map.c 28 Aug 2009 00:40:03 -0000 1.123
> +++ sys/uvm/uvm_map.c 31 Jan 2010 11:35:56 -0000
> @@ -2473,7 +2473,7 @@ int
> uvm_map_inherit(struct vm_map *map, vaddr_t start, vaddr_t end,
> vm_inherit_t new_inheritance)
> {
> - struct vm_map_entry *entry, *temp_entry;
> + struct vm_map_entry *entry;
> UVMHIST_FUNC("uvm_map_inherit"); UVMHIST_CALLED(maphist);
> UVMHIST_LOG(maphist,"(map=%p,start=0x%lx,end=0x%lx,new_inh=0x%lx)",
> map, start, end, new_inheritance);
> @@ -2492,11 +2492,10 @@ uvm_map_inherit(struct vm_map *map, vadd
>
> VM_MAP_RANGE_CHECK(map, start, end);
>
> - if (uvm_map_lookup_entry(map, start, &temp_entry)) {
> - entry = temp_entry;
> + if (uvm_map_lookup_entry(map, start, &entry)) {
> UVM_MAP_CLIP_START(map, entry, start);
> } else {
> - entry = temp_entry->next;
> + entry = entry->next;
> }
>
> while ((entry != &map->header) && (entry->start < end)) {
> @@ -2519,18 +2518,29 @@ uvm_map_inherit(struct vm_map *map, vadd
> int
> uvm_map_advice(struct vm_map *map, vaddr_t start, vaddr_t end, int
> new_advice)
> {
> - struct vm_map_entry *entry, *temp_entry;
> + struct vm_map_entry *entry;
> UVMHIST_FUNC("uvm_map_advice"); UVMHIST_CALLED(maphist);
> UVMHIST_LOG(maphist,"(map=%p,start=0x%lx,end=0x%lx,new_adv=0x%lx)",
> map, start, end, new_advice);
>
> + switch (new_advice) {
> + case MADV_NORMAL:
> + case MADV_RANDOM:
> + case MADV_SEQUENTIAL:
> + /* nothing special here */
> + break;
> +
> + default:
> + UVMHIST_LOG(maphist,"<- done (INVALID ARG)",0,0,0,0);
> + return (EINVAL);
> + }
> +
> vm_map_lock(map);
> VM_MAP_RANGE_CHECK(map, start, end);
> - if (uvm_map_lookup_entry(map, start, &temp_entry)) {
> - entry = temp_entry;
> + if (uvm_map_lookup_entry(map, start, &entry)) {
> UVM_MAP_CLIP_START(map, entry, start);
> } else {
> - entry = temp_entry->next;
> + entry = entry->next;
> }
>
> /*
> @@ -2539,19 +2549,6 @@ uvm_map_advice(struct vm_map *map, vaddr
>
> while ((entry != &map->header) && (entry->start < end)) {
> UVM_MAP_CLIP_END(map, entry, end);
> -
> - switch (new_advice) {
> - case MADV_NORMAL:
> - case MADV_RANDOM:
> - case MADV_SEQUENTIAL:
> - /* nothing special here */
> - break;
> -
> - default:
> - vm_map_unlock(map);
> - UVMHIST_LOG(maphist,"<- done (INVALID ARG)",0,0,0,0);
> - return (EINVAL);
> - }
> entry->advice = new_advice;
> entry = entry->next;
> }
Ok with me.
--
Ariane
