Re: CVS commit: src/sys/arch
On 24 February 2012 18:29, Manuel Bouyer wrote: > On Fri, Feb 24, 2012 at 06:14:11PM +0530, Cherry G. Mathew wrote: >> On 24 February 2012 15:33, Manuel Bouyer wrote: >> > On Fri, Feb 24, 2012 at 03:00:03PM +0530, Cherry G. Mathew wrote: >> >> On 22 February 2012 18:31, Manuel Bouyer wrote: >> >> > On Wed, Feb 22, 2012 at 06:05:21PM +0530, Cherry G. Mathew wrote: >> >> >> >> >> >> I meant we could make it work, (it would already for amd64/xen since >> >> >> cpu_init_msrs() is called from cpu_hatch()) since xen has its own cpu.c >> >> > >> >> > i don't know if we can do the same for i386. >> >> >> >> It wasn't fun, but I managed to do it. >> >> >> >> btw, do you see a gdt page leaked between machdep.c:initgdt() and >> >> gdt.c:gdt_init() ? >> > >> > I can't see initgdt(), did you remove it ? >> >> No, it's right there: >> http://nxr.netbsd.org/xref/src/sys/arch/i386/i386/machdep.c#1096 > > OK, I was looking in amd64 code. > Yes, it's quite possible that we waste a page here. > >> >> > >> >> >> >> > Also xpq_cpu() is time-critical; I guess a function pointer call is >> >> > faster >> >> > than a test. >> >> >> >> Well, as a bonus of the early %gs/%fs setup now, I'm thinking of >> >> pruning the xpq_queue_update_xxx() in favour of pmap_set_xxx(). Also, >> >> I'll revisiting the atomicity guarantees (eg: pmap_pte_cas() of these >> >> functions, once we only start using them. >> > >> > AFAIK they're already all used by pmap ? >> > >> >> Mostly, but not everywere. > > there are places where they're not used on purpose (e.g. because we > know taking the lock or raising the IPL is not needed). > I've made a few changes to pmap.c where it looks harmless to do so, but are in favour of consistency. ftp://ftp.netbsd.org/pub/NetBSD/misc/cherry/tmp/xen-set-pte.diff >> >> > What I want to look at is *why* they're used. In some case it's only >> > to collect PG_M/PG_D bits, and Xen has another, maybe more efficient >> > mechanism for that. This may allow us to batch more MMU updates. >> > >> > Also, I want to look using more multicalls. This may decrease the >> > number of hypercalls significantly. >> > >> >> I wonder if there's a way to align that with pmap(9) assumptions. >> Quoting the manpage: >> >> " In order to cope with hardware architectures that make the invalidation >> of virtual address mappings expensive (e.g., TLB invalidations, TLB >> shootdown operations for multiple processors), the pmap module is >> allowed >> to delay mapping invalidation or protection operations until such time >> as >> they are actually necessary. The functions that are allowed to delay >> such actions are pmap_enter(), pmap_remove(), pmap_protect(), >> pmap_kenter_pa(), and pmap_kremove(). Callers of these functions must >> use the pmap_update() function to notify the pmap module that the map- >> pings need to be made correct. Since the pmap module is provided with >> information as to which processors are using a given physical map, the >> pmap module may use whatever optimizations it has available to reduce >> the >> expense of virtual-to-physical mapping synchronization. >> " >> Since the XPQ can be regarded as a kind of TLB, I'm guessing we can >> attempt to marry the two apis ? > > This is more or less what I had in mind. But, for the cases where > we need atomic operations, pmap_update() is not appropriate ... > Very cool, Cheers, -- ~Cherry
Re: CVS commit: src/usr.bin/tip
In article <20120224082709.ga24...@britannica.bec.de>, Joerg Sonnenberger wrote: >On Fri, Feb 24, 2012 at 08:11:19AM +, David Laight wrote: >> On Thu, Feb 23, 2012 at 11:39:19PM +, Joerg Sonnenberger wrote: >> > Module Name: src >> > Committed By: joerg >> > Date: Thu Feb 23 23:39:19 UTC 2012 >> > >> > Modified Files: >> >src/usr.bin/tip: cmds.c >> > >> > Log Message: >> > while (...); >> > ; >> > is really pointless, so remove the first semicolon. >> >> I tend to use an explicit 'continue' in such loops. >> Much less confusing. > >I generally agree, but putting the semicolon on a separate line is good >enough for the purpose. I prefer the continue; it is easier to read. christos
Re: CVS commit: src/usr.bin/look
In article <4f473b48.1040...@netbsd.org>, Marc Balmer wrote: >Am 23.02.12 23:57, schrieb Joerg Sonnenberger: >> Module Name: src >> Committed By:joerg >> Date:Thu Feb 23 22:57:53 UTC 2012 >> >> Modified Files: >> src/usr.bin/look: look.c >> >> Log Message: >> Don't use while-loop with empty body. > >I see you did several such changes. What is the reason behind this, >i.e. what is wrong with such loops? They don't show the programmer's intend. I am still pissed off about me spending hours debugging something that did: if (condition); a += 3; But of course it was hidden in thousands of lines of code, so it was hard to find. christos
Re: CVS commit: src/sys/arch
On Fri, Feb 24, 2012 at 06:14:11PM +0530, Cherry G. Mathew wrote: > On 24 February 2012 15:33, Manuel Bouyer wrote: > > On Fri, Feb 24, 2012 at 03:00:03PM +0530, Cherry G. Mathew wrote: > >> On 22 February 2012 18:31, Manuel Bouyer wrote: > >> > On Wed, Feb 22, 2012 at 06:05:21PM +0530, Cherry G. Mathew wrote: > >> >> > >> >> I meant we could make it work, (it would already for amd64/xen since > >> >> cpu_init_msrs() is called from cpu_hatch()) since xen has its own cpu.c > >> > > >> > i don't know if we can do the same for i386. > >> > >> It wasn't fun, but I managed to do it. > >> > >> btw, do you see a gdt page leaked between machdep.c:initgdt() and > >> gdt.c:gdt_init() ? > > > > I can't see initgdt(), did you remove it ? > > No, it's right there: > http://nxr.netbsd.org/xref/src/sys/arch/i386/i386/machdep.c#1096 OK, I was looking in amd64 code. Yes, it's quite possible that we waste a page here. > > > > >> > >> > Also xpq_cpu() is time-critical; I guess a function pointer call is > >> > faster > >> > than a test. > >> > >> Well, as a bonus of the early %gs/%fs setup now, I'm thinking of > >> pruning the xpq_queue_update_xxx() in favour of pmap_set_xxx(). Also, > >> I'll revisiting the atomicity guarantees (eg: pmap_pte_cas() of these > >> functions, once we only start using them. > > > > AFAIK they're already all used by pmap ? > > > > Mostly, but not everywere. there are places where they're not used on purpose (e.g. because we know taking the lock or raising the IPL is not needed). > > > What I want to look at is *why* they're used. In some case it's only > > to collect PG_M/PG_D bits, and Xen has another, maybe more efficient > > mechanism for that. This may allow us to batch more MMU updates. > > > > Also, I want to look using more multicalls. This may decrease the > > number of hypercalls significantly. > > > > I wonder if there's a way to align that with pmap(9) assumptions. > Quoting the manpage: > > " In order to cope with hardware architectures that make the invalidation > of virtual address mappings expensive (e.g., TLB invalidations, TLB > shootdown operations for multiple processors), the pmap module is allowed > to delay mapping invalidation or protection operations until such time as > they are actually necessary. The functions that are allowed to delay > such actions are pmap_enter(), pmap_remove(), pmap_protect(), > pmap_kenter_pa(), and pmap_kremove(). Callers of these functions must > use the pmap_update() function to notify the pmap module that the map- > pings need to be made correct. Since the pmap module is provided with > information as to which processors are using a given physical map, the > pmap module may use whatever optimizations it has available to reduce the > expense of virtual-to-physical mapping synchronization. > " > Since the XPQ can be regarded as a kind of TLB, I'm guessing we can > attempt to marry the two apis ? This is more or less what I had in mind. But, for the cases where we need atomic operations, pmap_update() is not appropriate ... -- Manuel Bouyer NetBSD: 26 ans d'experience feront toujours la difference --
Re: CVS commit: src/sys/arch
On 24 February 2012 15:33, Manuel Bouyer wrote: > On Fri, Feb 24, 2012 at 03:00:03PM +0530, Cherry G. Mathew wrote: >> On 22 February 2012 18:31, Manuel Bouyer wrote: >> > On Wed, Feb 22, 2012 at 06:05:21PM +0530, Cherry G. Mathew wrote: >> >> >> >> I meant we could make it work, (it would already for amd64/xen since >> >> cpu_init_msrs() is called from cpu_hatch()) since xen has its own cpu.c >> > >> > i don't know if we can do the same for i386. >> >> It wasn't fun, but I managed to do it. >> >> btw, do you see a gdt page leaked between machdep.c:initgdt() and >> gdt.c:gdt_init() ? > > I can't see initgdt(), did you remove it ? No, it's right there: http://nxr.netbsd.org/xref/src/sys/arch/i386/i386/machdep.c#1096 > >> >> > Also xpq_cpu() is time-critical; I guess a function pointer call is faster >> > than a test. >> >> Well, as a bonus of the early %gs/%fs setup now, I'm thinking of >> pruning the xpq_queue_update_xxx() in favour of pmap_set_xxx(). Also, >> I'll revisiting the atomicity guarantees (eg: pmap_pte_cas() of these >> functions, once we only start using them. > > AFAIK they're already all used by pmap ? > Mostly, but not everywere. > What I want to look at is *why* they're used. In some case it's only > to collect PG_M/PG_D bits, and Xen has another, maybe more efficient > mechanism for that. This may allow us to batch more MMU updates. > > Also, I want to look using more multicalls. This may decrease the > number of hypercalls significantly. > I wonder if there's a way to align that with pmap(9) assumptions. Quoting the manpage: " In order to cope with hardware architectures that make the invalidation of virtual address mappings expensive (e.g., TLB invalidations, TLB shootdown operations for multiple processors), the pmap module is allowed to delay mapping invalidation or protection operations until such time as they are actually necessary. The functions that are allowed to delay such actions are pmap_enter(), pmap_remove(), pmap_protect(), pmap_kenter_pa(), and pmap_kremove(). Callers of these functions must use the pmap_update() function to notify the pmap module that the map- pings need to be made correct. Since the pmap module is provided with information as to which processors are using a given physical map, the pmap module may use whatever optimizations it has available to reduce the expense of virtual-to-physical mapping synchronization. " Since the XPQ can be regarded as a kind of TLB, I'm guessing we can attempt to marry the two apis ? Cheers, -- ~Cherry
Re: CVS commit: src/sys/arch
On Fri, Feb 24, 2012 at 03:00:03PM +0530, Cherry G. Mathew wrote: > On 22 February 2012 18:31, Manuel Bouyer wrote: > > On Wed, Feb 22, 2012 at 06:05:21PM +0530, Cherry G. Mathew wrote: > >> > >> I meant we could make it work, (it would already for amd64/xen since > >> cpu_init_msrs() is called from cpu_hatch()) since xen has its own cpu.c > > > > i don't know if we can do the same for i386. > > It wasn't fun, but I managed to do it. > > btw, do you see a gdt page leaked between machdep.c:initgdt() and > gdt.c:gdt_init() ? I can't see initgdt(), did you remove it ? > > > Also xpq_cpu() is time-critical; I guess a function pointer call is faster > > than a test. > > Well, as a bonus of the early %gs/%fs setup now, I'm thinking of > pruning the xpq_queue_update_xxx() in favour of pmap_set_xxx(). Also, > I'll revisiting the atomicity guarantees (eg: pmap_pte_cas() of these > functions, once we only start using them. AFAIK they're already all used by pmap ? What I want to look at is *why* they're used. In some case it's only to collect PG_M/PG_D bits, and Xen has another, maybe more efficient mechanism for that. This may allow us to batch more MMU updates. Also, I want to look using more multicalls. This may decrease the number of hypercalls significantly. -- Manuel Bouyer NetBSD: 26 ans d'experience feront toujours la difference --
Re: CVS commit: src/sys/arch
On 22 February 2012 18:31, Manuel Bouyer wrote: > On Wed, Feb 22, 2012 at 06:05:21PM +0530, Cherry G. Mathew wrote: >> >> I meant we could make it work, (it would already for amd64/xen since >> cpu_init_msrs() is called from cpu_hatch()) since xen has its own cpu.c > > i don't know if we can do the same for i386. It wasn't fun, but I managed to do it. btw, do you see a gdt page leaked between machdep.c:initgdt() and gdt.c:gdt_init() ? > Also xpq_cpu() is time-critical; I guess a function pointer call is faster > than a test. Well, as a bonus of the early %gs/%fs setup now, I'm thinking of pruning the xpq_queue_update_xxx() in favour of pmap_set_xxx(). Also, I'll revisiting the atomicity guarantees (eg: pmap_pte_cas() of these functions, once we only start using them. Cheers, -- ~Cherry
Re: CVS commit: src/sys/arch/x86/x86
Hi Thomas, On 24 February 2012 14:21, Thomas Klausner wrote: > Hi Cherry! > On Fri, Feb 24, 2012 at 08:44:45AM +, Cherry G. Mathew wrote: >> Module Name: src >> Committed By: cherry >> Date: Fri Feb 24 08:44:44 UTC 2012 >> >> Modified Files: >> src/sys/arch/x86/x86: pmap.c >> >> Log Message: >> kernel page attribute change should be reflected on all cpus, since >> the page is going to be released after the _dtor() hook is called. > > Can you please explain why you reverted the previous version and now use > xen_bcast*? > Thomas That was a typo that I committed by mistake. xpq_bcast*() does not exist. Sorry about that. Cheers, -- ~Cherry
Re: CVS commit: src/sys/arch/x86/x86
Hi Cherry! On Fri, Feb 24, 2012 at 08:44:45AM +, Cherry G. Mathew wrote: > Module Name: src > Committed By: cherry > Date: Fri Feb 24 08:44:44 UTC 2012 > > Modified Files: > src/sys/arch/x86/x86: pmap.c > > Log Message: > kernel page attribute change should be reflected on all cpus, since > the page is going to be released after the _dtor() hook is called. Can you please explain why you reverted the previous version and now use xen_bcast*? Thomas
Re: CVS commit: src/usr.bin/tip
On Fri, Feb 24, 2012 at 09:27:09AM +0100, Joerg Sonnenberger wrote:
> On Fri, Feb 24, 2012 at 08:11:19AM +, David Laight wrote:
> > On Thu, Feb 23, 2012 at 11:39:19PM +, Joerg Sonnenberger wrote:
> > > Module Name: src
> > > Committed By: joerg
> > > Date: Thu Feb 23 23:39:19 UTC 2012
> > >
> > > Modified Files:
> > > src/usr.bin/tip: cmds.c
> > >
> > > Log Message:
> > > while (...);
> > > ;
> > > is really pointless, so remove the first semicolon.
> >
> > I tend to use an explicit 'continue' in such loops.
> > Much less confusing.
>
> I generally agree, but putting the semicolon on a separate line is good
> enough for the purpose.
At least netbsd puts { and } on the same line as the keyword,
otherwise you can get things like:
...
}
while ();
{
int foo;
...
where a cursory glance gives no real indication of what the
programmer intended (especially if the 'while' line is very long!)
David
--
David Laight: da...@l8s.co.uk
Re: CVS commit: src/usr.bin/tip
On Fri, Feb 24, 2012 at 08:11:19AM +, David Laight wrote: > On Thu, Feb 23, 2012 at 11:39:19PM +, Joerg Sonnenberger wrote: > > Module Name:src > > Committed By: joerg > > Date: Thu Feb 23 23:39:19 UTC 2012 > > > > Modified Files: > > src/usr.bin/tip: cmds.c > > > > Log Message: > > while (...); > > ; > > is really pointless, so remove the first semicolon. > > I tend to use an explicit 'continue' in such loops. > Much less confusing. I generally agree, but putting the semicolon on a separate line is good enough for the purpose. Joerg
Re: CVS commit: src/sys/arch/x86/x86
On 24 February 2012 13:41, Cherry G. Mathew wrote: > Module Name: src > Committed By: cherry > Date: Fri Feb 24 08:11:14 UTC 2012 > > Modified Files: > src/sys/arch/x86/x86: pmap.c > > Log Message: > kernel page attribute change should be reflected on all cpus, since > the page is going to be released after the _dtor() hook is called. > > > To generate a diff of this commit: > cvs rdiff -u -r1.170 -r1.171 src/sys/arch/x86/x86/pmap.c > Sorry, that broke the build. I'll fix it in a tick. -- ~Cherry
Re: CVS commit: src/usr.bin/tip
On Thu, Feb 23, 2012 at 11:39:19PM +, Joerg Sonnenberger wrote: > Module Name: src > Committed By: joerg > Date: Thu Feb 23 23:39:19 UTC 2012 > > Modified Files: > src/usr.bin/tip: cmds.c > > Log Message: > while (...); > ; > is really pointless, so remove the first semicolon. I tend to use an explicit 'continue' in such loops. Much less confusing. David -- David Laight: da...@l8s.co.uk
