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