On Wed, Oct 07, 2020 at 04:49:28PM +0200, Martin Pieuchot wrote:
> On 01/10/20(Thu) 14:18, Martin Pieuchot wrote:
> > Use more KASSERT()s instead of the "if (x) panic()" idiom for sanity
> > checks and add a couple of local variables to reduce the difference
> > with NetBSD and help for upcoming locking.
>
> deraadt@ mentioned that KASSERT()s are not effective in RAMDISK kernels.
>
> So the revisited diff below only converts checks that are redundant with
> NULL dereferences.
>
> ok?
globally ok. but there is one problem, see below.
> Index: uvm/uvm_amap.c
> ===================================================================
> RCS file: /cvs/src/sys/uvm/uvm_amap.c,v
> retrieving revision 1.84
> diff -u -p -r1.84 uvm_amap.c
> --- uvm/uvm_amap.c 25 Sep 2020 08:04:48 -0000 1.84
> +++ uvm/uvm_amap.c 7 Oct 2020 14:40:53 -0000
> @@ -669,9 +669,7 @@ ReStart:
> pg = anon->an_page;
>
> /* page must be resident since parent is wired */
> - if (pg == NULL)
> - panic("amap_cow_now: non-resident wired page"
> - " in anon %p", anon);
> + KASSERT(pg != NULL);
>
> /*
> * if the anon ref count is one, we are safe (the child
> @@ -740,6 +738,7 @@ ReStart:
> void
> amap_splitref(struct vm_aref *origref, struct vm_aref *splitref, vaddr_t
> offset)
> {
> + struct vm_amap *amap = origref->ar_amap;
> int leftslots;
>
> AMAP_B2SLOT(leftslots, offset);
> @@ -747,17 +746,18 @@ amap_splitref(struct vm_aref *origref, s
> panic("amap_splitref: split at zero offset");
>
> /* now: we have a valid am_mapped array. */
> - if (origref->ar_amap->am_nslot - origref->ar_pageoff - leftslots <= 0)
> + if (amap->am_nslot - origref->ar_pageoff - leftslots <= 0)
> panic("amap_splitref: map size check failed");
>
> #ifdef UVM_AMAP_PPREF
> - /* establish ppref before we add a duplicate reference to the amap */
> - if (origref->ar_amap->am_ppref == NULL)
> - amap_pp_establish(origref->ar_amap);
> + /* Establish ppref before we add a duplicate reference to the amap.
> */
> + if (amap->am_ppref == NULL)
> + amap_pp_establish(amap);
> #endif
>
> - splitref->ar_amap = origref->ar_amap;
> - splitref->ar_amap->am_ref++; /* not a share reference */
> + /* Note: not a share reference. */
> + amap->am_ref++;
> + splitref->ar_amap = amap;
> splitref->ar_pageoff = origref->ar_pageoff + leftslots;
> }
>
> @@ -1104,12 +1104,11 @@ amap_add(struct vm_aref *aref, vaddr_t o
>
> slot = UVM_AMAP_SLOTIDX(slot);
> if (replace) {
> - if (chunk->ac_anon[slot] == NULL)
> - panic("amap_add: replacing null anon");
> - if (chunk->ac_anon[slot]->an_page != NULL &&
> - (amap->am_flags & AMAP_SHARED) != 0) {
> - pmap_page_protect(chunk->ac_anon[slot]->an_page,
> - PROT_NONE);
> + struct vm_anon *oanon = chunk->ac_anon[slot];
> +
> + KASSERT(oanon != NULL);
> + if (oanon->an_page && (amap->am_flags & AMAP_SHARED) != 0) {
> + pmap_page_protect(oanon->an_page, PROT_NONE);
> /*
> * XXX: suppose page is supposed to be wired somewhere?
> */
> @@ -1138,14 +1137,13 @@ amap_unadd(struct vm_aref *aref, vaddr_t
>
> AMAP_B2SLOT(slot, offset);
> slot += aref->ar_pageoff;
> - KASSERT(slot < amap->am_nslot);
> + if (chunk->ac_anon[slot] == NULL)
> + panic("amap_unadd: nothing there");
this change seems wrong. you're removing a KASSERT() and readd an
explicit panic(9) (taken from few lines after)
> chunk = amap_chunk_get(amap, slot, 0, PR_NOWAIT);
> - if (chunk == NULL)
> - panic("amap_unadd: chunk for slot %d not present", slot);
> + KASSERT(chunk != NULL);
>
> slot = UVM_AMAP_SLOTIDX(slot);
> - if (chunk->ac_anon[slot] == NULL)
> - panic("amap_unadd: nothing there");
> + KASSERT(chunk->ac_anon[slot] != NULL);
>
> chunk->ac_anon[slot] = NULL;
> chunk->ac_usedmap &= ~(1 << slot);
>
Thanks.
--
Sebastien Marie
