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

[Yorick Hardy <yorickhardy%gmail.com@localhost>]

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