Dear Chuck, On 2020-11-29, Chuck Silvers wrote: > 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.
I am sorry to have made more work for you, and thanks for looking at it! I agree, and since I have only a superficial understanding of how everything fits together I am not 100% sure that my reading is correct. > could you explain the specific case that you think is wrong now and > that your patch fixes? I will look at the tests as suggested below. Please read/ignore this part as you see fit. Superficially (and perhaps that is part of the "not reading correctly"!) 1) if we reach http://nxr.netbsd.org/xref/src/sys/uvm/uvm_map.c#1479 then one of two things happened (with merged == 1): a) an amap was extended b) neither prev_entry nor prev_entry->next have an amap 2) now we reach https://nxr.netbsd.org/xref/src/sys/uvm/uvm_map.c#1493 and since merged == 1 in (1) https://nxr.netbsd.org/xref/src/sys/uvm/uvm_map.c#1497 UVMMAP_EVCNT_DECR(ubackmerge); UVMMAP_EVCNT_INCR(ubimerge); but in case 1(b) the forward merge never happened? That was the starting point of my query. I thought perhaps that 1(b) never happens, but a printf shows that it happens often. I am "living dangerously" and running with the patch for now, although I am not sure yet where to look for any adverse effects. > 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). I did not think I could do that, I will look into it! (Energy is low again so, when I get to it ...) > 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 Understood, it is quite possible that I am way out of my depth! -- Kind regards, Yorick Hardy