svn commit: r210846 - in head/sys/mips: include mips

[Jayachandran C.]

On Thu, Aug 12, 2010 at 12:36 PM, Alan Cox <alc at cs.rice.edu> wrote:
> Jayachandran C. wrote:
>>
>> On Fri, Aug 6, 2010 at 10:06 PM, Alan Cox <alc at 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.)

Will fix this.

>> 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.

I missed this part, thanks. Will check-in with the changes.

JC.