Date: Thu, 12 Aug 2010 02:06:45 -0500 From: Alan Cox <alc@cs.rice.edu> To: "Jayachandran C." <c.jayachandran@gmail.com> Cc: "Jayachandran C." <jchandra@freebsd.org>, Alan Cox <alc@cs.rice.edu>, mips@freebsd.org Subject: Re: svn commit: r210846 - in head/sys/mips: include mips Message-ID: <4C639D85.9050000@cs.rice.edu> In-Reply-To: <AANLkTi=FowMNMy87aA2K=120a4L_Fd5GPDH%2BdEPKOsek@mail.gmail.com> References: <201008041412.o74ECAix092415@svn.freebsd.org> <4C5A569B.9090401@cs.rice.edu> <AANLkTinP7eMNm4yp6T2TTteSvthdgLJOj-ihHrQJ4T49@mail.gmail.com> <AANLkTi=vkG-cntJYYEdhO4AzOO91LB6n%2B45dUSxCMTp3@mail.gmail.com> <4C5BA088.7060105@cs.rice.edu> <AANLkTinxAkRTK8pLRkQ7JwesNkuwmuiRevOZMDpj_aj7@mail.gmail.com> <4C5C3A08.500@cs.rice.edu> <AANLkTi=FowMNMy87aA2K=120a4L_Fd5GPDH%2BdEPKOsek@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
Jayachandran C. wrote:
> On Fri, Aug 6, 2010 at 10:06 PM, Alan Cox <alc@cs.rice.edu> wrote:
>
>> The patch looks good.
>>
>> While we're talking about software dirty bit emulation, I would encourage
>> you to look at two things:
>>
>> 1. trap.c contains two copies of the same code for emulation. I would
>> encourage you to eliminate this duplication by creating a
>> pmap_emulate_modified().
>>
>> 2. Software dirty bit emulation is using pmap_update_page() to invalidate
>> the TLB entry on which the modified bit is being set. On a multiprocessor,
>> this is going to make dirty bit emulation very costly because every
>> processor will be interrupted. In principle, it should be possible and
>> faster to only flush the TLB entry from the current processor. The other
>> processors can handle this lazily. They either do not have that mapping in
>> their TLB, in which case interrupting them was wasted effort, or they do
>> have it in their TLB and when they fault on it they'll discover the dirty
>> bit is already set. In fact, the emulation code already handles this case,
>> on account of the fact that two processors could simultaneously write to the
>> same clean page and only one will get the pmap lock first.
>>
>
> I've made the changes suggested, the changes are attached.
>
> The first set of changes just re-arranges the pmap calls that use
> smp_rendezvous() on SMP, so that their per-cpu variants are also
> available to be called. The first patch also has an optimization from
> Juli's branch, to call pmap_update_page in pmap_kenter only if the pte
> is valid.
>
>
The patch looks good. style(9) requires a blank line after the opening
"{" here:
+static __inline void
+pmap_invalidate_all_local(pmap_t pmap)
+{
+ if (pmap == kernel_pmap) {
(There is also an extra blank line after the above "if" statement that
could be deleted.)
> The second patch makes the changes suggested above. My testing shows
> no issues so far, but please let me know if you have any comments.
>
I believe that you can now make pmap_update_page() static and delete the
declaration of pmap_set_modified() from pmap.h.
Alan
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?4C639D85.9050000>