RE: vm_pager_(de)allocate and vm_mtx
On 26-May-01 Dima Dorfman wrote: > Is there a reason vm_pager_allocate acquires vm_mtx itself if > necessary but vm_pager_deallocate does not? At the moment, detaching > an md(4) disk will panic the system with a failed mtx_assert in > vm_pager_deallocate. This can be fixed one of two ways: > vm_pager_deallocate could be made to deal with vm_mtx itself like > vm_pager_allocate does, or md(4) and any other drivers which call > vm_pager_deallocate can be fixed to acquire vm_mtx. So which will it > be? I'll supply patches for either case. > > Thanks, I think I have it the same in the patches on my laptop, but I've not finished those yet, so they aren't safe to be committed but are still a WIP. I would grab the lock around vm_pager_deallocate() for now. -- John Baldwin <[EMAIL PROTECTED]> -- http://www.FreeBSD.org/~jhb/ PGP Key: http://www.Baldwin.cx/~john/pgpkey.asc "Power Users Use the Power to Serve!" - http://www.FreeBSD.org/ To Unsubscribe: send mail to [EMAIL PROTECTED] with "unsubscribe freebsd-current" in the body of the message
Re: vm_pager_(de)allocate and vm_mtx
Alfred Perlstein wrote: > * Dima Dorfman <[EMAIL PROTECTED]> [010525 22:22] wrote: > > Is there a reason vm_pager_allocate acquires vm_mtx itself if > > necessary but vm_pager_deallocate does not? At the moment, detaching > > an md(4) disk will panic the system with a failed mtx_assert in > > vm_pager_deallocate. This can be fixed one of two ways: > > vm_pager_deallocate could be made to deal with vm_mtx itself like > > vm_pager_allocate does, or md(4) and any other drivers which call > > vm_pager_deallocate can be fixed to acquire vm_mtx. So which will it > > be? I'll supply patches for either case. > > Usually fixing the caller is better as it will catch people that > expect vm state to remain unchanged across several calls. It is a bit late now, but if we're going to push this to callers, I wish we had set up VM_LOCK() and VM_UNLOCK() macros or something so that we dont have to deal with fallout if something there changes (or gets conditionalized). At least use the macros outside of the vm/* and pmap.c files... On the same train of thought, perhaps GIANT_LOCK() and GIANT_UNLOCK() might have been an idea too. It would certainly have changed the code impact when the mutex api changed last time. It also gives us better 4.x porting targets because we can provide stub (do nothing) GIANT_LOCK()/UNLOCK() macros on 4.x. OK, we could do the same for mtx_lock()/unlock() too but I think the macros give us more flexability should we want to globally change the giant lock implementation at some point (or even locally add instrumentation for testing or whatever). Cheers, -Peter -- Peter Wemm - [EMAIL PROTECTED]; [EMAIL PROTECTED]; [EMAIL PROTECTED] "All of this is for nothing if we don't go to the stars" - JMS/B5 To Unsubscribe: send mail to [EMAIL PROTECTED] with "unsubscribe freebsd-current" in the body of the message
Re: vm_pager_(de)allocate and vm_mtx
In message <[EMAIL PROTECTED]>, Dima Dorfman write
s:
>Alfred Perlstein <[EMAIL PROTECTED]> writes:
>> * Dima Dorfman <[EMAIL PROTECTED]> [010525 22:22] wrote:
>> > Is there a reason vm_pager_allocate acquires vm_mtx itself if
>> > necessary but vm_pager_deallocate does not? At the moment, detaching
>> > an md(4) disk will panic the system with a failed mtx_assert in
>> > vm_pager_deallocate. This can be fixed one of two ways:
>> > vm_pager_deallocate could be made to deal with vm_mtx itself like
>> > vm_pager_allocate does, or md(4) and any other drivers which call
>> > vm_pager_deallocate can be fixed to acquire vm_mtx. So which will it
>> > be? I'll supply patches for either case.
>>
>> Usually fixing the caller is better as it will catch people that
>> expect vm state to remain unchanged across several calls.
>
>Patch to fix md(4) attached. Look okay?
Looks fine, go ahead and commit.
Poul-Henning
>
> Dima Dorfman
> [EMAIL PROTECTED]
>
>Index: md.c
>===
>RCS file: /stl/src/FreeBSD/src/sys/dev/md/md.c,v
>retrieving revision 1.33
>diff -u -r1.33 md.c
>--- md.c 2001/05/21 18:52:00 1.33
>+++ md.c 2001/05/26 05:48:57
>@@ -711,8 +711,11 @@
> (void)vn_close(sc->vnode, sc->flags & MD_READONLY ? FREAD :
>(FREAD|FWRITE), sc->cred, p);
> if (sc->cred != NULL)
> crfree(sc->cred);
>- if (sc->object != NULL)
>+ if (sc->object != NULL) {
>+ mtx_lock(&vm_mtx);
> vm_pager_deallocate(sc->object);
>+ mtx_unlock(&vm_mtx);
>+ }
> if (sc->secp != NULL) {
> for (u = 0; u < sc->nsect; u++)
> if ((uintptr_t)sc->secp[u] > 255)
>@@ -763,17 +766,20 @@
>* Note the truncation.
>*/
>
>+ mtx_lock(&vm_mtx);
> sc->secsize = PAGE_SIZE;
> sc->nsect = mdio->md_size / (PAGE_SIZE / DEV_BSIZE);
> sc->object = vm_pager_allocate(OBJT_SWAP, NULL, sc->secsize *
>(vm_offset_t)sc->nsect, VM_PROT_DEFAULT, 0);
> if (mdio->md_options & MD_RESERVE) {
> if (swap_pager_reserve(sc->object, 0, sc->nsect) < 0) {
> vm_pager_deallocate(sc->object);
>+ mtx_unlock(&vm_mtx);
> sc->object = NULL;
> mddestroy(sc, mdio, p);
> return(EDOM);
> }
> }
>+ mtx_unlock(&vm_mtx);
> error = mdsetcred(sc, p->p_ucred);
> if (error)
> mddestroy(sc, mdio, p);
>
>To Unsubscribe: send mail to [EMAIL PROTECTED]
>with "unsubscribe freebsd-current" in the body of the message
>
--
Poul-Henning Kamp | UNIX since Zilog Zeus 3.20
[EMAIL PROTECTED] | TCP/IP since RFC 956
FreeBSD committer | BSD since 4.3-tahoe
Never attribute to malice what can adequately be explained by incompetence.
To Unsubscribe: send mail to [EMAIL PROTECTED]
with "unsubscribe freebsd-current" in the body of the message
Re: vm_pager_(de)allocate and vm_mtx
Alfred Perlstein <[EMAIL PROTECTED]> writes:
> * Dima Dorfman <[EMAIL PROTECTED]> [010525 22:22] wrote:
> > Is there a reason vm_pager_allocate acquires vm_mtx itself if
> > necessary but vm_pager_deallocate does not? At the moment, detaching
> > an md(4) disk will panic the system with a failed mtx_assert in
> > vm_pager_deallocate. This can be fixed one of two ways:
> > vm_pager_deallocate could be made to deal with vm_mtx itself like
> > vm_pager_allocate does, or md(4) and any other drivers which call
> > vm_pager_deallocate can be fixed to acquire vm_mtx. So which will it
> > be? I'll supply patches for either case.
>
> Usually fixing the caller is better as it will catch people that
> expect vm state to remain unchanged across several calls.
Patch to fix md(4) attached. Look okay?
Dima Dorfman
[EMAIL PROTECTED]
Index: md.c
===
RCS file: /stl/src/FreeBSD/src/sys/dev/md/md.c,v
retrieving revision 1.33
diff -u -r1.33 md.c
--- md.c2001/05/21 18:52:00 1.33
+++ md.c2001/05/26 05:48:57
@@ -711,8 +711,11 @@
(void)vn_close(sc->vnode, sc->flags & MD_READONLY ? FREAD :
(FREAD|FWRITE), sc->cred, p);
if (sc->cred != NULL)
crfree(sc->cred);
- if (sc->object != NULL)
+ if (sc->object != NULL) {
+ mtx_lock(&vm_mtx);
vm_pager_deallocate(sc->object);
+ mtx_unlock(&vm_mtx);
+ }
if (sc->secp != NULL) {
for (u = 0; u < sc->nsect; u++)
if ((uintptr_t)sc->secp[u] > 255)
@@ -763,17 +766,20 @@
* Note the truncation.
*/
+ mtx_lock(&vm_mtx);
sc->secsize = PAGE_SIZE;
sc->nsect = mdio->md_size / (PAGE_SIZE / DEV_BSIZE);
sc->object = vm_pager_allocate(OBJT_SWAP, NULL, sc->secsize *
(vm_offset_t)sc->nsect, VM_PROT_DEFAULT, 0);
if (mdio->md_options & MD_RESERVE) {
if (swap_pager_reserve(sc->object, 0, sc->nsect) < 0) {
vm_pager_deallocate(sc->object);
+ mtx_unlock(&vm_mtx);
sc->object = NULL;
mddestroy(sc, mdio, p);
return(EDOM);
}
}
+ mtx_unlock(&vm_mtx);
error = mdsetcred(sc, p->p_ucred);
if (error)
mddestroy(sc, mdio, p);
To Unsubscribe: send mail to [EMAIL PROTECTED]
with "unsubscribe freebsd-current" in the body of the message
Re: vm_pager_(de)allocate and vm_mtx
* Dima Dorfman <[EMAIL PROTECTED]> [010525 22:22] wrote: > Is there a reason vm_pager_allocate acquires vm_mtx itself if > necessary but vm_pager_deallocate does not? At the moment, detaching > an md(4) disk will panic the system with a failed mtx_assert in > vm_pager_deallocate. This can be fixed one of two ways: > vm_pager_deallocate could be made to deal with vm_mtx itself like > vm_pager_allocate does, or md(4) and any other drivers which call > vm_pager_deallocate can be fixed to acquire vm_mtx. So which will it > be? I'll supply patches for either case. Usually fixing the caller is better as it will catch people that expect vm state to remain unchanged across several calls. -- -Alfred Perlstein [[EMAIL PROTECTED]] Instead of asking why a piece of software is using "1970s technology," start asking why software is ignoring 30 years of accumulated wisdom. To Unsubscribe: send mail to [EMAIL PROTECTED] with "unsubscribe freebsd-current" in the body of the message
