update_mmu_cache vs. lazy_mmu_prot_update

update_mmu_cache vs. lazy_mmu_prot_update

[Christoph Lameter]

On Tue, 23 May 2006, Chen, Kenneth W wrote:

> > Except that, instead of agreeing it should be renamed, I say it should
> > be deleted entirely.  It seems to represent that ia64 has an empty
> > update_mmu_cache, and someone decided to add a new interface instead
> > of giving ia64 that work to do in its update_mmu_cache.
> 
> My memory recollects that it was done just like what you suggested:
> overloading update_mmu_cache for ia64, but it was vetoed by several mm
> experts.  And as a result a new function was introduced.

lazy_mmu_prot_update is always called after update_mmu_cache except
when we change permissions (hugetlb_change_protection() and 
change_pte_range()). 

So if we conflate those two then arches may have to be updated to avoid 
flushing the mmu if we only modified protections.

I think update_mmu_cache() should be dropped in page_wrprotect_one() in 
order to be consistent scheme. And avoiding mmu flushes will increase the 
performance of page_wrprotect_one.. lazy_mmu_prot_update must be there 
since we are changing permissions.

Re: update_mmu_cache vs. lazy_mmu_prot_update

[Hugh Dickins]

On Tue, 23 May 2006, Christoph Lameter wrote:
> On Tue, 23 May 2006, Chen, Kenneth W wrote:
> 
> > My memory recollects that it was done just like what you suggested:
> > overloading update_mmu_cache for ia64, but it was vetoed by several mm
> > experts.  And as a result a new function was introduced.
> 
> lazy_mmu_prot_update is always called after update_mmu_cache except
> when we change permissions (hugetlb_change_protection() and 
> change_pte_range()). 
> 
> So if we conflate those two then arches may have to be updated to avoid 
> flushing the mmu if we only modified protections.

Ah, I missed those two lone usages of lazy_mmu_prot_update, thanks.
That makes sense, and fits with Ken's recollection: to have added
update_mmu_cache in those two places would have slowed down the
other architectures.

> I think update_mmu_cache() should be dropped in page_wrprotect_one() in 
> order to be consistent scheme. And avoiding mmu flushes will increase the 
> performance of page_wrprotect_one.. lazy_mmu_prot_update must be there 
> since we are changing permissions.

Agreed.

I'd still like to rename lazy_mmu_prot_update, and refactor it, but
that can be a later unrelated cleanup.  What makes sense to me is to
call it update_mmu_cache_prot, and #define the ia64 update_mmu_cache
to that: so we can unclutter common code from most of the
lazy_mmu_prot_update lines, leaving just those two significant
instances of update_mmu_cache_prot that you highlight.

And of the two instances of update_mmu_cache in mm/fremap.c:
it seems to me that the first, in install_page, ought to have a
lazy_mmu_prot_update (and will get it automatically by the #define
I suggest); whereas the second, in install_file_pte, ought not to
have an update_mmu_cache since it's installing a !present entry.

Hugh