Re: CVS commit: src/sys/uvm
On Nov 24, 2010, at 10:47 PM, Masao Uebayashi wrote: > On Thu, Nov 25, 2010 at 05:44:21AM +, YAMAMOTO Takashi wrote: >> hi, >> >>> On Thu, Nov 25, 2010 at 04:18:25AM +, YAMAMOTO Takashi wrote: hi, > Hi, thanks for review. > > On Thu, Nov 25, 2010 at 01:58:04AM +, YAMAMOTO Takashi wrote: >> hi, >> >> - what's VM_PHYSSEG_OP_PG? > > It's to lookup vm_physseg by "struct vm_page *", relying on that > "struct vm_page *[]" is allocated linearly. It'll be used to remove > vm_page::phys_addr as we talked some time ago. i'm not sure if commiting this unused uncommented code now helps it. some try-and-benchmark cycles might be necessary given that vm_page <-> paddr conversion could be performace critical. >>> >>> If you really care performance, we can directly pass "struct vm_page >>> *" to pmap_enter(). >>> >>> We're doing "struct vm_page *" -> "paddr_t" just before pmap_enter(), >>> then doing "paddr_t" -> "vm_physseg" reverse lookup again in >>> pmap_enter() to check if a given PA is managed. What is really >>> needed here is, to lookup "struct vm_page *" -> "vm_physseg" once >>> and you'll know both paddr_t and managed or not. >> >> i agree that the current code is not ideal in that respect. >> otoh, i'm not sure if passing vm_physseg around is a good idea. > > It's great you share the interest. > > I chose vm_physseg, because it was there. I'm open to alternatives, > but I don't think you have many options... Passing vm_page * doesn't work if the page isn't managed since there won't be a vm_page for the paddr_t. Now passing both paddr_t and vm_page * would work and if the pointer to the vm_page, it would be an unmanaged mapping. This also gives the access to mdpg without another lookup.
Re: CVS commit: src/sys/uvm
On Thu, Nov 25, 2010 at 05:44:21AM +, YAMAMOTO Takashi wrote: > hi, > > > On Thu, Nov 25, 2010 at 04:18:25AM +, YAMAMOTO Takashi wrote: > >> hi, > >> > >> > Hi, thanks for review. > >> > > >> > On Thu, Nov 25, 2010 at 01:58:04AM +, YAMAMOTO Takashi wrote: > >> >> hi, > >> >> > >> >> - what's VM_PHYSSEG_OP_PG? > >> > > >> > It's to lookup vm_physseg by "struct vm_page *", relying on that > >> > "struct vm_page *[]" is allocated linearly. It'll be used to remove > >> > vm_page::phys_addr as we talked some time ago. > >> > >> i'm not sure if commiting this unused uncommented code now helps it. > >> some try-and-benchmark cycles might be necessary given that > >> vm_page <-> paddr conversion could be performace critical. > > > > If you really care performance, we can directly pass "struct vm_page > > *" to pmap_enter(). > > > > We're doing "struct vm_page *" -> "paddr_t" just before pmap_enter(), > > then doing "paddr_t" -> "vm_physseg" reverse lookup again in > > pmap_enter() to check if a given PA is managed. What is really > > needed here is, to lookup "struct vm_page *" -> "vm_physseg" once > > and you'll know both paddr_t and managed or not. > > i agree that the current code is not ideal in that respect. > otoh, i'm not sure if passing vm_physseg around is a good idea. It's great you share the interest. I chose vm_physseg, because it was there. I'm open to alternatives, but I don't think you have many options...
Re: CVS commit: src/sys/uvm
hi, > On Thu, Nov 25, 2010 at 04:18:25AM +, YAMAMOTO Takashi wrote: >> hi, >> >> > Hi, thanks for review. >> > >> > On Thu, Nov 25, 2010 at 01:58:04AM +, YAMAMOTO Takashi wrote: >> >> hi, >> >> >> >> - what's VM_PHYSSEG_OP_PG? >> > >> > It's to lookup vm_physseg by "struct vm_page *", relying on that >> > "struct vm_page *[]" is allocated linearly. It'll be used to remove >> > vm_page::phys_addr as we talked some time ago. >> >> i'm not sure if commiting this unused uncommented code now helps it. >> some try-and-benchmark cycles might be necessary given that >> vm_page <-> paddr conversion could be performace critical. > > If you really care performance, we can directly pass "struct vm_page > *" to pmap_enter(). > > We're doing "struct vm_page *" -> "paddr_t" just before pmap_enter(), > then doing "paddr_t" -> "vm_physseg" reverse lookup again in > pmap_enter() to check if a given PA is managed. What is really > needed here is, to lookup "struct vm_page *" -> "vm_physseg" once > and you'll know both paddr_t and managed or not. i agree that the current code is not ideal in that respect. otoh, i'm not sure if passing vm_physseg around is a good idea. YAMAMOTO Takashi > >> >> YAMAMOTO Takashi > > -- > Masao Uebayashi / Tombi Inc. / Tel: +81-90-9141-4635
Re: CVS commit: src/sys/uvm
On Thu, Nov 25, 2010 at 04:18:25AM +, YAMAMOTO Takashi wrote: > hi, > > > Hi, thanks for review. > > > > On Thu, Nov 25, 2010 at 01:58:04AM +, YAMAMOTO Takashi wrote: > >> hi, > >> > >> - what's VM_PHYSSEG_OP_PG? > > > > It's to lookup vm_physseg by "struct vm_page *", relying on that > > "struct vm_page *[]" is allocated linearly. It'll be used to remove > > vm_page::phys_addr as we talked some time ago. > > i'm not sure if commiting this unused uncommented code now helps it. > some try-and-benchmark cycles might be necessary given that > vm_page <-> paddr conversion could be performace critical. If you really care performance, we can directly pass "struct vm_page *" to pmap_enter(). We're doing "struct vm_page *" -> "paddr_t" just before pmap_enter(), then doing "paddr_t" -> "vm_physseg" reverse lookup again in pmap_enter() to check if a given PA is managed. What is really needed here is, to lookup "struct vm_page *" -> "vm_physseg" once and you'll know both paddr_t and managed or not. > > YAMAMOTO Takashi -- Masao Uebayashi / Tombi Inc. / Tel: +81-90-9141-4635
Re: CVS commit: src/sys/uvm
hi, > Hi, thanks for review. > > On Thu, Nov 25, 2010 at 01:58:04AM +, YAMAMOTO Takashi wrote: >> hi, >> >> - what's VM_PHYSSEG_OP_PG? > > It's to lookup vm_physseg by "struct vm_page *", relying on that > "struct vm_page *[]" is allocated linearly. It'll be used to remove > vm_page::phys_addr as we talked some time ago. i'm not sure if commiting this unused uncommented code now helps it. some try-and-benchmark cycles might be necessary given that vm_page <-> paddr conversion could be performace critical. YAMAMOTO Takashi
Re: CVS commit: src/sys/uvm
Hi, thanks for review. On Thu, Nov 25, 2010 at 01:58:04AM +, YAMAMOTO Takashi wrote: > hi, > > - what's VM_PHYSSEG_OP_PG? It's to lookup vm_physseg by "struct vm_page *", relying on that "struct vm_page *[]" is allocated linearly. It'll be used to remove vm_page::phys_addr as we talked some time ago. > - why *offp calculation is done in _p functions, rather than the caller? Hmmm. I can't remember any reason, except it's similar to the original code. It's better to do in callers, yes. Masao > > YAMAMOTO Takashi > > > Module Name:src > > Committed By: uebayasi > > Date: Sun Nov 14 15:06:34 UTC 2010 > > > > Modified Files: > > src/sys/uvm: uvm_page.c uvm_page.h uvm_pglist.c > > > > Log Message: > > Be a little more friendly to dynamic physical segment registration. > > > > Maintain an array of pointer to struct vm_physseg, instead of struct > > array. So that VM subsystem can take its pointer safely. Pointer > > to this struct will replace raw paddr_t usage in the future. > > > > Dynamic removal is not supported yet. > > > > Only MD data structure changes, no kernel bump needed. > > > > Tested on i386, amd64, powerpc/ibm40x, arm11. > > > > > > To generate a diff of this commit: > > cvs rdiff -u -r1.163 -r1.164 src/sys/uvm/uvm_page.c > > cvs rdiff -u -r1.66 -r1.67 src/sys/uvm/uvm_page.h > > cvs rdiff -u -r1.46 -r1.47 src/sys/uvm/uvm_pglist.c > > > > Please note that diffs are not public domain; they are subject to the > > copyright notices on the relevant files. -- Masao Uebayashi / Tombi Inc. / Tel: +81-90-9141-4635
Re: CVS commit: src/sys/uvm
hi, - what's VM_PHYSSEG_OP_PG? - why *offp calculation is done in _p functions, rather than the caller? YAMAMOTO Takashi > Module Name: src > Committed By: uebayasi > Date: Sun Nov 14 15:06:34 UTC 2010 > > Modified Files: > src/sys/uvm: uvm_page.c uvm_page.h uvm_pglist.c > > Log Message: > Be a little more friendly to dynamic physical segment registration. > > Maintain an array of pointer to struct vm_physseg, instead of struct > array. So that VM subsystem can take its pointer safely. Pointer > to this struct will replace raw paddr_t usage in the future. > > Dynamic removal is not supported yet. > > Only MD data structure changes, no kernel bump needed. > > Tested on i386, amd64, powerpc/ibm40x, arm11. > > > To generate a diff of this commit: > cvs rdiff -u -r1.163 -r1.164 src/sys/uvm/uvm_page.c > cvs rdiff -u -r1.66 -r1.67 src/sys/uvm/uvm_page.h > cvs rdiff -u -r1.46 -r1.47 src/sys/uvm/uvm_pglist.c > > Please note that diffs are not public domain; they are subject to the > copyright notices on the relevant files.
Re: CVS commit: src/usr.bin/stat
On Nov 24, 11:49pm, dholland-sourcechan...@netbsd.org (David Holland) wrote: -- Subject: Re: CVS commit: src/usr.bin/stat | On Wed, Nov 24, 2010 at 11:11:36PM +, Christos Zoulas wrote: | > >- (void)snprintf(tmp, sizeof(tmp), "%dd", prec > 9 ? 9 : prec); | > >+ (void)snprintf(tmp, sizeof(tmp), "%dld", prec > 9 ? 9 : prec); | > | > perhaps %dlld? | | Nope, that's the nsecs, which are "long". | | I'm sort of inclined to rework the whole thing to make it clearer, but | I'm not sure it'd actually be an improvement. Ok, just making sure :-) christos
Re: NetBSD source-changes Digest V2 #1781
On Wed, Nov 24, 2010 at 04:08:50PM -0800, Paul Goyette wrote: > Hmmm, did something change recently? yeah, the source-changes digest has started going to current-users instead of the source-changes digest subscribers. I believe admins have figured out why this is happening and are looking into how it got that way, but I could be wrong. Anyway, it should stop soon. -- David A. Holland dholl...@netbsd.org
Re: CVS commit: src/sys/kern
hi,
> Hello,
>
> Sorry for late reply.
>
> y...@mwd.biglobe.ne.jp (YAMAMOTO Takashi) wrote:
>> >>>
>> >>> how about the attached patch?
>> >>
>> >> Consider offset = (INT64_MAX - PAGE_SIZE) and len = PAGE_SIZE. That
>> >> would still panic..
>> >
>> > heh, right.
>> >
>> > then, how about:
>> >if (round_page(offset) < trunc_page(endoffset)) {
>>
>> && offset < round_page(offset)
>
> I think that should be correct, except there is off-by-one (since offset
> at PAGE_SIZE boundary is valid). Should be:
>
> ... && offset <= round_page(offset)
heh, sure.
>
> Do you want to commit this?
if you can test and commit, please.
YAMAMOTO Takashi
>
> --
> Mindaugas
Re: NetBSD source-changes Digest V2 #1781
On Wed, 24 Nov 2010, NetBSD source-changes Digest wrote: NetBSD source-changes Digest Wednesday, November 24 2010 Volume 02 : Number 1781 ... Hmmm, did something change recently? I used to be subscribed only to source-changes but for the last couple of days I've also been getting the digest. Is this intentional? And if so, how can I unsubscribe from digest while still getting the usual stuff? - | Paul Goyette | PGP Key fingerprint: | E-mail addresses: | | Customer Service | FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com| | Network Engineer | 0786 F758 55DE 53BA 7731 | pgoyette at juniper.net | | Kernel Developer | | pgoyette at netbsd.org | -
Re: CVS commit: src/usr.bin/stat
On Wed, Nov 24, 2010 at 11:11:36PM +, Christos Zoulas wrote: > >- (void)snprintf(tmp, sizeof(tmp), "%dd", prec > 9 ? 9 : prec); > >+ (void)snprintf(tmp, sizeof(tmp), "%dld", prec > 9 ? 9 : prec); > > perhaps %dlld? Nope, that's the nsecs, which are "long". I'm sort of inclined to rework the whole thing to make it clearer, but I'm not sure it'd actually be an improvement. -- David A. Holland dholl...@netbsd.org
Re: CVS commit: src/usr.bin/stat
In article <20101124225754.6201e17...@cvs.netbsd.org>, David A. Holland wrote: >- (void)snprintf(tmp, sizeof(tmp), "%dd", prec > 9 ? 9 : prec); >+ (void)snprintf(tmp, sizeof(tmp), "%dld", prec > 9 ? 9 : prec); perhaps %dlld? > (void)strcat(lfmt, tmp); >- l = snprintf(buf, blen, lfmt, secs, nsecs); >+ l = snprintf(buf, blen, lfmt, (long long)secs, nsecs); christos
Re: CVS commit: src/sys/dev
On Wed, Nov 24, 2010 at 12:15:59PM +0100, Juergen Hannken-Illjes wrote: > > > +kmutex_t sc_lock; /* Protect self. */ > > > +kcondvar_t sc_cv; /* Signal work. */ > > > > I think "Signal work" is missleading. :) > > No. It DOES signal work to the umem server. But it's not for work on signals... what about "Wait here for work"? -- David A. Holland dholl...@netbsd.org
Re: CVS commit: src/bin/dd
David Holland writes: > Adding weird special case remote access hacks to dd (of all random > tools) is also poor design. Why not for the next round add support for > >dd ifurl=http://www.netbsd.org/index.html of=mycopy > > or > >dd ifurl=http://www.netbsd.org/ recurse=true of=mytree/ > > After all, all you have to do to implement this is open a pipe to ftp > or wget. Actually, neat idea (at least the first one)... -- Jim Wise jw...@draga.com pgpTopM5vOIzq.pgp Description: PGP signature
Re: CVS commit: src/sys/dev
On Wed, Nov 24, 2010 at 02:40:45AM +, Mindaugas Rasiukevicius wrote:
> Hello,
>
> "Juergen Hannken-Illjes" wrote:
> > Module Name:src
> > Committed By: hannken
> > Date: Tue Nov 23 09:30:43 UTC 2010
> >
> > Modified Files:
> > src/sys/dev: md.c
> >
> > Log Message:
> > Make md(4) mp-safe.
> >
> >
> > To generate a diff of this commit:
> > cvs rdiff -u -r1.64 -r1.65 src/sys/dev/md.c
>
> Few comments:
>
> > @@ -597,15 +626,18 @@ md_server_loop(struct md_softc *sc)
> > int error;
> > bool is_read;
> >
> > + KASSERT(mutex_owned(&sc->sc_lock));
> > +
> > for (;;) {
> > /* Wait for some work to arrive. */
> > while ((bp = bufq_get(sc->sc_buflist)) == NULL) {
> > - error = tsleep((void *)sc, md_sleep_pri, "md_idle", 0);
> > + error = cv_wait_sig(&sc->sc_cv, &sc->sc_lock);
> > <...>
> > biodone(bp);
> > + mutex_enter(&sc->sc_lock);
> > }
>
> Is this (as well as other parts of code) are safe in respect of mdclose()?
> For example, what happens if other thread executes close(2) while the lock
> is dropped here?
The last close will detach (and drain the queue). In the UMEM_SERVER case
the umem server (the thread running the ioctl) has to close before we
detach on last close.
> > @@ -383,7 +396,8 @@ mdstrategy(struct buf *bp)
> > case MD_UMEM_SERVER:
> > /* Just add this job to the server's queue. */
> > bufq_put(sc->sc_buflist, bp);
> > - wakeup((void *)sc);
> > + cv_signal(&sc->sc_cv);
> > + mutex_exit(&sc->sc_lock);
>
> It should be cv_broadcast(9).
No. There is only one possible waiter (the umem server thread).
> > @@ -421,6 +435,8 @@ mdstrategy(struct buf *bp)
> > }
> > done:
> > biodone(bp);
> > +
> > + mutex_exit(&sc->sc_lock);
>
> Any reason why biodone() is under lock?
No. Will fix. See diff attached.
> > @@ -534,6 +561,8 @@ md_ioctl_kalloc(struct md_softc *sc, str
> > vaddr_t addr;
> > vsize_t size;
> >
> > + KASSERT(mutex_owned(&sc->sc_lock));
>
> Ideally, allocations should be outside the locks (just FYI).
Ok. Will fix. See diff attached.
> > + kmutex_t sc_lock; /* Protect self. */
> > + kcondvar_t sc_cv; /* Signal work. */
>
> I think "Signal work" is missleading. :)
No. It DOES signal work to the umem server.
> --
> Mindaugas
Btw.: The KMEM server was and is fishy. The memory will never be freed.
--
Juergen Hannken-Illjes - hann...@eis.cs.tu-bs.de - TU Braunschweig (Germany)
Index: md.c
===
RCS file: /cvsroot/src/sys/dev/md.c,v
retrieving revision 1.65
diff -p -u -2 -r1.65 md.c
--- md.c23 Nov 2010 09:30:43 - 1.65
+++ md.c24 Nov 2010 10:15:48 -
@@ -434,8 +434,9 @@ mdstrategy(struct buf *bp)
break;
}
- done:
- biodone(bp);
+ done:
mutex_exit(&sc->sc_lock);
+
+ biodone(bp);
}
@@ -562,12 +563,21 @@ md_ioctl_kalloc(struct md_softc *sc, str
vsize_t size;
- KASSERT(mutex_owned(&sc->sc_lock));
+ mutex_exit(&sc->sc_lock);
/* Sanity check the size. */
size = umd->md_size;
addr = uvm_km_alloc(kernel_map, size, 0, UVM_KMF_WIRED|UVM_KMF_ZERO);
+
+ mutex_enter(&sc->sc_lock);
+
if (!addr)
return ENOMEM;
+ /* If another thread beat us to configure this unit: fail. */
+ if (sc->sc_type != MD_UNCONFIGURED) {
+ uvm_km_free(kernel_map, addr, size, UVM_KMF_WIRED);
+ return EINVAL;
+ }
+
/* This unit is now configured. */
sc->sc_addr = (void *)addr; /* kernel space */
