On 17/02/22(Thu) 16:22, Klemens Nanni wrote:
> On Wed, Feb 16, 2022 at 11:39:19PM +0100, Mark Kettenis wrote:
> > > Date: Wed, 16 Feb 2022 21:13:03 +0000
> > > From: Klemens Nanni <[email protected]>
> > >
> > > Unmodified -current with WITNESS enabled booting into X on my X230:
> > >
> > > wsdisplay0: screen 1-5 added (std, vt100 emulation)
> > > witness: acquiring duplicate lock of same type: "&uobj->vmobjlock"
> > > 1st uobjlk
> > > 2nd uobjlk
> > > Starting stack trace...
> > > witness_checkorder(fffffd83b625f9b0,9,0) at witness_checkorder+0x8ac
> > > rw_enter(fffffd83b625f9a0,1) at rw_enter+0x68
> > > uvm_obj_wire(fffffd843c39e948,0,40000,ffff800033b70428) at
> > > uvm_obj_wire+0x46
> > > shmem_get_pages(ffff800008008500) at shmem_get_pages+0xb8
> > > __i915_gem_object_get_pages(ffff800008008500) at
> > > __i915_gem_object_get_pages+0x6d
> > > i915_gem_fault(ffff800008008500,ffff800033b707c0,10009b000,a43d6b1c000,ffff800033b70740,1,35ba896911df1241,ffff8000000aa078,ffff8000000aa178)
> > > at i915_gem_fault+0x203
> > > drm_fault(ffff800033b707c0,a43d6b1c000,ffff800033b70740,1,0,0,7eca45006f70ee0,ffff800033b707c0)
> > > at drm_fault+0x156
> > > uvm_fault(fffffd843a7cf480,a43d6b1c000,0,2) at uvm_fault+0x179
> > > upageflttrap(ffff800033b70920,a43d6b1c000) at upageflttrap+0x62
> > > usertrap(ffff800033b70920) at usertrap+0x129
> > > recall_trap() at recall_trap+0x8
> > > end of kernel
> > > end trace frame: 0x7f7ffffdc7c0, count: 246
> > > End of stack trace.
> > >
> > > The system works fine (unless booted with kern.witness.watch=3), so I'm
> > > posting it here for reference -- haven't had time to look into this.
> >
> > Yes, this is expected. The graphics buffers are implented as a uvm
> > object and this object is backed by an anonymous memory uvm_object
> > (aobj). So I think the vmobjlock needs a RW_DUPOK flag.
>
> I see, thanks for the hint.
>
> I looked at drm first to see if I could easily add RW_DUPOK to their
> init/enter calls only such that RW_DUPOK for objlk is contained within
> drm, but that's neither easy nor needed.
>
> uvm_obj_wire() is only called from sys/dev/pci/drm/ anyway, so we can
> just treat drm there.
>
> The lock order reversal is about uvm_obj_wire() only and I haven't seen
> one in uvm_obj_unwire(), but my diff consequently adds RW_DUPOK to both
> as both are being used in drm.
>
> This makes the witness report go away on my X230.
>
> Does that RW_DUPOK deserve a comment?
If the commit message is clear enough I don't think so.
> Feedback? Objections? OK?
ok mpi@
> > > wsdisplay0: screen 1-5 added (std, vt100 emulation)
> > > witness: acquiring duplicate lock of same type: "&uobj->vmobjlock"
> > > 1st uobjlk
> > > 2nd uobjlk
> > > Starting stack trace...
> > > witness_checkorder(fffffd83b625f9b0,9,0) at witness_checkorder+0x8ac
> > > rw_enter(fffffd83b625f9a0,1) at rw_enter+0x68
> > > uvm_obj_wire(fffffd843c39e948,0,40000,ffff800033b70428) at
> > > uvm_obj_wire+0x46
> > > shmem_get_pages(ffff800008008500) at shmem_get_pages+0xb8
> > > __i915_gem_object_get_pages(ffff800008008500) at
> > > __i915_gem_object_get_pages+0x6d
> > > i915_gem_fault(ffff800008008500,ffff800033b707c0,10009b000,a43d6b1c000,ffff800033b70740,1,35ba896911df1241,ffff8000000aa078,ffff8000000aa178)
> > > at i915_gem_fault+0x203
> > > drm_fault(ffff800033b707c0,a43d6b1c000,ffff800033b70740,1,0,0,7eca45006f70ee0,ffff800033b707c0)
> > > at drm_fault+0x156
> > > uvm_fault(fffffd843a7cf480,a43d6b1c000,0,2) at uvm_fault+0x179
> > > upageflttrap(ffff800033b70920,a43d6b1c000) at upageflttrap+0x62
> > > usertrap(ffff800033b70920) at usertrap+0x129
> > > recall_trap() at recall_trap+0x8
> > > end of kernel
> > > end trace frame: 0x7f7ffffdc7c0, count: 246
> > > End of stack trace.
>
>
> Index: uvm_object.c
> ===================================================================
> RCS file: /cvs/src/sys/uvm/uvm_object.c,v
> retrieving revision 1.24
> diff -u -p -r1.24 uvm_object.c
> --- uvm_object.c 17 Jan 2022 13:55:32 -0000 1.24
> +++ uvm_object.c 17 Feb 2022 16:12:54 -0000
> @@ -133,7 +133,7 @@ uvm_obj_wire(struct uvm_object *uobj, vo
>
> left = (end - start) >> PAGE_SHIFT;
>
> - rw_enter(uobj->vmobjlock, RW_WRITE);
> + rw_enter(uobj->vmobjlock, RW_WRITE | RW_DUPOK);
> while (left) {
>
> npages = MIN(FETCH_PAGECOUNT, left);
> @@ -147,7 +147,7 @@ uvm_obj_wire(struct uvm_object *uobj, vo
> if (error)
> goto error;
>
> - rw_enter(uobj->vmobjlock, RW_WRITE);
> + rw_enter(uobj->vmobjlock, RW_WRITE | RW_DUPOK);
> for (i = 0; i < npages; i++) {
>
> KASSERT(pgs[i] != NULL);
> @@ -197,7 +197,7 @@ uvm_obj_unwire(struct uvm_object *uobj,
> struct vm_page *pg;
> off_t offset;
>
> - rw_enter(uobj->vmobjlock, RW_WRITE);
> + rw_enter(uobj->vmobjlock, RW_WRITE | RW_DUPOK);
> uvm_lock_pageq();
> for (offset = start; offset < end; offset += PAGE_SIZE) {
> pg = uvm_pagelookup(uobj, offset);
>
