Re: uvm_map_enter entry merging (was Re: vrelel...)

[Chuck Silvers <chuq%chuq.com@localhost>]

hi Yorick,

On Sat, Nov 28, 2020 at 12:39:56AM +0200, Yorick Hardy wrote:
> May I ask if you have an opinion on this patch? I have
> not noticed any bad behaviour if it is omitted but, if I read
> the code correctly, I don't think it is correct to fall through
> for this case.

this function is very hard to follow, it's very tangled.
I stared at it for a while and I didn't see anything wrong,
but it's hard to be sure just from reading the code.

could you explain the specific case that you think is wrong now and
that your patch fixes?

even better would be if you could write a set of atf tests to exercise
all of the possible merge cases and verify that the contents of memory
after the new mapping is created is what it should be.
any previous and next mapping should have the same contents as before,
and the new mapping should have either zeroes (for a new amap mapping)
or the uobj contents at that offset (for a new uobj mapping).

note that a vm_map_entry can reference both a uobj and an amap at the
same time, so there are 4 possible cases for the each of previous and next
entries (none, uobj, amap, uobj+amap), and two possible cases for the
new entry (uobj, amap).  then I guess there are two more factors of 2
for whether the forward and/or backward merges succeed, so that gives
at least 128 cases to test.  I think there are some more cases hidden
in there because there are multiple reasons why the merges might fail
and those checks are in different places, so it would really be best
to test all of the different possible paths through this function.

I would be reluctant to change anything here without such a set of
comprehensive tests, because even if we are sure that a change fixes
one case, it would be very hard to be sure that it doesn't break
some other case.

-Chuck


> -- 
> Kind regards,
> 
> Yorick Hardy
> 
> Index: uvm_map.c
> ===================================================================
> RCS file: /cvsroot/src/sys/uvm/uvm_map.c,v
> retrieving revision 1.385
> diff -u -r1.385 uvm_map.c
> --- uvm_map.c	9 Jul 2020 05:57:15 -0000	1.385
> +++ uvm_map.c	19 Nov 2020 16:04:07 -0000
> @@ -1477,6 +1477,13 @@
>  				    amapwaitflag | AMAP_EXTEND_BACKWARDS))
>  					goto nomerge;
>  			}
> +
> +			/*
> +			 * We could not extend either amap, just skip on.
> +			 */
> +			else {
> +				goto nomerge;
> +			}
>  		} else {
>  			/*
>  			 * Pull the next entry's amap backwards to cover this