Re: [Xen-ia64-devel][PATCH][VTD] small patches for VTD

[Isaku Yamahata <yamahata@xxxxxxxxxxxxx>]

On Wed, Oct 22, 2008 at 02:50:40PM +0800, Xu, Anthony wrote:
> Isaku Yamahata wrote:
> > On Wed, Oct 22, 2008 at 02:30:50PM +0800, Xu, Anthony wrote:
> >> Isaku Yamahata wrote:
> >>> On Wed, Oct 22, 2008 at 02:11:52PM +0800, Xu, Anthony wrote:
> >>>> Isaku Yamahata wrote:
> >>>>> On Wed, Oct 22, 2008 at 01:56:05PM +0800, Xu, Anthony wrote:
> >>>>>> Yes, it is not SMP-safe there is lock for p2m.
> >>>>>> Modifying p2m is not a frequent operation, why not add a lock for
> >>>>>> it?
> >>>>>
> >>>>> It is frequent to read p2m table. So lockless approach was adopted
> >>>>> for scalability. It doesn't make sense to lock around only writer
> >>>>> side.
> >>>>>
> >>>>
> >>>> If only add write lock for p2m, is there any bad impact/senario?
> >>>> Can you explain more details?
> >>>
> >>> Generally lock should protect both readers and writers.
> >>> So locking around only writers doesn't make sense.
> >>
> >>
> >> So you can use read/write lock, multiple reader one writer.
> >> Because write is very rare, it will not impact performance, but it
> >> makes code in mm.c clear and easy to modify. I think that's why
> >> read/write lock exist.
> >
> > Yes, that's quite right.
> > It's another discussion to go for reader/writer lock or
> > to keep the current lockless approach.
> 
> If we want to keep the current lockless approach, we need to
> use ptep_cmpxchg_rel as less as possible
> 
> Currently there are four functions using ptep_cmpxchg_rel.
> assign_domain_page_cmpxchg_rel
> zap_domain_page_one
> replace_grant_host_mapping
> __assign_domain_page
> 
> And the four functions have similar function.
> 
> I suggest implementing below code segment as a core function
> __replace_domain_page(){
> 
> old_pte = ptep_xchg_rel();
> If(old_pte is INVALID)
> {
> }
> Elseif( old_pte is _PAGE_IO)
> {
> }
> Else if (old_pte is assigned_MMIO_page)
> {
> }
> Else if ( old_pte is normal page)
> {
>         put_page( old_page)
> }
> .....
> 
> We need to fully consider the old_pte type.
> 
> Other functions are simple wrapper of this core fucntion 
> __replace_domain_page.
> This way mm.c may be more readable.

Sounds great. Refactoring mm.c is good itself.
I have to admit that mm.c is unnecessarily complicated.
-- 
yamahata

_______________________________________________
Xen-ia64-devel mailing list
Xen-ia64-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-ia64-devel